From 04a0eb8d63615d15f373f3cdc0f000fb404dcd5c Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Tue, 21 Jul 2026 09:53:14 -0400 Subject: [PATCH] fix(#2450): CRLF-tolerant session-section rewrite + no-op-detection guard (#2482) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(#2444): re-resolve body-parser to 2.3.0 in lockfile (GHSA-v422-hmwv-36x6) GHSA-v422-hmwv-36x6 (body-parser DoS via invalid limit value, low severity, published 2026-07-20T23:23:26Z) made tests/npm-integrity-gate.test.cjs (#3588: root workspace production tree has no advisories) fail any subsequent npm audit --omit=dev. The advisory affects body-parser >=2.0.0 <2.3.0 pulled transitively via @anthropic-ai/claude-agent-sdk -> @modelcontextprotocol/sdk -> express -> body-parser@2.2.2. express@5.2.1 already declares body-parser as ^2.2.1, so 2.3.0 is a valid re-resolution within express's own compatibility range — no override needed. Regenerated the lockfile via 'npm audit fix --omit=dev' which re-resolves transitive deps within their declared ranges; package.json is unchanged. Verified: npm audit --omit=dev reports 0/0/0/0/0 advisories; body-parser now reads as 2.3.0 in 'npm ls body-parser --omit=dev'. * test(#2450): failing-first CRLF regression for record-session insert path cmdStateRecordSession's section-rewrite regexes (src/state.cts:1166, :1195) used literal \\n which cannot match CRLF STATE.md delimiters. The detector regex (CRLF-tolerant via $ under /m) entered the rewrite branch, the writer regex silently no-op'd, but updated.push(...)/ sessionCreated=true ran unconditionally. Result: caller reported recorded:true with 'Resume File' in updated, but the field was never written to disk. With core.autocrlf=input, the CRLF working-tree file produces no git diff, so the bug was invisible. Adds three regression tests covering all three rewrite paths: - CRLF STATE.md with ## Session and Resume file absent - CRLF STATE.md with ## Session and Stopped at absent - CRLF STATE.md with ## Session Continuity (bootstrap shape) Each asserts the field IS on disk (the bug discriminator: the pre-fix command's JSON output looked identical to a successful write). * fix(#2450): CRLF-tolerant session-section rewrite + no-op-detection guard Two regexes in cmdStateRecordSession used literal \\n which cannot match CRLF STATE.md, silently no-op'ing the section rewrite while the CRLF- tolerant detector above entered the branch. The reporter's exact repro: on a CRLF STATE.md with one canonical session field absent, the command returned recorded:true + updated:['Resume File'] but the field was never written to disk. Three changes: 1. src/state.cts:1166 (canonical ## Session rewrite regex): \\n -> \\r?\\n 2. src/state.cts:1195 (## Session Continuity insert regex): \\n -> \\r?\\n 3. Defensive invariant (#2450 class fix per reporter's suggestion): track whether the chosen branch's replace actually matched via callback flag. Only set sessionCreated=true and push to updated when rewriteMatched. Unreachable post-fix, but fail-loud is the right posture for a silent- success gate. If a future drift between the detector and writer regexes reintroduces the asymmetry, the caller will not see false updated entries. Same canonical CRLF-tolerant form already in use at check-command-router.cts :205 (extractPlanDesignatedSections). Same bug class previously fixed in #1658, #1668, #2206, #2449. * fix(#2450): address review followups + add changeset Code-review + security-review both flagged the unreachable else at the Session Continuity branch (defaulted rewriteMatched=true in dead code, re-arming the bug class for future drift). Removed the else; the remaining code path leaves rewriteMatched=false if linesToInsert is empty, preserving the fail-loud posture. Added scope-limitation doc to the rewriteMatched gate: it covers the INSERT path only, not the earlier in-place stateReplaceField successes (which DID land on disk and correctly push to updated unconditionally). Tests: - Normalized STATE_CRLF_SESSION_MISSING_RESUME fixture to match the canonical 6-key frontmatter of STATE_WITH_SESSION (code-review I2). - Added mixed-ending test (LF frontmatter + CRLF body) to close CONTRIBUTING.md:490 'Mixed CRLF/LF newlines' requirement (I1). Added Fixed changeset (code-review H1). * docs(changeset): backfill PR number to 2482 --- .changeset/eight-foxes-cheer.md | 5 + src/state.cts | 81 ++++++++++++---- tests/state.test.cjs | 162 ++++++++++++++++++++++++++++++++ 3 files changed, 231 insertions(+), 17 deletions(-) create mode 100644 .changeset/eight-foxes-cheer.md diff --git a/.changeset/eight-foxes-cheer.md b/.changeset/eight-foxes-cheer.md new file mode 100644 index 000000000..d674babc7 --- /dev/null +++ b/.changeset/eight-foxes-cheer.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 2482 +--- +**`state record-session` no longer silently drops inserted fields on a CRLF `STATE.md`** — the section-rewrite regexes in `cmdStateRecordSession` used literal `\n` which couldn't match a CRLF STATE.md (`---\r\n`), so when a canonical session field (`Resume file` / `Stopped at` / `Last session`) was missing and had to be **inserted** via the section-rewrite path, the CRLF-tolerant detector entered the branch, the writer regex silently no-op'd, but `updated.push(...)` ran unconditionally. The command returned `{"recorded": true, "updated": ["Resume File"]}` while the field was never written to disk. With `core.autocrlf=input`, the CRLF working-tree file produced no `git diff`/`git status` change, so the bug was invisible. Both regexes now use the CRLF-tolerant `\r?\n` form (same canonical pattern already in use elsewhere), and a new defensive invariant gates `updated.push(...)` on the replace callback actually firing — so a future detector/writer drift will surface as missing `updated` entries rather than re-arming this silent-success class. diff --git a/src/state.cts b/src/state.cts index 11179a601..720d72033 100644 --- a/src/state.cts +++ b/src/state.cts @@ -1155,6 +1155,16 @@ function cmdStateRecordSession(cwd: string, options: StateRecordSessionOptions, const existingCanonicalSession = /^## Session[ \t]*$/im.test(content); const existingSessionContinuity = /^## Session Continuity[ \t]*$/im.test(content); + // Track whether the chosen branch's rewrite actually matched. The detector + // regexes (existingCanonicalSession/existingSessionContinuity) are CRLF- + // tolerant ($ under /m treats \r as a line terminator); the writer regexes + // below must be too. If a writer regex silently fails to match (line-ending + // mismatch, unexpected heading shape, ...), do NOT report success — the + // caller would believe fields were persisted that were silently dropped + // (#2450). The append branch always sets rewriteMatched=true (it always + // mutates content). + let rewriteMatched = false; + if (existingCanonicalSession) { // Normalize in place: replace the ENTIRE BODY of the existing ## Session // section (heading + all content up to the next ## heading or EOF) with @@ -1162,17 +1172,27 @@ function cmdStateRecordSession(cwd: string, options: StateRecordSessionOptions, // `(?!^## )[\s\S]` consumes every line that doesn't start with "## ", // which correctly stops at the next section boundary without consuming it. // A trailing blank line is added so the next ## heading keeps its spacing. + // + // CRLF-tolerant (`\r?\n` after `[ \t]*`): the prior literal `\n` could not + // match a CRLF STATE.md (`---\r\n`), silently no-op'ing the replace while + // updated.push(...) reported success — #2450. The detector regex on the + // line above (`/^## Session[ \t]*$/im`) was already CRLF-tolerant, so the + // asymmetry armed the bug. + const canonicalReplacement = [ + '## Session', + '', + `**Last session:** ${now}`, + `**Stopped at:** ${stoppedAtValue}`, + `**Resume file:** ${resumeValue}`, + '', + '', + ].join('\n'); content = content.replace( - /^(## Session[ \t]*\n(?:(?!^## )[\s\S])*)/m, - [ - '## Session', - '', - `**Last session:** ${now}`, - `**Stopped at:** ${stoppedAtValue}`, - `**Resume file:** ${resumeValue}`, - '', - '', - ].join('\n'), + /^(## Session[ \t]*\r?\n(?:(?!^## )[\s\S])*)/m, + () => { + rewriteMatched = true; + return canonicalReplacement; + }, ); } else if (existingSessionContinuity) { // #1101: a `## Session Continuity` section already exists (bootstrap @@ -1183,6 +1203,8 @@ function cmdStateRecordSession(cwd: string, options: StateRecordSessionOptions, // (e.g. prose like "Next recommended action"). Fields already updated in // place above (needs* false) are not re-inserted. A function replacement // is used so `$`-bearing caller values are inserted literally (#3454). + // + // CRLF-tolerant (`\r?\n`): same #2450 fix as the canonical branch above. const linesToInsert: string[] = []; if (needsLastSession) linesToInsert.push(`**Last session:** ${now}`); if (needsStoppedAt) linesToInsert.push(`**Stopped at:** ${stoppedAtValue}`); @@ -1192,10 +1214,20 @@ function cmdStateRecordSession(cwd: string, options: StateRecordSessionOptions, // above (#1101 review F3) — otherwise a lowercase heading would detect // but no-op the insert while still reporting the fields as updated. content = content.replace( - /^(## Session Continuity[ \t]*\n)/im, - (_m, heading: string) => heading + linesToInsert.join('\n') + '\n', + /^(## Session Continuity[ \t]*\r?\n)/im, + (_m, heading: string) => { + rewriteMatched = true; + return heading + linesToInsert.join('\n') + '\n'; + }, ); } + // No `else` branch: if linesToInsert.length === 0 the outer guard at + // :1144 (callerSuppliedValues && (needsStoppedAt || needsResumeFile + // || needsLastSession)) could not have fired, so this whole block is + // unreachable. Leaving `rewriteMatched = false` here is the fail-loud + // posture — a future change to the outer guard or needs* computation + // that makes this branch reachable will surface as a missing + // updated[] entry (silent recorded:false) rather than re-arming #2450. } else { // No session heading exists at all — append a new canonical section. const scaffold = [ @@ -1208,13 +1240,28 @@ function cmdStateRecordSession(cwd: string, options: StateRecordSessionOptions, '', ].join('\n'); content = content.trimEnd() + '\n' + scaffold; + rewriteMatched = true; } - sessionCreated = true; - - if (needsLastSession) updated.push('Last session'); - if (needsStoppedAt) updated.push('Stopped At'); - if (needsResumeFile) updated.push('Resume File'); + // #2450 defensive invariant: only report sessionCreated/updated when the + // chosen branch's rewrite actually mutated content. Unreachable when the + // writer regexes above stay in sync with the CRLF-tolerant detector — + // but unreachable-defensive is the right posture for a silent-success + // gate. A no-op replace here means a future line-ending or shape drift + // between detector and writer; fail to record rather than claim success. + // + // Scope limitation (not a regression of this fix): the gate covers only + // the section-rewrite block. The earlier in-place stateReplaceField + // successes at :1081/:1083/:1089/:1101/:1108/:1114 push to `updated` + // unconditionally — those represent fields that DID land on disk via + // same-line replace (CRLF-agnostic seam), so unconditional push is + // correct. The class-defect防御 here is for the INSERT path only. + if (rewriteMatched) { + sessionCreated = true; + if (needsLastSession) updated.push('Last session'); + if (needsStoppedAt) updated.push('Stopped At'); + if (needsResumeFile) updated.push('Resume File'); + } } return content; diff --git a/tests/state.test.cjs b/tests/state.test.cjs index edfbbfa80..2665f9953 100644 --- a/tests/state.test.cjs +++ b/tests/state.test.cjs @@ -4806,6 +4806,168 @@ describe('T6 section-splice characterization — record-session', () => { cleanup(d); } }); + + // ─── #2450: CRLF STATE.md must not silently drop inserted session fields ───── + // + // Bug class: cmdStateRecordSession's section-rewrite regex used literal \n + // for the `## Session` and `## Session Continuity` heading boundaries, which + // cannot match a CRLF STATE.md (---\r\n style). The detector regex (using + // /^## Session[ \t]*$/im) WAS CRLF-tolerant ($ under /m treats \r as a line + // terminator), so the bug fired when a canonical session field was missing + // and had to be INSERTED via the section-rewrite path: the detector entered + // the branch, the writer regex silently no-op'd, but `updated.push('Resume File')` + // ran unconditionally → reported as updated but never written to disk. + // + // With core.autocrlf=input, the CRLF working-tree file produces no git diff, + // so the contributor cannot tell their STATE.md was misclassified. + + const STATE_CRLF_SESSION_MISSING_RESUME = [ + '---', + "gsd_state_version: '1.0'", + 'milestone: v1.0', + 'milestone_name: TestMilestone', + 'status: executing', + "last_updated: '2026-01-01T00:00:00.000Z'", + "last_activity: '2026-01-01'", + '---', + '', + '# Project State', + '', + '## Session', + '', + '**Last session:** 2026-01-01T00:00:00.000Z', + '**Stopped at:** None', + // Resume file absent — forces the section-rewrite insert path (the buggy block) + '', + ].join('\r\n'); + + test('record-session --resume-file on CRLF STATE.md with field absent: field IS written (#2450)', () => { + const d = createTempProject(); + try { + fs.writeFileSync(path.join(d, '.planning', 'STATE.md'), STATE_CRLF_SESSION_MISSING_RESUME); + const result = runGsdTools(['state', 'record-session', '--resume-file', 'plan-3.md'], d); + assert.ok(result.success, `Command failed: ${result.error}`); + const output = JSON.parse(result.output); + + // Bug discriminator: command reported recorded:true + 'Resume File' in + // updated, but the field was never written to disk. The disk assertion + // below is what fails pre-fix and passes post-fix. + assert.ok(Array.isArray(output.updated) && output.updated.includes('Resume File'), + `updated must include 'Resume File': ${JSON.stringify(output.updated)}`); + + const after = fs.readFileSync(path.join(d, '.planning', 'STATE.md'), 'utf-8'); + assert.ok( + /\*\*Resume file:\*\*\s*plan-3\.md/.test(after), + `Resume file field must be on disk after the call; STATE.md head:\n${after.slice(0, 500)}`, + ); + } finally { + cleanup(d); + } + }); + + test('record-session --stopped-at on CRLF STATE.md with field absent: field IS written (#2450)', () => { + // Same bug class, different field. The bug fires whenever a canonical + // session field must be INSERTED (vs. same-line replaced) on a CRLF STATE.md. + const STATE_CRLF_SESSION_MISSING_STOPPED = [ + '---', + 'status: executing', + '---', + '', + '## Session', + '', + '**Last session:** 2026-01-01T00:00:00.000Z', + // Stopped at absent — forces the section-rewrite insert path + '**Resume file:** None', + '', + ].join('\r\n'); + + const d = createTempProject(); + try { + fs.writeFileSync(path.join(d, '.planning', 'STATE.md'), STATE_CRLF_SESSION_MISSING_STOPPED); + const result = runGsdTools(['state', 'record-session', '--stopped-at', '14.3'], d); + assert.ok(result.success, `Command failed: ${result.error}`); + + const after = fs.readFileSync(path.join(d, '.planning', 'STATE.md'), 'utf-8'); + assert.ok( + /\*\*Stopped at:\*\*\s*14\.3/.test(after), + `Stopped at field must be on disk; STATE.md head:\n${after.slice(0, 500)}`, + ); + } finally { + cleanup(d); + } + }); + + test('record-session --resume-file on CRLF STATE.md with Session Continuity heading: field IS inserted (#2450)', () => { + // Same bug class, different heading: `## Session Continuity` is the + // bootstrap-template shape. The rewrite regex at the second bug site used + // the same literal-\n pattern. + const STATE_CRLF_CONTINUITY = [ + '---', + 'status: executing', + '---', + '', + '## Session Continuity', + '', + 'Next recommended action: resume phase 2.', + '', + ].join('\r\n'); + + const d = createTempProject(); + try { + fs.writeFileSync(path.join(d, '.planning', 'STATE.md'), STATE_CRLF_CONTINUITY); + const result = runGsdTools(['state', 'record-session', '--resume-file', 'plan-9.md'], d); + assert.ok(result.success, `Command failed: ${result.error}`); + + const after = fs.readFileSync(path.join(d, '.planning', 'STATE.md'), 'utf-8'); + assert.ok( + /\*\*Resume file:\*\*\s*plan-9\.md/.test(after), + `Resume file field must be inserted under Session Continuity; STATE.md head:\n${after.slice(0, 500)}`, + ); + // Existing prose must be preserved (#1101 invariant) + assert.ok(/Next recommended action: resume phase 2\./.test(after), + 'Session Continuity prose must be preserved'); + } finally { + cleanup(d); + } + }); + + test('record-session --resume-file on mixed-ending STATE.md (LF frontmatter + CRLF body): field IS written (#2450)', () => { + // CONTRIBUTING.md §"Parser and project-file inputs" lists "Mixed CRLF/LF + // newlines" as a required adversarial fixture class. Realistic when a + // Windows editor normalizes frontmatter bytes but preserves body text, + // or when tooling concatenates LF + CRLF fragments. The bug class is + // body-section-rewrite, so CRLF body + LF frontmatter is the worst case: + // the frontmatter delimiter is LF (syncStateFrontmatter parses either) + // but the `## Session` heading is CRLF, exercising the writer regex. + const STATE_MIXED_LF_FM_CRLF_BODY = [ + '---', + 'status: executing', + '---', + '', // LF after frontmatter + ].join('\n') + [ + '## Session', + '', + '**Last session:** 2026-01-01T00:00:00.000Z', + '**Stopped at:** None', + // Resume file absent + '', + ].join('\r\n'); + + const d = createTempProject(); + try { + fs.writeFileSync(path.join(d, '.planning', 'STATE.md'), STATE_MIXED_LF_FM_CRLF_BODY); + const result = runGsdTools(['state', 'record-session', '--resume-file', 'plan-mix.md'], d); + assert.ok(result.success, `Command failed: ${result.error}`); + + const after = fs.readFileSync(path.join(d, '.planning', 'STATE.md'), 'utf-8'); + assert.ok( + /\*\*Resume file:\*\*\s*plan-mix\.md/.test(after), + `Resume file field must be on disk despite mixed line endings; STATE.md head:\n${after.slice(0, 500)}`, + ); + } finally { + cleanup(d); + } + }); }); describe('T6 section-splice characterization — add-decision', () => {