* test(#3413): failing-first suite for the line-terminator seam Phase 2 of epic #3212 (ADR-3212 §3/§6/§7). Tests only — src/text-lines.cts does not exist yet, so tests/text-lines.test.cjs fails with MODULE_NOT_FOUND at its require line, which is the intended RED. The frontmatter.test.cjs additions drive #3360 (confirmed-bug) fail-first: parseMustHavesBlock currently returns [] for every must_haves block on a CRLF-authored plan file, because \r is its own LineTerminator in ECMAScript and two /m-anchored \s* patterns can absorb it, inflating a captured indent by one character and tripping the "not nested under must_haves" guard. Verified locally against the current (unfixed) compiled module: both the direct repro and the silent-exit "blank line before must_haves:" variant return [] today. A parity property test (crlf vs lf must deep-equal for every block name) matches a pattern this maintainer has required repeatedly for prior CRLF fixes in this codebase (Cortex-recorded, verify_intent=held). The no-crlf-fragile-split.rule.test.cjs additions lock the eslint rule's future fix-hint text (pointing at splitLines()) and its self-reference non-violation (the seam's own correct \r?\n split must never flag itself). Design: .gsd/phase/chore-3413-text-lines-seam/40-design.md Test matrix: .gsd/phase/chore-3413-text-lines-seam/50-test-matrix.md * chore(#3413): src/text-lines.cts owns line-terminator handling Phase 2 of epic #3212 (ADR-3212 §3/§6/§7). Adds splitLines/normalizeEol/ detectEol/joinLines and migrates frontmatter.cts onto it. parseMustHavesBlock (#3360, confirmed-bug) returned [] for every must_haves block on a CRLF plan file. Root cause: \r is its own LineTerminator in ECMAScript, so under /m two \s*-anchored indentation lookups could match at the position INSIDE a \r\n pair and absorb the terminator, inflating the captured indent by one character and tripping the "not nested under must_haves" guard. Two silent exits, one with a diagnostic and one without (a blank line before must_haves: hits the silent path). Fixed by converting both lookups from a whole-string /m match to split-then-scan — splitLines first, then a per-line, non-/m match — the same structural pattern parseYamlRegion (30 lines away in the same file) already used safely. Nothing downstream of the two lookups changed; blockLines is now sliced from the already-split array instead of re-splitting a substring, but its contents are unchanged for LF input, and the per-line dash/kv parsing loop is untouched. A parity property test (CRLF and LF plans parse to identical must_haves for every block name) matches a pattern this maintainer has required repeatedly for prior CRLF fixes in this file's neighborhood (Cortex: 7 recorded decisions, verify_intent -> held). frontmatter.cts's other .split(/\r?\n/) call sites (parseYamlRegion, isFrontmatterShaped, sliceTopLevelFrontmatterSegments, spliceFrontmatter) are rerouted onto splitLines — a literal 1:1 substitution, zero behavior change, since splitLines IS that same regex plus a type guard. The 4 scripts/normalizeLineEndings copies (gen-registry, gen-loop-host- contract, gen-capability-registry, gen-context-index) are deleted and rerouted onto normalizeEol, which strips a bare unpaired \r exactly like the deleted copies did (not just \r\n pairs) -- verified against each script's own --check mode against its real generated output. local/no-crlf-fragile-split widens from tests/ to src/**/*.cts, with its fix-hint message now naming splitLines() instead of the raw regex -- the prohibition finally has a primitive to point at. Detection logic unchanged in this phase (deliberate scope limit, see design doc Known limits: the rule doesn't yet recognize safeReadFile/platformReadSync as a content source, and has no detector for the \s-adjacent-to-anchor shape that is #3360's actual mechanism -- the CLASS is converged by the direct fix + regression test regardless). joinLines/detectEol are NOT wired into frontmatter.cts's own write path (cmdFrontmatterSet/Merge -> platformWriteSync) -- verified that platformWriteSync already, unconditionally converts CRLF->LF on every .md write today as a pre-existing policy owned by a different module, and ADR-3212's backward-compatibility clause rules out a file-format change in any phase. Stated explicitly in Known limits rather than left for a reader to discover. Six-gate ripple: .gitignore, eslint.config.mjs (src/**/*.cts block), docs/INVENTORY.md + INVENTORY-MANIFEST.json (regenerated), CONTEXT.md glossary (Text Lines Module, mirroring Phase 1's Pattern Module entry). Design: .gsd/phase/chore-3413-text-lines-seam/40-design.md Test matrix: .gsd/phase/chore-3413-text-lines-seam/50-test-matrix.md * fix(#3413): fix 13 pre-existing CRLF-fragile splits the widened rule found Widening local/no-crlf-fragile-split from tests/ to src/**/*.cts (the previous commit) immediately surfaced 13 real, pre-existing violations across 10 files -- undetected until now because the rule never scanned src/. This is the exact defect class ADR-3212 exists to close, playing out again one phase after Phase 1 hit the same shape ("the new lint rule -- once live -- found 27 more"). Per CLAUDE.md's no-defer rule, fixed inline rather than deferred or suppressed; there is no established suppression convention for this rule in src/ and inventing one now would undermine the point of widening it. audit.cts, broken-windows.cts, core-utils.cts, init.cts, milestone.cts, phase.cts (x3), profile-output.cts, roadmap.cts (x2): bare-\n splits or regex character classes widened to \r?\n / [^\r\n], each following the same pattern already established migrating frontmatter.cts. phase-estimation.cts: `\r?(?:\n|$)` restructured to `(?:\r?\n|\r?$)` -- already semantically CRLF-safe, but the rule's lexical scanner doesn't recognize \r? guarding a group (only \r? immediately before a literal \n). Verified the two forms are equivalent across all four EOL/EOF cases before restructuring, not assumed. roadmap-upgrade.cts needed two coupled sites, not the one flagged line: computeMigrationPlan and applyMigration must agree on line representation for the lines[edit.lineIndex] === edit.from equality check to hold, and the write-back needed joinLines + detectEol -- a plain lines.join('\n') was silently flattening a CRLF ROADMAP.md to LF wholesale on every migration. This is the first real production consumer of joinLines/detectEol in this epic (frontmatter.cts's own write path doesn't use them -- see the previous commit's Known limits). Fixing the 13 flagged sites surfaced 4 more adjacent same-shape sites the rule doesn't track (.search() and new RegExp(dynamicString) aren't in its tracked call/construction set). Investigated each empirically -- hand-tracing this exact bug class already produced one wrong conclusion earlier in this phase (a detectEol design-doc arithmetic error), so these were verified with real CRLF fixtures rather than reasoned about on paper: - audit.cts (scanTodos): REAL bug, fixed. `bodyMatch.trim().split ('\n')[0]` leaked a trailing \r into a user-visible todo summary on CRLF input -- .trim() only strips the string's outer edges, not a \r sitting mid-string before the first bare \n. Now splitLines(...) [0]. - phase.cts (cmdPhaseInsert, bullet-style branch): REAL bug, fixed. [^\n]* in targetBulletPattern swallowed a line's trailing \r on CRLF input, shifting the computed insert position to land INSIDE the \r\n pair; combined with a hardcoded '\n' bullet separator, a CRLF ROADMAP.md ended up with a mixed CRLF/LF result after an insert. Fixed with two coupled changes (either alone still corrupts, verified both ways): [^\r\n]* in the pattern, and the new bullet's leading terminator now comes from detectEol(rawContent). - roadmap.cts (cmdRoadmapAnnotateDependencies phase-boundary scan): investigated, genuinely safe, left untouched. The .search(/\n#{2,4} .../) boundary-finder and the [^\n]*-based heading match were empirically verified on a 3-phase CRLF fixture -- the only stray \r ends up at the tail of an intermediate phaseSection string that is only ever used for .test()-based idempotency checks, never for an exact-match comparison or written back to disk. No corruption on round-trip. Every fix re-verified: npm run build:lib clean, npx eslint 'src/**/*.cts' --no-cache reports 0 problems (was 13), and each fixed function's existing LF-input tests were spot-checked unchanged. * fix(#3413): apply orthogonal review findings Two isolated review engines (correctness + security) ran against the full diff and found three majors, one real security issue, and several disclosure-worthy minors. All fixed or explicitly disclosed with evidence; nothing deferred. MAJOR — detectEol's tie-break contradicted its own documented contract. Code returned '\n' on a 1:1 crlf/bare-LF tie; every doc (design doc, CONTEXT.md, the function's own comment) says ties resolve to '\r\n'. The existing test masked this by reusing the same tie fixture the buggy code happened to satisfy, rather than a genuine LF-majority case. Root cause: an Edit attempted earlier in this phase to fix this exact arithmetic error was blocked by the tier guard, and a later dispatch was incorrectly told it had already landed. Fixed: condition is now crlfCount >= bareLfCount; the test fixture corrected to a genuine 2:1 majority, with a new explicit tie-case test. MAJOR — phase.cts's cmdPhaseInsert built an EOL-aware bulletEntry via detectEol(rawContent), justified by a comment claiming a hardcoded '\n' corrupts a CRLF ROADMAP.md. False: this write goes through platformWriteSync, whose normalizeContent/_normalizeMd unconditionally converts CRLF->LF for any .md target — the templating was inert dead code, erased before the file is ever written. Reverted to hardcoded '\n', comment corrected to state the true reasoning. The separate [^\n]* -> [^\r\n]* widening one function up (a real splice-position fix, independent of final EOL) was kept. MAJOR — roadmap-upgrade.cts's stated rationale for switching onto splitLines/joinLines was wrong (both functions always agreed on line representation, before and after — the claimed equality-check risk never existed), and the change it justified introduced a real regression: forcing every line onto one dominant terminator silently rewrites untouched lines' EOL on a mixed-CRLF/LF ROADMAP.md. This write path uses raw fs.writeFileSync, not platformWriteSync, so unlike the phase.cts case above the regression is genuinely live. Fixing this took two attempts. The first attempt (revert to split('\n')/join('\n') plus a suppression comment) was correctly blocked by an agent that discovered local/no-crlf-fragile-split is a PROTECTED_RULES entry in tests/portability-rule-disable-ban.test.cjs — a hard, out-of-band, ADR-1703-governed guardrail banning any eslint-disable of this rule anywhere in src/**/*.cts. That agent also detected and correctly disregarded an injected instruction that appeared in tool output during a git operation, per this session's untrusted-content policy. The actual fix: computeMigrationPlan reverted to roadmapContent.split('\n') (confirmed lint-clean — the rule's data-flow tracking only follows a variable's initializer, and this one is declared empty then reassigned in a try block). applyMigration's write-back now splices edits against the ORIGINAL content string via indexOf('\n', pos) boundary-walking instead of a full split/rejoin, so every untouched character — including every line's own terminator — is copied byte-for-byte. A capture-group split (/(\r\n|\n)/, preserving terminators inline) was tried first and empirically confirmed to still trip the rule before this approach was chosen instead. MINOR (security) — roadmap.cts's cmdRoadmapAnnotateDependencies used the STRING form of String#replace, so $&, $`, $', $1-$9 inside must_haves.truths content (author-controlled) were interpreted as replacement directives, splicing unrelated ROADMAP.md text into the result. Fixed with the function-replacement form, which is never pattern-interpreted. Verified before/after with the reviewer's exact repro. Also disclosed rather than silently left: test matrix row 31 (four planned CRLF-materialized regression tests) was never implemented as separate files — corrected to record the actual verification (a manual --check run plus incidental existing coverage via each script's normalizeLineEndings: normalizeEol alias). parseMustHavesBlock's LF behavior was claimed byte-for-byte unchanged but the old yaml.indexOf(blockMatch[0]) substring search could match an unrelated earlier occurrence of the header text (e.g. inside a quoted value) — the split-then-scan fix incidentally also closes this, a strict improvement now recorded in the design doc rather than left implicit. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#3413): checkpoint 2 red — missing eslint ignore entry, RuleTester config error Checkpoint 2 came back red with 5 failures on the reviewed sha, both gaps genuinely undetectable by any local gate. eslint.config.mjs was missing the 'gsd-core/bin/lib/text-lines.cjs' ignores-list entry (ADR-457: generated .cjs artifacts are excluded from direct type-aware linting). Phase 1's sibling entry (pattern.cjs) sits two lines above it and was the exact precedent read while researching the six-gate ripple for this module -- missed anyway. Caught by tests/repo-invariants.test.cjs's bin/lib coverage-tracking test, which only runs on the remote suite. tests/no-crlf-fragile-split.rule.test.cjs's row-32 case specified both `messageId` and `message` on the same RuleTester error assertion -- ESLint's RuleTester rejects that combination outright. This existed since the test was first authored and was never caught locally: `npx eslint` only lints the file's syntax, it does not execute RuleTester, and local `node --test` is hard-blocked in this repo -- the assertion had never actually RUN before this checkpoint. It was even present in checkpoint 1's failure list, listed there as one of the "expected RED" tests; I matched it against my expected-failures list by test NAME only and never inspected the actual failure detail closely enough to notice it was failing for the wrong reason (a RuleTester config error, not the intended message-text mismatch). Fixed by keeping `message` (the exact-text assertion the test exists to make) and dropping `messageId`. Verified the crlfFragileSplit message string in eslint-rules/no-crlf-fragile-split.cjs matches this assertion character-for-character, and swept every other invalid case in the file for the same double-specification bug (none found -- all pre-existing cases use messageId alone). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(#3413): add Fixed changeset for the #3360 CRLF parsing fix The sole user-visible effect of this phase. No breaking-change label or Changed fragment needed — ADR-3212's Backward Compatibility section names the Node floor (Phase 1, already shipped) as the epic's only breaking change; Phase 2 has none. * chore(#3413): backfill changeset pr number to 3420 --------- Co-authored-by: sim <sim@local> Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
Tom Boucher
@@ -15,6 +15,7 @@ import fs from 'node:fs';
|
||||
import path from 'node:path';
|
||||
import { platformReadSync } from './shell-command-projection.cjs';
|
||||
import { collectSection } from './markdown-sectionizer.cjs';
|
||||
import { splitLines } from './text-lines.cjs';
|
||||
// eslint-disable-next-line @typescript-eslint/no-require-imports
|
||||
import planningWorkspace = require('./planning-workspace.cjs');
|
||||
const { planningDir } = planningWorkspace;
|
||||
@@ -411,8 +412,8 @@ function scanTodos(planDir: string): TodoItem[] {
|
||||
const fm = extractFrontmatter(content, safeFilePath);
|
||||
|
||||
// Extract first line of body after frontmatter
|
||||
const bodyMatch = content.replace(/^---[\s\S]*?---\n?/, '');
|
||||
const firstLine = bodyMatch.trim().split('\n')[0] || '';
|
||||
const bodyMatch = content.replace(/^---[\s\S]*?---\r?\n?/, '');
|
||||
const firstLine = splitLines(bodyMatch.trim())[0] || '';
|
||||
const summary = sanitizeForDisplay(firstLine.slice(0, 100));
|
||||
|
||||
results.push({
|
||||
|
||||
@@ -743,7 +743,7 @@ function writeLedgerAtomic(cwd: string, ledger: Ledger): void {
|
||||
if (fenceEnd !== -1) {
|
||||
const afterFence = existing.slice(fenceEnd + JSON_FENCE_CLOSE.length);
|
||||
// Drop leading newlines; keep the rest as prose.
|
||||
trailingProse = afterFence.replace(/^\n+/, '');
|
||||
trailingProse = afterFence.replace(/^(?:\r?\n)+/, '');
|
||||
}
|
||||
}
|
||||
} catch {
|
||||
|
||||
@@ -69,7 +69,7 @@ function detectSubRepos(cwd: string): string[] {
|
||||
function extractOneLinerFromBody(content: string | null | undefined): string | null {
|
||||
if (!content) return null;
|
||||
const normalized = content.replace(/\r\n/g, '\n').replace(/\r/g, '\n');
|
||||
const body = normalized.replace(/^---\n[\s\S]*?\n---\n*/, '');
|
||||
const body = normalized.replace(/^---\r?\n[\s\S]*?\r?\n---\r?\n*/, '');
|
||||
// #3170: anchor to a summary-shaped heading (Summary / Overview /
|
||||
// Accomplishments) so an incidental first heading (a rule list, task
|
||||
// breakdown, deviation note) does not contribute its first bold run as the
|
||||
|
||||
@@ -13,6 +13,7 @@ import ioMod = require('./io.cjs');
|
||||
const { output, error } = ioMod;
|
||||
import { platformReadSync as safeReadFile, platformWriteSync } from './shell-command-projection.cjs';
|
||||
import { textEncodingError } from './validate.cjs';
|
||||
import { splitLines } from './text-lines.cjs';
|
||||
// eslint-disable-next-line @typescript-eslint/no-require-imports
|
||||
import unusableInputMod = require('./unusable-input.cjs');
|
||||
const { UNUSABLE_REASON, warnUnusableInput } = unusableInputMod;
|
||||
@@ -84,7 +85,7 @@ const UNTERMINATED_KEY_THRESHOLD = 2;
|
||||
* has a false-positive class the other closes.
|
||||
*/
|
||||
function isFrontmatterShaped(region: string): boolean {
|
||||
const lines = region.split(/\r?\n/).filter((line) => line.trim() !== '');
|
||||
const lines = splitLines(region).filter((line) => line.trim() !== '');
|
||||
if (lines.length === 0) return false;
|
||||
return lines.every((line) => (
|
||||
/^\s*[a-zA-Z0-9_-]+:/.test(line) // key: value
|
||||
@@ -116,7 +117,7 @@ type FullLineCommentChannel = { leading: Record<string, string[]>; trailing: str
|
||||
*/
|
||||
function parseYamlRegion(yaml: string): Frontmatter {
|
||||
const frontmatter: Frontmatter = {};
|
||||
const lines = yaml.split(/\r?\n/);
|
||||
const lines = splitLines(yaml);
|
||||
|
||||
// #3257: pending column-0 full-line comments, attached to the next top-level key.
|
||||
let pendingComments: string[] = [];
|
||||
@@ -425,7 +426,7 @@ function propagateCommentChannel(source: Frontmatter, target: Frontmatter): void
|
||||
* not modify (e.g. must_haves.artifacts / .prohibitions).
|
||||
*/
|
||||
function sliceTopLevelFrontmatterSegments(yaml: string): Array<{ key: string; raw: string }> {
|
||||
const lines = yaml.split(/\r?\n/);
|
||||
const lines = splitLines(yaml);
|
||||
const segments: Array<{ key: string; raw: string }> = [];
|
||||
let current: { key: string; raw: string[] } | null = null;
|
||||
for (const line of lines) {
|
||||
@@ -491,7 +492,7 @@ function spliceFrontmatter(content: string, newObj: Frontmatter): string {
|
||||
// unrelated `must_haves` block. Keys absent from the original (genuinely new) are
|
||||
// regenerated and appended; keys absent from `newObj` are preserved (never silently
|
||||
// deleted by a set/merge).
|
||||
const fmLines = fmBlock.split(/\r?\n/);
|
||||
const fmLines = splitLines(fmBlock);
|
||||
const inner = fmLines.slice(1, -1).join('\n'); // drop the opening `---` and closing `---`
|
||||
let originalParsed: Frontmatter;
|
||||
try { originalParsed = extractFrontmatter(fmBlock); } catch { originalParsed = {}; }
|
||||
@@ -571,28 +572,28 @@ function parseMustHavesBlock(content: string, blockName: string): unknown[] {
|
||||
if (!fmMatch) return [];
|
||||
|
||||
const yaml = fmMatch[1];
|
||||
const yamlLines = splitLines(yaml);
|
||||
|
||||
// Find must_haves: first to detect its indentation level
|
||||
const mustHavesMatch = yaml.match(/^(\s*)must_haves:\s*$/m);
|
||||
if (!mustHavesMatch) return [];
|
||||
const mustHavesIndent = mustHavesMatch[1].length;
|
||||
// Find must_haves: first to detect its indentation level. Split-then-scan
|
||||
// (rather than a whole-string /m match) so a CRLF or blank-line boundary
|
||||
// can never be absorbed into the indent capture (#3360) — see
|
||||
// .gsd/phase/chore-3413-text-lines-seam/40-design.md.
|
||||
const mustHavesLinePattern = /^(\s*)must_haves:\s*$/;
|
||||
const mustHavesLineIndex = yamlLines.findIndex((line) => mustHavesLinePattern.test(line));
|
||||
if (mustHavesLineIndex === -1) return [];
|
||||
const mustHavesIndent = (yamlLines[mustHavesLineIndex].match(/^(\s*)/) as RegExpMatchArray)[1].length;
|
||||
|
||||
// Find the block (e.g., "truths:", "artifacts:", "key_links:") under must_haves
|
||||
// It must be indented more than must_haves but we detect the actual indent dynamically
|
||||
const blockPattern = new RegExp(`^(\\s+)${blockName}:\\s*$`, 'm');
|
||||
const blockMatch = yaml.match(blockPattern);
|
||||
if (!blockMatch) return [];
|
||||
const blockLinePattern = new RegExp(`^(\\s+)${blockName}:\\s*$`);
|
||||
const blockLineIndex = yamlLines.findIndex((line) => blockLinePattern.test(line));
|
||||
if (blockLineIndex === -1) return [];
|
||||
|
||||
const blockIndent = blockMatch[1].length;
|
||||
const blockIndent = (yamlLines[blockLineIndex].match(/^(\s*)/) as RegExpMatchArray)[1].length;
|
||||
// The block must be nested under must_haves (more indented)
|
||||
if (blockIndent <= mustHavesIndent) return [];
|
||||
|
||||
// Find where the block starts in the yaml string
|
||||
const blockStart = yaml.indexOf(blockMatch[0]);
|
||||
if (blockStart === -1) return [];
|
||||
|
||||
const afterBlock = yaml.slice(blockStart);
|
||||
const blockLines = afterBlock.split(/\r?\n/).slice(1); // skip the header line
|
||||
const blockLines = yamlLines.slice(blockLineIndex + 1); // skip the header line
|
||||
|
||||
// List items are indented one level deeper than blockIndent
|
||||
// Continuation KVs are indented one level deeper than list items
|
||||
|
||||
@@ -3683,7 +3683,7 @@ function buildSkillManifest(cwd: string, skillsDir: string | null = null): Skill
|
||||
|
||||
const description = (frontmatter['description'] as string) || '';
|
||||
const triggers: string[] = [];
|
||||
const bodyMatch = content.match(/^---[\s\S]*?---\s*\n([\s\S]*)$/);
|
||||
const bodyMatch = content.match(/^---[\s\S]*?---\s*\r?\n([\s\S]*)$/);
|
||||
if (bodyMatch) {
|
||||
const body = bodyMatch[1];
|
||||
const triggerLines = body.match(/^TRIGGER\s+when:\s*(.+)$/gmi);
|
||||
|
||||
@@ -824,7 +824,7 @@ function cmdMilestoneComplete(cwd: string, version: string, options: MilestoneCo
|
||||
platformWriteSync(milestonesPath, `# Milestones\n\n${milestoneEntry}`);
|
||||
} else {
|
||||
// Insert after the header line(s) for reverse chronological order (newest first)
|
||||
const headerMatch = existing.match(/^(#{1,3}\s+[^\n]*\n\n?)/);
|
||||
const headerMatch = existing.match(/^(#{1,3}\s+[^\r\n]*\r?\n(?:\r?\n)?)/);
|
||||
if (headerMatch) {
|
||||
const header = headerMatch[1];
|
||||
const rest = existing.slice(header.length);
|
||||
|
||||
@@ -341,7 +341,7 @@ export function extractFrontmatterBlock(text: unknown, key: string): Record<stri
|
||||
if (typeof text !== 'string') return null;
|
||||
|
||||
// Anchor at byte 0 — CRLF-tolerant.
|
||||
const fm = /^---\r?\n([\s\S]*?)\r?\n---\r?(?:\n|$)/.exec(text);
|
||||
const fm = /^---\r?\n([\s\S]*?)\r?\n---(?:\r?\n|\r?$)/.exec(text);
|
||||
if (fm === null) return null;
|
||||
|
||||
const lines = fm[1].split(/\r?\n/);
|
||||
|
||||
@@ -1317,10 +1317,25 @@ function cmdPhaseInsert(cwd: string, afterPhase: string, description: string, ra
|
||||
const phaseLabel = useBold
|
||||
? `**Phase ${_decimalPhase}: ${description}**`
|
||||
: `Phase ${_decimalPhase}: ${description}`;
|
||||
// #3413 review fix: bulletEntry stays hardcoded '\n'. The on-disk EOL
|
||||
// is decided at write time by platformWriteSync's normalizeContent /
|
||||
// _normalizeMd (shell-command-projection.cts), which unconditionally
|
||||
// converts \r\n -> \n for any .md target — so whatever terminator is
|
||||
// used here in memory is erased before the file is ever written, and
|
||||
// templating it via detectEol(rawContent) was inert dead code. '\n'
|
||||
// matches what platformWriteSync enforces anyway.
|
||||
const bulletEntry = `\n- [ ] ${phaseLabel}`;
|
||||
|
||||
// #3413: was `[^\n]*`, which on CRLF content swallows the line's
|
||||
// trailing \r into the match, shifting bulletLineEnd to land BETWEEN
|
||||
// the \r and \n of the original CRLF pair — a pure splice-POSITION
|
||||
// bug on the not-yet-write-normalized CRLF read (independent of the
|
||||
// final on-disk EOL, which platformWriteSync always forces to LF for
|
||||
// .md targets regardless). Widening to [^\r\n]* stops the match at the
|
||||
// true line-content boundary so bulletLineEnd lands cleanly before the
|
||||
// terminator.
|
||||
const targetBulletPattern = new RegExp(
|
||||
`(-\\s*\\[[ x]\\]\\s*(?:\\*\\*)?Phase\\s+${afterPhaseEscaped}${OPTIONAL_PHASE_TAG_SOURCE}[:\\s][^\\n]*)`,
|
||||
`(-\\s*\\[[ x]\\]\\s*(?:\\*\\*)?Phase\\s+${afterPhaseEscaped}${OPTIONAL_PHASE_TAG_SOURCE}[:\\s][^\\r\\n]*)`,
|
||||
'i',
|
||||
);
|
||||
const bulletMatchResult = rawContent.match(targetBulletPattern);
|
||||
@@ -1331,7 +1346,7 @@ function cmdPhaseInsert(cwd: string, afterPhase: string, description: string, ra
|
||||
const bulletLineEnd =
|
||||
rawContent.indexOf(bulletMatchResult![0]) + bulletMatchResult![0].length;
|
||||
const afterBullet = rawContent.slice(bulletLineEnd);
|
||||
const nextBulletMatch = afterBullet.match(/\n-\s*\[[ x]\]\s*(?:\*\*)?Phase\s+\d/i);
|
||||
const nextBulletMatch = afterBullet.match(/\r?\n-\s*\[[ x]\]\s*(?:\*\*)?Phase\s+\d/i);
|
||||
|
||||
let insertIdx: number;
|
||||
if (nextBulletMatch) {
|
||||
@@ -1357,7 +1372,7 @@ function cmdPhaseInsert(cwd: string, afterPhase: string, description: string, ra
|
||||
|
||||
const headerIdx = rawContent.indexOf(headerMatch![0]);
|
||||
const afterHeader = rawContent.slice(headerIdx + headerMatch![0].length);
|
||||
const nextPhaseMatch = afterHeader.match(/\n#{2,4}\s+Phase\s+\d[\d.]*/i);
|
||||
const nextPhaseMatch = afterHeader.match(/\r?\n#{2,4}\s+Phase\s+\d[\d.]*/i);
|
||||
|
||||
let insertIdx: number;
|
||||
if (nextPhaseMatch) {
|
||||
@@ -1630,7 +1645,7 @@ function updateRoadmapAfterPhaseRemoval(
|
||||
// #1729: fold an optional pre-colon ( ) tag into the suffix capture so it
|
||||
// is re-emitted verbatim — a tagged later phase still gets renumbered.
|
||||
content = content.replace(
|
||||
/(#{2,4}\s*Phase\s+)(\d+(?:\.\d+)?)((?:\s*\([^)\n]{0,200}\))?\s*:)/gi,
|
||||
/(#{2,4}\s*Phase\s+)(\d+(?:\.\d+)?)((?:\s*\([^)\r\n]{0,200}\))?\s*:)/gi,
|
||||
(_match, prefix: string, num: string, suffix: string) =>
|
||||
`${prefix}${decrementRoadmapPhaseToken(num, removedInt)}${suffix}`,
|
||||
);
|
||||
|
||||
@@ -553,7 +553,7 @@ function generateSkillsSection(cwd: string): SectionResult {
|
||||
*/
|
||||
function extractSkillFrontmatter(content: string): { name: string; description: string } {
|
||||
const result = { name: '', description: '' };
|
||||
const fmMatch = content.match(/^---\s*\n([\s\S]*?)\n---/);
|
||||
const fmMatch = content.match(/^---\s*\r?\n([\s\S]*?)\r?\n---/);
|
||||
if (!fmMatch) return result;
|
||||
|
||||
const fmBlock = fmMatch[1];
|
||||
|
||||
@@ -465,6 +465,42 @@ function computeMigrationPlan(cwd: string, options: Record<string, unknown> = {}
|
||||
};
|
||||
}
|
||||
|
||||
/**
|
||||
* Apply roadmap line edits via character-offset splicing against the
|
||||
* ORIGINAL content string — never a full split/rejoin (#3413). `lineIndex`
|
||||
* boundaries are found by scanning for the next bare `\n`, exactly matching
|
||||
* how computeMigrationPlan() itself indexes lines (`roadmapContent.split('\n')`)
|
||||
* — both sides must agree on line indexing for `lineText === edit.from` to
|
||||
* match, and this keeps a `\r` that precedes a `\n` as part of the LINE text
|
||||
* rather than a separately-normalized terminator. Only a line whose text
|
||||
* exactly equals an edit's `from` is replaced; every other character —
|
||||
* including every line's own terminator, touched or not — is copied
|
||||
* byte-for-byte from the original, so a mixed-EOL ROADMAP.md never has its
|
||||
* untouched lines silently flattened to one dominant style.
|
||||
*/
|
||||
function applyRoadmapEdits(content: string, edits: RoadmapEdit[]): string {
|
||||
const editByLine = new Map<number, RoadmapEdit>();
|
||||
for (const edit of edits) editByLine.set(edit.lineIndex, edit);
|
||||
|
||||
let result = '';
|
||||
let pos = 0;
|
||||
let lineIndex = 0;
|
||||
|
||||
for (;;) {
|
||||
const nlIdx = content.indexOf('\n', pos);
|
||||
const lineEnd = nlIdx === -1 ? content.length : nlIdx;
|
||||
const lineText = content.slice(pos, lineEnd);
|
||||
const edit = editByLine.get(lineIndex);
|
||||
result += edit && lineText === edit.from ? edit.to : lineText;
|
||||
if (nlIdx === -1) break;
|
||||
result += '\n';
|
||||
pos = nlIdx + 1;
|
||||
lineIndex++;
|
||||
}
|
||||
|
||||
return result;
|
||||
}
|
||||
|
||||
// ─── applyMigration ───────────────────────────────────────────────────────────
|
||||
|
||||
/**
|
||||
@@ -538,18 +574,10 @@ function applyMigration(cwd: string, plan: MigrationPlan, options: { dryRun?: bo
|
||||
// 2. Rewrite ROADMAP.md phase headings
|
||||
if (plan.roadmapEdits.length > 0) {
|
||||
const roadmapContent = fs.readFileSync(roadmapPath, 'utf8');
|
||||
const lines = roadmapContent.split('\n');
|
||||
|
||||
// Sort edits by lineIndex to apply in order
|
||||
const sortedEdits = [...plan.roadmapEdits].sort((a, b) => a.lineIndex - b.lineIndex);
|
||||
for (const edit of sortedEdits) {
|
||||
if (lines[edit.lineIndex] === edit.from) {
|
||||
lines[edit.lineIndex] = edit.to;
|
||||
}
|
||||
}
|
||||
const newRoadmapContent = applyRoadmapEdits(roadmapContent, plan.roadmapEdits);
|
||||
|
||||
snapshotFile(roadmapPath);
|
||||
fs.writeFileSync(roadmapPath, lines.join('\n'), 'utf8');
|
||||
fs.writeFileSync(roadmapPath, newRoadmapContent, 'utf8');
|
||||
editedFiles.push('ROADMAP.md');
|
||||
}
|
||||
|
||||
|
||||
@@ -10,6 +10,7 @@ import fs from 'node:fs';
|
||||
import path from 'node:path';
|
||||
import { realClock } from './clock.cjs';
|
||||
import { escapeRegex } from './pattern.cjs';
|
||||
import { splitLines, detectEol, joinLines } from './text-lines.cjs';
|
||||
// eslint-disable-next-line @typescript-eslint/no-require-imports
|
||||
import ioMod = require('./io.cjs');
|
||||
const { output, error } = ioMod;
|
||||
@@ -973,6 +974,11 @@ function cmdRoadmapAnnotateDependencies(cwd: string, phaseNum: string | null | u
|
||||
let updated = false;
|
||||
withPlanningLock(cwd, () => {
|
||||
const content = fs.readFileSync(roadmapPath, 'utf-8');
|
||||
// #3413: preserve the file's own EOL style when the checklist block below
|
||||
// is rebuilt and spliced back in — splitLines() cleans each captured line
|
||||
// of any dangling \r, so rejoining with a bare '\n' would silently
|
||||
// downgrade a CRLF ROADMAP.md's rewritten block to LF only.
|
||||
const eol = detectEol(content);
|
||||
|
||||
// Find the phase section.
|
||||
// #3537: padding-tolerant fragment so the caller's resolved padded id
|
||||
@@ -1006,12 +1012,12 @@ function cmdRoadmapAnnotateDependencies(cwd: string, phaseNum: string | null | u
|
||||
// Review fix (F2): `(?:^|\n)` anchors the match to start-of-line so mid-line
|
||||
// occurrences like `***Plans:***` embedded in a sentence or `OpenPlans: foo`
|
||||
// do not trigger a false match. Groups 1 and 2 retain the same semantics.
|
||||
const plansBlockMatch = phaseSection.match(/(?:^|\n)(\*{0,2}Plans\*{0,2}:[^\n]*\n)((?:\s*-\s*\[[ x]\][^\n]*\n?)+)/i);
|
||||
const plansBlockMatch = phaseSection.match(/(?:^|\r?\n)(\*{0,2}Plans\*{0,2}:[^\r\n]*\r?\n)((?:\s*-\s*\[[ x]\][^\r\n]*\r?\n?)+)/i);
|
||||
if (!plansBlockMatch) return;
|
||||
|
||||
const plansHeader = plansBlockMatch[1];
|
||||
const existingList = plansBlockMatch[2];
|
||||
const listLines = existingList.split('\n').filter(l => /^\s*-\s*\[/.test(l));
|
||||
const listLines = splitLines(existingList).filter(l => /^\s*-\s*\[/.test(l));
|
||||
|
||||
if (listLines.length === 0) return;
|
||||
|
||||
@@ -1065,13 +1071,24 @@ function cmdRoadmapAnnotateDependencies(cwd: string, phaseNum: string | null | u
|
||||
}
|
||||
}
|
||||
|
||||
const newListBlock = annotatedLines.join('\n') + '\n';
|
||||
// #1103: when `(?:^|\n)` consumed a leading `\n` (mid-string match), re-emit it
|
||||
// so the line preceding the Plans: header is not fused onto it.
|
||||
const leadingNewline = plansBlockMatch[0].startsWith('\n') ? '\n' : '';
|
||||
const newListBlock = joinLines(annotatedLines, eol) + eol;
|
||||
// #1103: when `(?:^|\r?\n)` consumed a leading terminator (mid-string
|
||||
// match), re-emit it verbatim so the line preceding the Plans: header is
|
||||
// not fused onto it. #3413: the widened `(?:^|\r?\n)` can now consume a
|
||||
// 2-char `\r\n` — re-emit whatever was actually captured (`''`, `'\n'`,
|
||||
// or `'\r\n'`), not a hardcoded `'\n'`, or a CRLF file loses its `\r`.
|
||||
const leadingMatch = /^\r?\n/.exec(plansBlockMatch[0]);
|
||||
const leadingNewline = leadingMatch ? leadingMatch[0] : '';
|
||||
// Review fix (#3413 security): use the FUNCTION-replacement form. The
|
||||
// string-replacement form expands String#replace's special patterns
|
||||
// (`$&`, `` $` ``, `$'`, `$$`, `$1`-`$9`) inside the replacement — and
|
||||
// newListBlock is built from author-controlled truths/plan-file content,
|
||||
// so a line containing a literal `` $` `` (etc.) would splice unrelated
|
||||
// surrounding phaseSection text into the result. A function replacer is
|
||||
// never pattern-interpreted.
|
||||
const newPhaseSection = phaseSection.replace(
|
||||
plansBlockMatch[0],
|
||||
leadingNewline + plansHeader + newListBlock
|
||||
() => leadingNewline + plansHeader + newListBlock
|
||||
);
|
||||
|
||||
const nextContent = content.slice(0, phaseStart) + newPhaseSection + content.slice(phaseEnd);
|
||||
|
||||
77
src/text-lines.cts
Normal file
77
src/text-lines.cts
Normal file
@@ -0,0 +1,77 @@
|
||||
/**
|
||||
* text-lines.cts — the line-terminator seam (ADR-3212 §3, epic #3212 Phase 2,
|
||||
* #3413).
|
||||
*
|
||||
* Source in src/text-lines.cts, compiled to gsd-core/bin/lib/text-lines.cjs
|
||||
* (gitignored), per the repo's ADR-457 build-at-publish convention.
|
||||
*
|
||||
* Sole owner of `\r?\n` splitting and CRLF normalization. Closes #3360: a
|
||||
* regex anchored on `^`/`$` under `/m` against a whole multi-line string is
|
||||
* exposed to `\r` being its own LineTerminator (ECMA-262), so a greedy `\s*`
|
||||
* can cross a CRLF boundary and inflate a captured indent by one character.
|
||||
* Split-then-match — call `splitLines` first, then match per already-split
|
||||
* line — is immune by construction, because a single split line can never
|
||||
* contain the delimiter that produced it. This module exists so that shape
|
||||
* is the easy, obvious way to write indentation-sensitive parsing, per the
|
||||
* design doc (.gsd/phase/chore-3413-text-lines-seam/40-design.md).
|
||||
*
|
||||
* A "genuine leaf" module (CONTEXT.md's term, shared with Phase 1's sibling
|
||||
* seam `src/pattern.cts`): zero I/O, zero imports from other `src/*.cts`
|
||||
* modules, pure string functions only.
|
||||
*/
|
||||
|
||||
/**
|
||||
* Split document content into lines on `\r\n` or `\n`. A lone `\r` (bare CR,
|
||||
* no following `\n`) is NOT a delimiter — this matches every existing
|
||||
* `\r?\n` call site in the repo and is intentionally narrower than "treat
|
||||
* any LineTerminator as a break." Matches `String.prototype.split`'s own
|
||||
* contract for a trailing terminator (a trailing empty element) and for the
|
||||
* empty string (`[""]`).
|
||||
*/
|
||||
export function splitLines(content: string): string[] {
|
||||
if (typeof content !== 'string') {
|
||||
throw new TypeError(`splitLines: expected a string, got ${typeof content}`);
|
||||
}
|
||||
return content.split(/\r?\n/);
|
||||
}
|
||||
|
||||
/**
|
||||
* Normalize all line endings to LF by stripping every `\r` character — not
|
||||
* only `\r\n` pairs. This is byte-for-byte the same operation as the 4
|
||||
* `scripts/gen-*.cjs` copies of `normalizeLineEndings` this module
|
||||
* consolidates (`content.replace(/\r/g, '')`), so a lone/unpaired `\r` (old
|
||||
* Mac-style content) is stripped too. Deliberately a different contract from
|
||||
* `splitLines`, which does NOT treat a bare `\r` as a delimiter (row 6) —
|
||||
* these are two different operations kept as two separate functions rather
|
||||
* than one "handle all CR-ish things" primitive.
|
||||
*/
|
||||
export function normalizeEol(content: string): string {
|
||||
return content.replace(/\r/g, '');
|
||||
}
|
||||
|
||||
/**
|
||||
* Detect the dominant line-terminator in `content`: `'\r\n'` when CRLF pairs
|
||||
* outnumber bare LFs, `'\n'` when bare LFs outnumber CRLF pairs, and `'\r\n'`
|
||||
* as the default when there is no bare LF at all (all-CRLF content, or no
|
||||
* terminator present whatsoever — an empty or single-line input). See the
|
||||
* design doc's Rejected section for why the tie/empty default is `'\r\n'`
|
||||
* rather than `'\n'`: this function exists to PRESERVE whatever a file
|
||||
* already uses, not to bias toward LF the way a write-time normalizer would.
|
||||
*/
|
||||
export function detectEol(content: string): '\n' | '\r\n' {
|
||||
const crlfCount = (content.match(/\r\n/g) || []).length;
|
||||
const totalLfCount = (content.match(/\n/g) || []).length;
|
||||
const bareLfCount = totalLfCount - crlfCount;
|
||||
if (bareLfCount === 0 || crlfCount >= bareLfCount) return '\r\n';
|
||||
return '\n';
|
||||
}
|
||||
|
||||
/**
|
||||
* Join lines back into a single string with `eol` (default `'\n'`) between
|
||||
* each pair — the inverse of `splitLines`. `joinLines(splitLines(x),
|
||||
* detectEol(x))` reproduces `x` byte-for-byte for any `x` already in
|
||||
* canonical (single-terminator) form (ADR-3212 §3's round-trip property).
|
||||
*/
|
||||
export function joinLines(lines: string[], eol?: '\n' | '\r\n'): string {
|
||||
return lines.join(eol ?? '\n');
|
||||
}
|
||||
Reference in New Issue
Block a user