From ae02d2a560e3dd4187ae59b9a66b6e444cb81ff9 Mon Sep 17 00:00:00 2001 From: AmirF194 Date: Wed, 1 Jul 2026 23:50:28 -0600 Subject: [PATCH] fix(openclaw): stray SOUL.md marker no longer chains into data loss MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A truncated/stray marker (interrupted write, partial user edit) chained into deleting user content: appendBootstrapToSoul saw 'no complete block' and appended a second one; stripBootstrapFromSoul then cut from the FIRST begin to the FIRST end — spanning everything between the stray marker and the appended block. Reported reproduction ended with the whole SOUL.md deleted. Replace the single-span cut with a scan that pairs each begin with the nearest end before the next begin; unpaired markers are removed as just the marker text, never as a span. Append now detects damaged markers (orphans, duplicates), strips them safely, and writes one clean block. Fixes #596 --- bin/lib/openclaw.js | 71 ++++++++++++++++---- tests/installer/e2e.freshinstall.test.mjs | 81 +++++++++++++++++++++++ 2 files changed, 138 insertions(+), 14 deletions(-) diff --git a/bin/lib/openclaw.js b/bin/lib/openclaw.js index 3ee844d..72f6cab 100644 --- a/bin/lib/openclaw.js +++ b/bin/lib/openclaw.js @@ -116,33 +116,76 @@ function loadSkillBody(repoRoot) { } // ── SOUL.md marker-block append/strip ───────────────────────────────────── +// +// Damage tolerance (#596): a stray or truncated marker (interrupted write, +// partial user edit) used to chain into data loss — append saw "no complete +// block" and added a SECOND block; strip then cut from the FIRST begin to the +// FIRST end, which spanned all user content between the stray marker and the +// appended block. The scan below pairs each begin with the nearest end BEFORE +// the next begin; an unpaired marker is removed as just the marker itself, +// never as a span over user content. + +function stripAllBootstrapBlocks(text) { + let result = ''; + let found = false; + let i = 0; + while (i < text.length) { + const b = text.indexOf(MARK_BEGIN, i); + if (b === -1) { result += text.slice(i); break; } + result += text.slice(i, b); + found = true; + const nextB = text.indexOf(MARK_BEGIN, b + MARK_BEGIN.length); + const e = text.indexOf(MARK_END, b + MARK_BEGIN.length); + if (e !== -1 && (nextB === -1 || e < nextB)) { + i = e + MARK_END.length; // well-formed block — drop begin..end inclusive + } else { + i = b + MARK_BEGIN.length; // orphan begin — drop only the marker itself + } + // Collapse the blank-line scar around the cut (same cosmetic rule the + // old single-cut code applied): keep at most one newline on each side. + result = result.replace(/\n+$/, '\n'); + const lead = /^\n+/.exec(text.slice(i)); + if (lead) i += lead[0].length - (result ? 1 : 0); + } + // Orphan end markers (begin already gone or never written) — drop marker only. + while (result.includes(MARK_END)) { found = true; result = result.replace(MARK_END, ''); } + return { next: result, found }; +} + function appendBootstrapToSoul(soulPath, snippet) { const existing = readIfExists(soulPath); - if (existing && existing.includes(MARK_BEGIN) && existing.includes(MARK_END)) { - return { changed: false, reason: 'already present' }; + const count = (s, sub) => s.split(sub).length - 1; + let base = existing; + let repaired = false; + if (existing) { + const nb = count(existing, MARK_BEGIN); + const ne = count(existing, MARK_END); + if (nb === 1 && ne === 1 && existing.indexOf(MARK_END) > existing.indexOf(MARK_BEGIN)) { + return { changed: false, reason: 'already present' }; + } + if (nb > 0 || ne > 0) { + // Damaged markers — strip them safely first, then append one clean block. + base = stripAllBootstrapBlocks(existing).next; + repaired = true; + } } let next; - if (existing && existing.length) { - const sep = existing.endsWith('\n\n') ? '' : (existing.endsWith('\n') ? '\n' : '\n\n'); - next = existing + sep + snippet; + if (base && base.length) { + const sep = base.endsWith('\n\n') ? '' : (base.endsWith('\n') ? '\n' : '\n\n'); + next = base + sep + snippet; } else { next = snippet; } fs.writeFileSync(soulPath, next, { mode: 0o644 }); - return { changed: true }; + return repaired ? { changed: true, repaired: true } : { changed: true }; } function stripBootstrapFromSoul(soulPath) { const existing = readIfExists(soulPath); if (!existing) return { changed: false, reason: 'no SOUL.md' }; - const begin = existing.indexOf(MARK_BEGIN); - const end = existing.indexOf(MARK_END); - if (begin === -1 || end === -1 || end <= begin) return { changed: false, reason: 'no marker block' }; - const before = existing.slice(0, begin); - const after = existing.slice(end + MARK_END.length); - // Collapse adjacent blank lines around the cut so we don't leave a triple - // newline scar from `\n\n...\n\n\n`. - let next = (before.replace(/\n+$/, '\n') + after.replace(/^\n+/, '\n')).trimEnd(); + const { next: stripped, found } = stripAllBootstrapBlocks(existing); + if (!found) return { changed: false, reason: 'no marker block' }; + let next = stripped.trimEnd(); next = next ? next + '\n' : ''; if (next === '') { // SOUL.md only contained our block — remove the file so OpenClaw doesn't diff --git a/tests/installer/e2e.freshinstall.test.mjs b/tests/installer/e2e.freshinstall.test.mjs index bf85217..e41029d 100644 --- a/tests/installer/e2e.freshinstall.test.mjs +++ b/tests/installer/e2e.freshinstall.test.mjs @@ -400,3 +400,84 @@ test('lib settings.addCommandHook is idempotent across two synthetic install pas fs.rmSync(dir, { recursive: true, force: true }); } }); + +// ── Tests: SOUL.md marker damage tolerance (#596) ────────────────────────── +// A stray/truncated marker used to chain into data loss: append added a +// second block, then strip cut from the FIRST begin to the FIRST end — +// spanning all user content in between. These drive the helper directly. +test('openclaw: truncated begin marker does not eat user content (issue #596 chain)', () => { + const helper = requireCjs(path.join(REPO_ROOT, 'bin', 'lib', 'openclaw.js')); + const dir = freshTmpDir(); + const soul = path.join(dir, 'SOUL.md'); + try { + // Begin marker with no end (interrupted write), then user content. + fs.writeFileSync(soul, helper.MARK_BEGIN + '\n\nUSER IMPORTANT CONTENT\n'); + const snippet = helper.loadBootstrapSnippet(REPO_ROOT); + + const a = helper.appendBootstrapToSoul(soul, snippet); + assert.equal(a.changed, true); + const afterAppend = fs.readFileSync(soul, 'utf8'); + assert.match(afterAppend, /USER IMPORTANT CONTENT/, 'user content lost during repair-append'); + assert.equal(afterAppend.split(helper.MARK_BEGIN).length - 1, 1, 'repair must leave exactly one begin marker'); + + const s = helper.stripBootstrapFromSoul(soul); + assert.equal(s.changed, true); + assert.equal(s.removed, undefined, 'file with user content must not be deleted'); + const afterStrip = fs.readFileSync(soul, 'utf8'); + assert.match(afterStrip, /USER IMPORTANT CONTENT/, 'user content deleted by strip — the #596 data loss'); + assert.doesNotMatch(afterStrip, /caveman-begin/, 'marker survived strip'); + } finally { + fs.rmSync(dir, { recursive: true, force: true }); + } +}); + +test('openclaw: strip removes multiple blocks pairwise, keeping user content between them', () => { + const helper = requireCjs(path.join(REPO_ROOT, 'bin', 'lib', 'openclaw.js')); + const dir = freshTmpDir(); + const soul = path.join(dir, 'SOUL.md'); + try { + const block = helper.MARK_BEGIN + '\nrules v1\n' + helper.MARK_END; + fs.writeFileSync(soul, block + '\n\nUSER KEEP ME\n\n' + block + '\n'); + const s = helper.stripBootstrapFromSoul(soul); + assert.equal(s.changed, true); + const after = fs.readFileSync(soul, 'utf8'); + assert.match(after, /USER KEEP ME/, 'user content between blocks deleted'); + assert.doesNotMatch(after, /caveman-(begin|end)/, 'markers survived'); + assert.doesNotMatch(after, /rules v1/, 'block bodies survived'); + } finally { + fs.rmSync(dir, { recursive: true, force: true }); + } +}); + +test('openclaw: orphan end marker stripped without touching content', () => { + const helper = requireCjs(path.join(REPO_ROOT, 'bin', 'lib', 'openclaw.js')); + const dir = freshTmpDir(); + const soul = path.join(dir, 'SOUL.md'); + try { + fs.writeFileSync(soul, 'before\n' + helper.MARK_END + '\nafter\n'); + const s = helper.stripBootstrapFromSoul(soul); + assert.equal(s.changed, true); + const after = fs.readFileSync(soul, 'utf8'); + assert.match(after, /before/); + assert.match(after, /after/); + assert.doesNotMatch(after, /caveman-end/); + } finally { + fs.rmSync(dir, { recursive: true, force: true }); + } +}); + +test('openclaw: append on a well-formed block stays a no-op', () => { + const helper = requireCjs(path.join(REPO_ROOT, 'bin', 'lib', 'openclaw.js')); + const dir = freshTmpDir(); + const soul = path.join(dir, 'SOUL.md'); + try { + const snippet = helper.loadBootstrapSnippet(REPO_ROOT); + helper.appendBootstrapToSoul(soul, snippet); + const first = fs.readFileSync(soul, 'utf8'); + const again = helper.appendBootstrapToSoul(soul, snippet); + assert.equal(again.changed, false); + assert.equal(fs.readFileSync(soul, 'utf8'), first, 'no-op append must not modify the file'); + } finally { + fs.rmSync(dir, { recursive: true, force: true }); + } +});