From e3c77fe7e94ac1919085081d0834a01301c5893f Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 21 Sep 2026 20:47:00 -0700 Subject: [PATCH] fix: migrate-root merges memory files by record id instead of skipping them (GIT-100) learnings.json, threads.json, decisions.json and sessions.json were skipped as "already exists", so a destination holding any local memory received none of the project store's, and the report listed the user's scars under "skipped". - Those four are merged by record id: new ids are added; where both sides hold a record the newer wins (updated_at, else resolved_at, last_touched_at, created_at), the destination on a tie or when neither is dated. - Every loser is written to /migrate-root-conflicts.json with which side was kept, deduplicated so a re-run logs nothing new. - The destination's memory files are backed up to /backups/ before the first write; nothing is backed up when nothing changes. - Idempotent: a re-run changes nothing. - A memory file that is not a readable list of records is reported as NOT merged (exit 1) and left untouched; never listed as skipped. - Other files keep copy-if-absent; backups/ and the conflicts log are never migrated out of a source. migrate-root.test.ts: "never overwrites a file that already exists" and "reports why each file was skipped" used learnings.json and asserted it was skipped, i.e. the old behaviour. They now use config.json to keep testing the non-memory rule. +11 tests. Co-Authored-By: Claude Opus 5 --- src/commands/migrate-root.ts | 266 +++++++++++++++++++---- tests/unit/commands/migrate-root.test.ts | 168 +++++++++++++- 2 files changed, 387 insertions(+), 47 deletions(-) diff --git a/src/commands/migrate-root.ts b/src/commands/migrate-root.ts index faa4241..48bc4c2 100644 --- a/src/commands/migrate-root.ts +++ b/src/commands/migrate-root.ts @@ -18,10 +18,21 @@ * is wrong about anything, the user still has their data where it was. Moving * would make a bad merge unrecoverable. * - * NEVER OVERWRITE. A file that already exists at the destination wins. The - * destination is the live store; the source is, by definition, the one that - * has not been read recently. Clobbering current memory with stale memory is - * worse than skipping. + * NEVER OVERWRITE a non-memory file. One that already exists at the + * destination wins; the destination is the live store. + * + * MERGE THE MEMORY FILES (GIT-100). learnings.json, threads.json, + * decisions.json and sessions.json used to be skipped as "already exists" — + * so a store with ANY local memory at the destination migrated none of the + * project store's memory, and the report listed the user's scars under + * "skipped". They are now merged by record id: new ids are added; where both + * sides hold a record, the newer (updated_at, else resolved_at / + * last_touched_at / created_at) wins, the destination on a tie or when + * neither is dated; every loser is written to migrate-root-conflicts.json. + * The destination's memory files are backed up before the first write. + * Re-running is idempotent: nothing changes, and a conflict is logged once. + * A memory file that cannot be read as a list of records is reported as NOT + * merged — never as skipped. * * REPORT EVERY SKIP. A silent partial migration would leave the user believing * they had merged when they had not — the failure class GIT-93 was about. @@ -31,11 +42,117 @@ import * as fs from "fs"; import * as path from "path"; import { findStrandedProjectRoots, getHomeGitmemDir } from "../services/gitmem-dir.js"; +/** Memory files merged by record id instead of skipped (GIT-100). */ +export const MERGED_FILES = ["learnings.json", "threads.json", "decisions.json", "sessions.json"] as const; +const CONFLICTS_FILE = "migrate-root-conflicts.json"; + +export interface MergeReport { + file: string; + added: number; + updated: number; + unchanged: number; + /** Both sides held the record and the destination's was kept. */ + kept_destination: number; + /** Conflicts written to the conflicts file by this run (already-logged ones are not repeated). */ + conflicts_logged: number; +} + interface MigrationPlan { source: string; destination: string; copied: string[]; skipped: Array<{ file: string; reason: string }>; + merged: MergeReport[]; + /** Memory files that could not be merged. Never silently skipped. */ + failed: Array<{ file: string; reason: string }>; + /** Where the destination's memory files were backed up, when anything was written. */ + backup?: string; + conflicts_file?: string; +} + +type MemoryRecord = Record & { id: string }; + +interface ConflictEntry { + file: string; + id: string; + kept: "destination" | "source"; + kept_timestamp: string | null; + lost_timestamp: string | null; + lost_record: unknown; + logged_at: string; +} + +const RECENCY_FIELDS = ["updated_at", "resolved_at", "last_touched_at", "created_at"] as const; + +function recency(r: MemoryRecord): { at: number | null; raw: string | null } { + for (const f of RECENCY_FIELDS) { + const v = r[f]; + if (typeof v === "string") { + const at = Date.parse(v); + if (Number.isFinite(at)) return { at, raw: v }; + } + } + return { at: null, raw: null }; +} + +/** Key-order-independent JSON, so re-serialized records compare equal. */ +function stable(v: unknown): string { + if (Array.isArray(v)) return `[${v.map(stable).join(",")}]`; + if (v && typeof v === "object") { + return `{${Object.keys(v as object).sort().map((k) => `${JSON.stringify(k)}:${stable((v as Record)[k])}`).join(",")}}`; + } + return JSON.stringify(v); +} + +function readRecords(file: string): { ok: true; records: MemoryRecord[] } | { ok: false; reason: string } { + let data: unknown; + try { + data = JSON.parse(fs.readFileSync(file, "utf-8")); + } catch (e) { + return { ok: false, reason: `unreadable JSON (${e instanceof Error ? e.message : String(e)})` }; + } + if (!Array.isArray(data)) return { ok: false, reason: "not a list of records" }; + const bad = data.findIndex((r) => !r || typeof r !== "object" || typeof (r as { id?: unknown }).id !== "string"); + if (bad !== -1) return { ok: false, reason: `record ${bad} has no string id` }; + return { ok: true, records: data as MemoryRecord[] }; +} + +interface PlannedMerge { + report: MergeReport; + merged: MemoryRecord[]; + conflicts: Omit[]; + changed: boolean; +} + +/** Merge source into destination by id. Pure: returns the result, writes nothing. */ +function planMerge(file: string, dest: MemoryRecord[], src: MemoryRecord[]): PlannedMerge { + const report: MergeReport = { file, added: 0, updated: 0, unchanged: 0, kept_destination: 0, conflicts_logged: 0 }; + const conflicts: Omit[] = []; + const merged = dest.slice(); + const index = new Map(merged.map((r, i) => [r.id, i])); + + for (const s of src) { + const i = index.get(s.id); + if (i === undefined) { + index.set(s.id, merged.length); + merged.push(s); + report.added++; + continue; + } + const d = merged[i]; + if (stable(d) === stable(s)) { report.unchanged++; continue; } + const rd = recency(d), rs = recency(s); + const sourceNewer = rs.at !== null && (rd.at === null || rs.at > rd.at); + if (sourceNewer) { + merged[i] = s; + report.updated++; + conflicts.push({ file, id: s.id, kept: "source", kept_timestamp: rs.raw, lost_timestamp: rd.raw, lost_record: d }); + } else { + report.kept_destination++; + conflicts.push({ file, id: s.id, kept: "destination", kept_timestamp: rd.raw, lost_timestamp: rs.raw, lost_record: s }); + } + } + return { report, merged, conflicts, changed: report.added > 0 || report.updated > 0 }; } /** @@ -47,19 +164,26 @@ export function findProjectRoots(): string[] { return findStrandedProjectRoots(); } +const isPerInstall = (relative: string, name: string) => + relative === "" && (name === "cache" || name === "license-cache.json" || name === "backups" || name === CONFLICTS_FILE); + +const isMergedFile = (relative: string, name: string) => + relative === "" && (MERGED_FILES as readonly string[]).includes(name); + /** - * Recursively copy `from` into `to`, never overwriting an existing file. - * - * Returns what was copied and what was left alone, so the caller can report both - * rather than claiming a clean merge. + * Walk `from`, planning (and unless dryRun, performing) the copy of every + * non-memory file that does not exist at `to`. Memory files present on both + * sides are collected for merging instead. */ -function copyTree( +function walk( from: string, to: string, plan: MigrationPlan, + dryRun: boolean, + toMerge: string[], relative = "" ): void { - fs.mkdirSync(to, { recursive: true }); + if (!dryRun) fs.mkdirSync(to, { recursive: true }); for (const entry of fs.readdirSync(from, { withFileTypes: true })) { const rel = relative ? path.join(relative, entry.name) : entry.name; const src = path.join(from, entry.name); @@ -67,50 +191,101 @@ function copyTree( // Caches and license state are per-install, not memory. Copying them would // move a license binding between roots, which is not this command's job. - if (relative === "" && (entry.name === "cache" || entry.name === "license-cache.json")) { + // Backups and the conflicts log belong to the root they were written in. + if (isPerInstall(relative, entry.name)) { plan.skipped.push({ file: rel, reason: "per-install state, not memory" }); continue; } if (entry.isDirectory()) { - copyTree(src, dst, plan, rel); + walk(src, dst, plan, dryRun, toMerge, rel); continue; } if (fs.existsSync(dst)) { - plan.skipped.push({ file: rel, reason: "already exists in destination" }); + if (isMergedFile(relative, entry.name)) { + toMerge.push(entry.name); + } else { + plan.skipped.push({ file: rel, reason: "already exists in destination" }); + } continue; } - fs.copyFileSync(src, dst); + if (!dryRun) fs.copyFileSync(src, dst); plan.copied.push(rel); } } +function loadConflicts(file: string): ConflictEntry[] { + try { + const data = JSON.parse(fs.readFileSync(file, "utf-8")); + return Array.isArray(data) ? data : []; + } catch { + return []; + } +} + +const conflictKey = (c: Omit) => `${c.file}\n${c.id}\n${stable(c.lost_record)}`; + export function migrateRoot(source: string, destination: string, dryRun: boolean): MigrationPlan { - const plan: MigrationPlan = { source, destination, copied: [], skipped: [] }; - - if (dryRun) { - // Walk the same tree without writing, so --dry-run reports the real plan - // rather than a guess at one. - const probe = (from: string, to: string, rel = ""): void => { - for (const entry of fs.readdirSync(from, { withFileTypes: true })) { - const r = rel ? path.join(rel, entry.name) : entry.name; - if (rel === "" && (entry.name === "cache" || entry.name === "license-cache.json")) { - plan.skipped.push({ file: r, reason: "per-install state, not memory" }); - continue; - } - if (entry.isDirectory()) { probe(path.join(from, entry.name), path.join(to, entry.name), r); continue; } - if (fs.existsSync(path.join(to, entry.name))) { - plan.skipped.push({ file: r, reason: "already exists in destination" }); - continue; - } - plan.copied.push(r); - } - }; - probe(source, destination); - return plan; + const plan: MigrationPlan = { source, destination, copied: [], skipped: [], merged: [], failed: [] }; + const toMerge: string[] = []; + + // Walk the same tree either way, so --dry-run reports the real plan rather + // than a guess at one. + walk(source, destination, plan, dryRun, toMerge); + + // Memory files on both sides: merge by record id (GIT-100). + const planned: Array = []; + for (const name of toMerge) { + const d = readRecords(path.join(destination, name)); + const s = readRecords(path.join(source, name)); + if (!d.ok || !s.ok) { + const reason = !d.ok ? `destination ${d.reason}` : `source ${(s as { reason: string }).reason}`; + plan.failed.push({ file: name, reason: `NOT merged: ${reason}` }); + continue; + } + planned.push({ name, ...planMerge(name, d.records, s.records) }); + } + + const conflictsPath = path.join(destination, CONFLICTS_FILE); + const logged = loadConflicts(conflictsPath); + const seen = new Set(logged.map(conflictKey)); + const newConflicts: ConflictEntry[] = []; + const now = new Date().toISOString(); + for (const p of planned) { + for (const c of p.conflicts) { + const k = conflictKey(c); + if (seen.has(k)) continue; + seen.add(k); + newConflicts.push({ ...c, logged_at: now }); + p.report.conflicts_logged++; + } + plan.merged.push(p.report); } + if (newConflicts.length > 0 || logged.length > 0) plan.conflicts_file = conflictsPath; - copyTree(source, destination, plan); + const writes = planned.filter((p) => p.changed); + if (dryRun || (writes.length === 0 && newConflicts.length === 0)) return plan; + + // Back up the destination's memory files before the first write. + if (writes.length > 0) { + const backup = path.join(destination, "backups", `migrate-root-${now.replace(/[:.]/g, "-")}`); + fs.mkdirSync(backup, { recursive: true }); + for (const name of MERGED_FILES) { + const f = path.join(destination, name); + if (fs.existsSync(f)) fs.copyFileSync(f, path.join(backup, name)); + } + plan.backup = backup; + } + + for (const p of writes) { + const target = path.join(destination, p.name); + const tmpFile = `${target}.migrate-${process.pid}.tmp`; + fs.writeFileSync(tmpFile, JSON.stringify(p.merged, null, 2)); + fs.renameSync(tmpFile, target); + } + if (newConflicts.length > 0) { + fs.writeFileSync(conflictsPath, JSON.stringify([...logged, ...newConflicts], null, 2)); + } return plan; } @@ -156,6 +331,23 @@ export function main(args: string[]): void { const plan = migrateRoot(source, home, dryRun); console.log(`${plan.copied.length} file(s) ${dryRun ? "would be " : ""}copied.`); + if (plan.merged.length > 0) { + console.log(`${plan.merged.length} memory file(s) ${dryRun ? "would be " : ""}merged by record id:`); + for (const m of plan.merged) { + console.log( + ` ${m.file} — ${m.added} added, ${m.updated} updated from the source, ${m.unchanged} unchanged, ` + + `${m.kept_destination} kept from the destination` + + (m.conflicts_logged > 0 ? `, ${m.conflicts_logged} conflict(s) ${dryRun ? "would be " : ""}logged` : "") + ); + } + } + if (plan.failed.length > 0) { + console.log(`${plan.failed.length} memory file(s) NOT merged — fix these and re-run:`); + for (const f of plan.failed) console.log(` ${f.file} — ${f.reason}`); + process.exitCode = 1; + } + if (plan.backup) console.log(`Destination memory files backed up to ${plan.backup}`); + if (plan.conflicts_file) console.log(`Conflicting records (the version not kept) are in ${plan.conflicts_file}`); if (plan.skipped.length > 0) { console.log(`${plan.skipped.length} skipped:`); for (const s of plan.skipped) console.log(` ${s.file} — ${s.reason}`); diff --git a/tests/unit/commands/migrate-root.test.ts b/tests/unit/commands/migrate-root.test.ts index e26adf7..01f6ade 100644 --- a/tests/unit/commands/migrate-root.test.ts +++ b/tests/unit/commands/migrate-root.test.ts @@ -10,7 +10,10 @@ * merely fail: * * copies, never moves — a wrong call leaves the original intact - * never overwrites — stale memory cannot clobber current memory + * never overwrites — a non-memory file at the destination wins + * merges memory by id — GIT-100: learnings/threads/decisions/sessions + * .json were skipped as "already exists", so a + * destination with any memory received none * reports every skip — a partial merge is never reported as complete * writes where gitmem reads — this one was a real bug: main() computed the * destination with os.homedir() instead of the @@ -67,24 +70,27 @@ describe("GIT-91: migrate-root", () => { expect(read(path.join(source, "learnings.json"))).toBe(before); }); - it("never overwrites a file that already exists in the destination", () => { - // The destination is the live store; the source is by definition the one - // that has not been read recently. Stale must not win. - fs.writeFileSync(path.join(destination, "learnings.json"), '{"learnings":["CURRENT"]}'); + it("never overwrites a non-memory file that already exists in the destination", () => { + // GIT-100: this used learnings.json and asserted it was skipped, i.e. the + // project store's memory was dropped whenever the destination had any. + // Memory files are merged now (below); other files keep this rule. + fs.writeFileSync(path.join(source, "config.json"), '{"project":"old"}'); + fs.writeFileSync(path.join(destination, "config.json"), '{"project":"CURRENT"}'); const plan = migrateRoot(source, destination, false); - expect(read(path.join(destination, "learnings.json"))).toBe('{"learnings":["CURRENT"]}'); - expect(plan.copied).not.toContain("learnings.json"); - expect(plan.skipped.map((s) => s.file)).toContain("learnings.json"); + expect(read(path.join(destination, "config.json"))).toBe('{"project":"CURRENT"}'); + expect(plan.copied).not.toContain("config.json"); + expect(plan.skipped.map((s) => s.file)).toContain("config.json"); }); it("reports why each file was skipped", () => { - fs.writeFileSync(path.join(destination, "learnings.json"), "{}"); + fs.writeFileSync(path.join(source, "config.json"), "{}"); + fs.writeFileSync(path.join(destination, "config.json"), "{}"); const plan = migrateRoot(source, destination, false); - const skip = plan.skipped.find((s) => s.file === "learnings.json"); + const skip = plan.skipped.find((s) => s.file === "config.json"); // A silent partial merge would read as a complete one. expect(skip?.reason).toMatch(/already exists/i); }); @@ -111,3 +117,145 @@ describe("GIT-91: migrate-root", () => { expect(dry.copied.sort()).toEqual(applied.copied.sort()); }); }); + +// =========================================================================== +// GIT-100: memory files are merged by record id, not skipped +// =========================================================================== + +const ts = (h: number) => `2026-09-${String(h).padStart(2, "0")}T00:00:00.000Z`; +const writeJson = (p: string, v: unknown) => fs.writeFileSync(p, JSON.stringify(v, null, 2)); +const readJson = (p: string) => JSON.parse(read(p)); +const ids = (p: string) => readJson(p).map((r: { id: string }) => r.id); + +describe("GIT-100: migrate-root merges memory files by record id", () => { + beforeEach(() => { + for (const f of ["learnings.json", "threads.json", "decisions.json", "sessions.json"]) { + fs.rmSync(path.join(source, f), { force: true }); + } + writeJson(path.join(source, "learnings.json"), [ + { id: "L-src-only", title: "only in project store", updated_at: ts(1) }, + { id: "L-both-src-newer", title: "project edit", updated_at: ts(10) }, + { id: "L-both-dst-newer", title: "stale project copy", updated_at: ts(2) }, + { id: "L-same", title: "identical", updated_at: ts(3) }, + ]); + writeJson(path.join(destination, "learnings.json"), [ + { id: "L-dst-only", title: "only in home store", updated_at: ts(4) }, + { id: "L-both-src-newer", title: "old home copy", updated_at: ts(5) }, + { id: "L-both-dst-newer", title: "home edit", updated_at: ts(9) }, + { updated_at: ts(3), title: "identical", id: "L-same" }, // same record, different key order + ]); + writeJson(path.join(source, "threads.json"), [{ id: "t-src", text: "project thread", created_at: ts(1) }]); + writeJson(path.join(destination, "threads.json"), [{ id: "t-dst", text: "home thread", created_at: ts(2) }]); + writeJson(path.join(source, "decisions.json"), [{ id: "D1", title: "undated A" }]); + writeJson(path.join(destination, "decisions.json"), [{ id: "D1", title: "undated B" }]); + writeJson(path.join(source, "sessions.json"), [{ id: "S-src", session_date: "2026-09-01" }]); + }); + + it("never lists a memory file under skipped", () => { + const plan = migrateRoot(source, destination, false); + const skipped = plan.skipped.map((s) => s.file); + for (const f of ["learnings.json", "threads.json", "decisions.json", "sessions.json"]) expect(skipped).not.toContain(f); + }); + + it("adds new ids, keeps destination-only ids, and the newer record wins", () => { + const plan = migrateRoot(source, destination, false); + const merged = readJson(path.join(destination, "learnings.json")); + const byId = Object.fromEntries(merged.map((r: { id: string; title: string }) => [r.id, r.title])); + + expect(byId).toEqual({ + "L-dst-only": "only in home store", + "L-both-src-newer": "project edit", // source newer: replaced + "L-both-dst-newer": "home edit", // destination newer: kept + "L-same": "identical", + "L-src-only": "only in project store", + }); + expect(plan.merged.find((m) => m.file === "learnings.json")).toMatchObject({ + added: 1, updated: 1, unchanged: 1, kept_destination: 1, conflicts_logged: 2, + }); + expect(ids(path.join(destination, "threads.json")).sort()).toEqual(["t-dst", "t-src"]); + }); + + it("a memory file only in the source is copied", () => { + const plan = migrateRoot(source, destination, false); + expect(plan.copied).toContain("sessions.json"); + expect(ids(path.join(destination, "sessions.json"))).toEqual(["S-src"]); + }); + + it("undated records that differ: the destination is kept and the loser logged", () => { + migrateRoot(source, destination, false); + expect(readJson(path.join(destination, "decisions.json"))).toEqual([{ id: "D1", title: "undated B" }]); + const conflicts = readJson(path.join(destination, "migrate-root-conflicts.json")); + expect(conflicts.find((c: { id: string }) => c.id === "D1")).toMatchObject({ kept: "destination", lost_record: { id: "D1", title: "undated A" } }); + }); + + it("logs every loser, with which side was kept", () => { + migrateRoot(source, destination, false); + const conflicts = readJson(path.join(destination, "migrate-root-conflicts.json")); + const learningConflicts = conflicts.filter((c: { file: string }) => c.file === "learnings.json"); + expect(learningConflicts).toHaveLength(2); + expect(learningConflicts.find((c: { id: string }) => c.id === "L-both-src-newer")).toMatchObject({ + kept: "source", lost_record: { title: "old home copy" }, kept_timestamp: ts(10), lost_timestamp: ts(5), + }); + expect(learningConflicts.find((c: { id: string }) => c.id === "L-both-dst-newer")).toMatchObject({ + kept: "destination", lost_record: { title: "stale project copy" }, + }); + }); + + it("backs up the destination's memory files before writing", () => { + const before = read(path.join(destination, "learnings.json")); + const plan = migrateRoot(source, destination, false); + expect(plan.backup).toBeDefined(); + expect(read(path.join(plan.backup!, "learnings.json"))).toBe(before); + expect(fs.existsSync(path.join(plan.backup!, "threads.json"))).toBe(true); + }); + + it("is idempotent: a re-run changes nothing, backs up nothing, logs nothing new", () => { + migrateRoot(source, destination, false); + const snapshot = ["learnings.json", "threads.json", "decisions.json", "sessions.json", "migrate-root-conflicts.json"] + .map((f) => read(path.join(destination, f))); + + const again = migrateRoot(source, destination, false); + + expect(["learnings.json", "threads.json", "decisions.json", "sessions.json", "migrate-root-conflicts.json"] + .map((f) => read(path.join(destination, f)))).toEqual(snapshot); + expect(again.backup).toBeUndefined(); + for (const m of again.merged) { + expect(m.added).toBe(0); + expect(m.updated).toBe(0); + expect(m.conflicts_logged).toBe(0); + } + }); + + it("leaves the source intact", () => { + const before = read(path.join(source, "learnings.json")); + migrateRoot(source, destination, false); + expect(read(path.join(source, "learnings.json"))).toBe(before); + }); + + it("dry-run writes nothing but reports the same merge", () => { + const before = read(path.join(destination, "learnings.json")); + const dry = migrateRoot(source, destination, true); + expect(read(path.join(destination, "learnings.json"))).toBe(before); + expect(fs.existsSync(path.join(destination, "migrate-root-conflicts.json"))).toBe(false); + expect(fs.existsSync(path.join(destination, "backups"))).toBe(false); + + const applied = migrateRoot(source, destination, false); + expect(dry.merged).toEqual(applied.merged); + }); + + it("an unreadable memory file is reported as NOT merged, never skipped, and left untouched", () => { + fs.writeFileSync(path.join(destination, "threads.json"), "{ not json"); + const plan = migrateRoot(source, destination, false); + expect(plan.failed).toEqual([{ file: "threads.json", reason: expect.stringMatching(/^NOT merged: destination unreadable JSON/) }]); + expect(plan.skipped.map((s) => s.file)).not.toContain("threads.json"); + expect(read(path.join(destination, "threads.json"))).toBe("{ not json"); + }); + + it("does not migrate backups or the conflicts log out of a source that has them", () => { + fs.mkdirSync(path.join(source, "backups", "old"), { recursive: true }); + fs.writeFileSync(path.join(source, "migrate-root-conflicts.json"), "[]"); + const plan = migrateRoot(source, destination, false); + expect(plan.copied.some((f) => f.startsWith("backups"))).toBe(false); + expect(plan.copied).not.toContain("migrate-root-conflicts.json"); + }); +});