diff --git a/src/config/instruction-blocks.ts b/src/config/instruction-blocks.ts index e2f4b16..9714326 100644 --- a/src/config/instruction-blocks.ts +++ b/src/config/instruction-blocks.ts @@ -189,11 +189,11 @@ export async function removeManagedBlocksFromFile( return false; } - if (!repaired.trim() || isOnlyFrontmatter(repaired)) { - // The file held nothing but the xtctx block — or the YAML frontmatter - // xtctx itself wrote above it, which Cursor would keep loading as an - // xtctx rule. Either way setup created it and disconnect owns removing - // it, rather than leaving a stub behind. + if (!repaired.trim() || isXtctxPrelude(repaired, filePath, projectRoot)) { + // The file held nothing but the xtctx block — or that block under the + // exact frontmatter xtctx itself wrote above it, which Cursor would keep + // loading as an xtctx rule. Either way setup created it and disconnect + // owns removing it, rather than leaving a stub behind. await rm(filePath, { force: true }); return true; } @@ -207,14 +207,26 @@ export async function removeManagedBlocksFromFile( return true; } -/** True when nothing survives but a single YAML frontmatter block. */ -function isOnlyFrontmatter(content: string): boolean { - const trimmed = content.trim(); - if (!trimmed.startsWith("---")) { +/** + * True when what survives removal is exactly the prelude xtctx wrote here. + * + * This asked a looser question — "is the remainder *any* YAML frontmatter?" — + * and deleted the file when it was. That cannot tell xtctx's own Cursor-rule + * prelude from frontmatter the user wrote, so a `CLAUDE.md` holding nothing + * but the author's own `---\ntitle: ...\n---` was destroyed by `disconnect`. + * Reproduced before the fix: the file did not exist afterwards. + * + * The writer only ever prepends `target.prelude`, and only when the file did + * not already open with frontmatter — so the exact string it added is the only + * thing removal is entitled to take back. Anything else, however much it looks + * like boilerplate, belongs to whoever wrote it. + */ +function isXtctxPrelude(content: string, filePath: string, projectRoot: string): boolean { + const prelude = memoryTargets(projectRoot).find((target) => target.path === filePath)?.prelude; + if (!prelude) { return false; } - const end = trimmed.indexOf("\n---", 3); - return end !== -1 && trimmed.slice(end + 4).trim().length === 0; + return normalizeNewlines(content).trim() === normalizeNewlines(prelude).trim(); } export async function inspectManagedFile(filePath: string): Promise<{ diff --git a/src/config/managed-block.ts b/src/config/managed-block.ts index 87816aa..4b1daa0 100644 --- a/src/config/managed-block.ts +++ b/src/config/managed-block.ts @@ -38,15 +38,58 @@ export function stripMarkers(value: string): string { return value.split(MARKERS.begin).join("").split(MARKERS.end).join(""); } -function escapeRegExp(value: string): string { - return value.replace(/[.*+?^${}()|[\]\\]/g, "\\$&"); -} -function managedBlockPattern(trailingNewline: boolean): RegExp { - return new RegExp( - `${escapeRegExp(MARKERS.begin)}[\\s\\S]*?${escapeRegExp(MARKERS.end)}${trailingNewline ? "\\n?" : ""}`, - "g", - ); +/** + * The file, split around every *well-formed* managed block. + * + * Same shape as `String.split` on the block pattern, and different from it in + * the one case that destroyed a user's work: `begin[\s\S]*?end` pairs an + * opening marker with the nearest following close, whatever lies between. A + * file carrying one orphaned `begin` — the end marker deleted in a hand-edit, + * or a merge that kept half of one side — therefore matched from that orphan + * all the way to the *next block's* end, and removal swallowed every line in + * between. + * + * Measured on a real reproduction: a file holding a user's heading and body + * between an orphaned marker and a later valid block came back as one line. + * + * So a `begin` that meets another `begin` before it meets an `end` is not an + * opening at all — it is ordinary text that happens to look like a marker, and + * the safe thing is to leave it alone. An unterminated trailing `begin` is the + * same case. Both leave inert text in the file, which `inspectManagedFile` + * already surfaces, and inert text is recoverable where deleted content is + * not. + */ +function splitOnManagedBlocks(content: string): string[] { + const parts: string[] = []; + let sliceFrom = 0; + let searchFrom = 0; + + for (;;) { + const begin = content.indexOf(MARKERS.begin, searchFrom); + if (begin === -1) { + break; + } + const end = content.indexOf(MARKERS.end, begin + MARKERS.begin.length); + if (end === -1) { + // Opened and never closed: not a block, and nothing after it is ours. + break; + } + const nextBegin = content.indexOf(MARKERS.begin, begin + MARKERS.begin.length); + if (nextBegin !== -1 && nextBegin < end) { + // Another block opens before this one closes, so this marker never had + // a partner. Skip it and try again from the one that might. + searchFrom = nextBegin; + continue; + } + + parts.push(content.slice(sliceFrom, begin)); + sliceFrom = end + MARKERS.end.length; + searchFrom = sliceFrom; + } + + parts.push(content.slice(sliceFrom)); + return parts; } /** @@ -59,7 +102,7 @@ export function removeManagedBlocks(content: string): string { // a newline after the block takes one from the user's text when a block sits // between paragraphs. The separator handled below is the only whitespace // removal that belongs to xtctx. - const parts = normalized.split(managedBlockPattern(false)); + const parts = splitOnManagedBlocks(normalized); if (parts.length === 1) { return normalized; } @@ -106,5 +149,8 @@ export function matchLineEndings(content: string, original: string | null): stri } export function countManagedBlocks(content: string): number { - return normalizeNewlines(content).match(managedBlockPattern(false))?.length ?? 0; + // Counted by the same pairing rule removal uses. A regex count would report + // blocks that removal then refuses to touch — status would say "needs + // repair" about a file `setup --repair` cannot change, forever. + return splitOnManagedBlocks(normalizeNewlines(content)).length - 1; } diff --git a/tests/config/managed-block.test.ts b/tests/config/managed-block.test.ts index a238dca..a262688 100644 --- a/tests/config/managed-block.test.ts +++ b/tests/config/managed-block.test.ts @@ -87,6 +87,78 @@ describe("removeManagedBlocks", () => { * The reachable route is the project path, which is rendered verbatim and can * legally contain the marker text on POSIX. */ +/** + * An unpaired marker must never take the user's content with it. + * + * The removal pattern was `begin[\s\S]*?end`, which pairs an opening marker + * with the nearest following close whatever lies between them. A file holding + * one orphaned `begin` — an end marker lost to a hand-edit, or a merge that + * kept half of one side — therefore matched from that orphan to the *next + * block's* end, and removal deleted every line in between. + * + * Reproduced end to end before the fix, through a real setup/disconnect: a + * file containing the user's own heading and body came back as a single line. + */ +describe("removeManagedBlocks with unpaired markers", () => { + it("keeps content after an orphaned begin marker", () => { + const content = [ + "TOP LINE THE USER WROTE", + begin, + "stale text whose end marker was deleted", + "", + "## The user's own heading", + "Content the user cares about.", + "", + ].join("\n"); + + // Nothing here is a well-formed block, so nothing is xtctx's to remove. + expect(removeManagedBlocks(content)).toBe(content); + }); + + it("removes only the real block when an orphaned begin precedes it", () => { + const content = [ + "USER TOP", + begin, + "orphaned, never closed", + "## The user's own heading", + begin, + "the genuine managed block", + end, + "USER BOTTOM", + ].join("\n"); + + const result = removeManagedBlocks(content); + + expect(result).toContain("USER TOP"); + expect(result).toContain("## The user's own heading"); + expect(result).toContain("USER BOTTOM"); + expect(result).toContain("orphaned, never closed"); + expect(result).not.toContain("the genuine managed block"); + }); + + it("still removes two properly paired blocks", () => { + // The case a stricter rule could easily break: a merge that kept both + // sides leaves two complete blocks, and both are ours. + const content = ["A", begin, "one", end, "MIDDLE", begin, "two", end, "B"].join("\n"); + + const result = removeManagedBlocks(content); + + expect(result).not.toContain("one"); + expect(result).not.toContain("two"); + expect(result).toContain("MIDDLE"); + }); + + it("counts blocks by the same rule removal uses", () => { + // Otherwise status reports a block that removal refuses to touch, and + // tells the user to run a repair that cannot change anything. + const orphaned = ["USER", begin, "not a block", "## heading"].join("\n"); + expect(countManagedBlocks(orphaned)).toBe(0); + + const paired = ["A", begin, "one", end, "B"].join("\n"); + expect(countManagedBlocks(paired)).toBe(1); + }); +}); + describe("stripMarkers", () => { it("removes an end marker embedded in an interpolated value", () => { expect(stripMarkers(`/tmp/${end}/x`)).toBe("/tmp//x"); diff --git a/tests/config/memory-file-preservation.test.ts b/tests/config/memory-file-preservation.test.ts new file mode 100644 index 0000000..3c18a1a --- /dev/null +++ b/tests/config/memory-file-preservation.test.ts @@ -0,0 +1,110 @@ +/** + * Disconnect must not destroy a file the user wrote. + * + * Two ways it did, both found by driving a real setup/disconnect round trip + * rather than by reading the code: + * + * 1. A `CLAUDE.md` containing nothing but the author's own YAML frontmatter + * was DELETED. Removal asked "is the remainder any frontmatter?" and + * deleted the file when it was — which cannot tell xtctx's own Cursor-rule + * prelude from a metadata stub someone wrote themselves. + * + * 2. A file with one orphaned `begin` marker lost every line between that + * marker and the next block's `end`, because the removal pattern paired an + * opening marker with the nearest close whatever lay between. + * + * Both are asserted here through `setupProject` + `disconnectProject`, not + * through the splicing helpers, because that is the layer where the loss + * happened: the unit helpers were individually defensible and the round trip + * still ate the file. + * + * `homeDir` is a temp directory throughout. `setupProject` always wires + * Antigravity's machine-global MCP config, so a test that let it reach the + * real home would edit a file shared by every project on the machine. + */ +import { mkdtemp, readFile, rm, writeFile, stat } from "node:fs/promises"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { afterEach, beforeEach, describe, expect, it } from "vitest"; +import { setupProject } from "@xtctx/config/setup"; +import { disconnectProject } from "@xtctx/config/disconnect"; + +describe("a user's memory file survives setup and disconnect", () => { + let projectRoot = ""; + let homeDir = ""; + + beforeEach(async () => { + projectRoot = await mkdtemp(join(tmpdir(), "xtctx-memory-")); + homeDir = await mkdtemp(join(tmpdir(), "xtctx-memory-home-")); + }); + + afterEach(async () => { + await rm(projectRoot, { recursive: true, force: true }); + await rm(homeDir, { recursive: true, force: true }); + }); + + async function roundTrip(): Promise { + await setupProject({ projectPath: projectRoot, homeDir, yes: true }); + await disconnectProject({ projectPath: projectRoot, homeDir, all: true }); + } + + async function exists(path: string): Promise { + return stat(path).then( + () => true, + () => false, + ); + } + + it("does not delete a file that is only the author's own frontmatter", async () => { + const path = join(projectRoot, "CLAUDE.md"); + const original = "---\ntitle: My own notes\ncustom_field: true\n---\n"; + await writeFile(path, original, "utf-8"); + + await roundTrip(); + + expect(await exists(path), "CLAUDE.md was deleted").toBe(true); + expect(await readFile(path, "utf-8")).toBe(original); + }); + + it("does not eat content sitting after an orphaned begin marker", async () => { + const path = join(projectRoot, "CLAUDE.md"); + const original = [ + "TOP LINE THE USER WROTE", + "", + "stale text whose end marker was deleted", + "", + "## The user's own heading", + "Content the user cares about.", + "", + ].join("\n"); + await writeFile(path, original, "utf-8"); + + await roundTrip(); + + expect(await readFile(path, "utf-8")).toBe(original); + }); + + it("still removes a file that held nothing but xtctx's own block", async () => { + // The behaviour the deletion exists for, which the fix must not lose: + // setup created this file, so disconnect owns removing it rather than + // leaving a stub behind. + const path = join(projectRoot, "CLAUDE.md"); + expect(await exists(path)).toBe(false); + + await roundTrip(); + + expect(await exists(path), "an xtctx-created file was left behind").toBe(false); + }); + + it("leaves the user's prose exactly as written", async () => { + const path = join(projectRoot, "AGENTS.md"); + // Trailing spaces are a markdown hard break, and blank lines at EOF are + // the author's: both have been destroyed by trimming here before. + const original = "# My notes\n\nA line with a hard break \nand the next one.\n\n\n"; + await writeFile(path, original, "utf-8"); + + await roundTrip(); + + expect(await readFile(path, "utf-8")).toBe(original); + }); +});