From f51ff9f3f85dec7dc0d3bfc88acaf1721472a24c Mon Sep 17 00:00:00 2001 From: Lan Nguyen Si Date: Mon, 3 Aug 2026 19:17:09 +0200 Subject: [PATCH 1/2] fix(memory-sync): honest conflict count on marker carryover + owner-writes-only push Two data-integrity defects in the sync path, both reproduced live 2026-08-03: Defect A (silent conflicts=0 on marker carryover): mergeText's fast-path branches (local===base / remote===base) and the append-only success path returned conflict:false without checking whether the winning payload already carried inline conflict markers from a prior pass. In a single sync (pull then push), a genuine pull-time conflict wrote markers to the local file and a clean base; the immediately-following push then saw remote===base and re-labeled that marker-carrying local content as a clean 'local wins' with conflicts=0, then committed it to the hub and base. Fix: hasConflictMarkers() line-anchored check; every conflict:false return that hands back a winning payload now reports conflict:true when that payload carries markers. Payload unchanged, count honest. The genuine single-pass conflict path (constructs markers, conflict:true) is unchanged. Defect B (peer-file echo / last-writer-wins): no ownership concept existed, so a spoke re-offered a peer's machine-state/frictions file it had merely pulled as its own 'local' change. Fix: optional SyncPathConfig.ownerScoped; the push-side collection offers only .json under an ownerScoped directory, and a companion base-map filter strips foreign ownerScoped keys so push's base-union-local merge never visits a peer file this machine doesn't own. Pull is unchanged (peers are still materialized for read). ownerScoped:true set on machine-state and frictions in all three profiles; memory stays shared. Tests: 19 new (repro A, repro B + owner-tolerance, 15 merge-honesty units); full suite 128 pass / 0 fail; build + typecheck clean; watch-mirror-delete and machine-state-syncpath negative controls unchanged and green. Refs: agent-memory task 06d09cde Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01GbC4gdPEf4T1YD8LZV9GA7 --- .../agent-memory-sync/profiles/linux.json | 14 +- .../agent-memory-sync/profiles/mac-mini.json | 14 +- .../agent-memory-sync/profiles/macbook.json | 16 +- .../agent-memory-sync/src/config/loader.ts | 7 +- .../src/memory-sync/config.ts | 89 ++++++++- .../src/memory-sync/merge.ts | 31 ++- .../agent-memory-sync/src/memory-sync/push.ts | 17 +- .../cross-machine-profiles.test.ts | 36 ++++ .../integration/owner-scoped-push.test.ts | 183 ++++++++++++++++++ .../push-conflict-marker-honesty.test.ts | 135 +++++++++++++ .../tests/unit/merge.test.ts | 131 +++++++++++++ 11 files changed, 660 insertions(+), 13 deletions(-) create mode 100644 packages/agent-memory-sync/tests/integration/owner-scoped-push.test.ts create mode 100644 packages/agent-memory-sync/tests/integration/push-conflict-marker-honesty.test.ts create mode 100644 packages/agent-memory-sync/tests/unit/merge.test.ts diff --git a/packages/agent-memory-sync/profiles/linux.json b/packages/agent-memory-sync/profiles/linux.json index 274f6a8..c8f3d32 100644 --- a/packages/agent-memory-sync/profiles/linux.json +++ b/packages/agent-memory-sync/profiles/linux.json @@ -31,6 +31,16 @@ "frictions producer: friction-log sync_export in", "~/.config/friction-log/config.yml writes ~/.harness/frictions/linux.json.", "", + "Both the machine-state and frictions entries set \"ownerScoped\": true —", + "the owner-writes-only convention above is now enforced, not just", + "documented: push only ever offers this machine's own .json", + "(machine-state/linux.json, frictions/linux.json), never a peer's file", + "this machine merely pulled. See collectLocalSyncFiles' ownerFilter", + "option in src/memory-sync/config.ts and", + ".ai/runs/2026-08-03-sync-conflict-markers-echo/03-decisions.md (D-002 to", + "D-004) for the echo/last-writer-wins race this closes. Pull is", + "unaffected — a peer's file is still materialized locally as before.", + "", "Activation: pass BOTH --config (this file) and the profile name", "positionally:", " agent-memory-sync run linux --config profiles/linux.json" @@ -45,7 +55,7 @@ "reachabilityTimeoutMs": 10000, "syncPaths": [ { "source": ".", "destination": "memory", "kind": "directory" }, - { "source": "/home/lan/.harness/machine-state", "destination": "machine-state", "kind": "directory" }, - { "source": "/home/lan/.harness/frictions", "destination": "frictions", "kind": "directory" } + { "source": "/home/lan/.harness/machine-state", "destination": "machine-state", "kind": "directory", "ownerScoped": true }, + { "source": "/home/lan/.harness/frictions", "destination": "frictions", "kind": "directory", "ownerScoped": true } ] } diff --git a/packages/agent-memory-sync/profiles/mac-mini.json b/packages/agent-memory-sync/profiles/mac-mini.json index 891a90f..4ef6144 100644 --- a/packages/agent-memory-sync/profiles/mac-mini.json +++ b/packages/agent-memory-sync/profiles/mac-mini.json @@ -53,6 +53,16 @@ "machine's toolchain snapshot for cross-machine parity checks — see", "docs/machine-setup.md section e) for the payload convention.", "", + "The machine-state and frictions entries both set \"ownerScoped\": true —", + "each is a one-file-per-machine convention (this machine only ever writes", + "its own .json, e.g. machine-state/mac-mini.json), so push must", + "only ever offer that one file, never a peer's file this machine merely", + "pulled. See collectLocalSyncFiles' ownerFilter option in", + "src/memory-sync/config.ts and", + ".ai/runs/2026-08-03-sync-conflict-markers-echo/03-decisions.md (D-002 to", + "D-004) for the echo/last-writer-wins race this closes. Pull is", + "unaffected — a peer's file is still materialized locally as before.", + "", "The directory walk skips hidden files/dot-directories (.DS_Store, ._*", "AppleDouble shadows, .git, ...) by design — see isHiddenEntryName in", "src/memory-sync/config.ts and src/memory-sync/git-client.ts — so macOS", @@ -117,7 +127,7 @@ "reachabilityTimeoutMs": 5000, "syncPaths": [ { "source": ".", "destination": "memory", "kind": "directory" }, - { "source": "/Users/lannguyensi/.harness/machine-state", "destination": "machine-state", "kind": "directory" }, - { "source": "/Users/lannguyensi/.harness/frictions", "destination": "frictions", "kind": "directory" } + { "source": "/Users/lannguyensi/.harness/machine-state", "destination": "machine-state", "kind": "directory", "ownerScoped": true }, + { "source": "/Users/lannguyensi/.harness/frictions", "destination": "frictions", "kind": "directory", "ownerScoped": true } ] } diff --git a/packages/agent-memory-sync/profiles/macbook.json b/packages/agent-memory-sync/profiles/macbook.json index 36d8959..978bfa3 100644 --- a/packages/agent-memory-sync/profiles/macbook.json +++ b/packages/agent-memory-sync/profiles/macbook.json @@ -45,6 +45,16 @@ "machine's toolchain snapshot for cross-machine parity checks \u2014 see", "docs/machine-setup.md section e) for the payload convention.", "", + "The machine-state and frictions entries both set \"ownerScoped\": true \u2014", + "each is a one-file-per-machine convention (this machine only ever writes", + "its own .json, e.g. machine-state/macbook.json), so push must", + "only ever offer that one file, never a peer's file this machine merely", + "pulled. See collectLocalSyncFiles' ownerFilter option in", + "src/memory-sync/config.ts and", + ".ai/runs/2026-08-03-sync-conflict-markers-echo/03-decisions.md (D-002 to", + "D-004) for the echo/last-writer-wins race this closes. Pull is", + "unaffected \u2014 a peer's file is still materialized locally as before.", + "", "The directory walk skips hidden files/dot-directories (.DS_Store, ._*", "AppleDouble shadows, .git, ...) by design \u2014 see isHiddenEntryName in", "src/memory-sync/config.ts and src/memory-sync/git-client.ts \u2014 so macOS", @@ -109,12 +119,14 @@ { "source": "/Users/lan/.harness/machine-state", "destination": "machine-state", - "kind": "directory" + "kind": "directory", + "ownerScoped": true }, { "source": "/Users/lan/.harness/frictions", "destination": "frictions", - "kind": "directory" + "kind": "directory", + "ownerScoped": true } ] } \ No newline at end of file diff --git a/packages/agent-memory-sync/src/config/loader.ts b/packages/agent-memory-sync/src/config/loader.ts index 1777c48..de10427 100644 --- a/packages/agent-memory-sync/src/config/loader.ts +++ b/packages/agent-memory-sync/src/config/loader.ts @@ -13,6 +13,10 @@ interface SyncPathConfig { destination?: string; kind?: "file" | "directory"; required?: boolean; + // See src/memory-sync/config.ts's SyncPathConfig.ownerScoped for the full + // rationale — this is the same field, mirrored here since this file + // declares its own structural copy of the shape rather than importing it. + ownerScoped?: boolean; } interface UserConfig { @@ -435,7 +439,8 @@ function normalizeSyncPathConfigList(value?: SyncPathConfig[]): SyncPathConfig[] source: entry.source, destination: entry.destination || entry.source, kind: entry.kind, - required: Boolean(entry.required) + required: Boolean(entry.required), + ownerScoped: Boolean(entry.ownerScoped) }; }); } diff --git a/packages/agent-memory-sync/src/memory-sync/config.ts b/packages/agent-memory-sync/src/memory-sync/config.ts index db10ccc..5aaa263 100644 --- a/packages/agent-memory-sync/src/memory-sync/config.ts +++ b/packages/agent-memory-sync/src/memory-sync/config.ts @@ -7,12 +7,24 @@ interface SyncPathConfig { destination?: string; kind?: "file" | "directory"; required?: boolean; + // Marks a directory-kind entry as machine-exclusive (one owner file per + // machine, named `.json` — the machine-state/frictions + // convention documented in profiles/linux.json). Absent/false is the + // pre-existing behavior (every file under the directory is offered). + // See collectLocalSyncFiles' `options.ownerFilter` below: this field only + // takes effect there, i.e. only for the PUSH collection, never for pull. + ownerScoped?: boolean; } interface RunConfig { rootDir: string; repositorySubdir: string; syncPaths: SyncPathConfig[]; + // Present on every real caller (PushConfig/PullConfig both declare it + // required) — optional here only so a minimal hand-built config (as some + // existing tests use) still type-checks. Used solely to derive the + // `.json` owner filename below. + profile?: string; } interface LocalSyncFile { @@ -22,8 +34,20 @@ interface LocalSyncFile { content: string; } -function collectLocalSyncFiles(config: RunConfig): LocalSyncFile[] { +interface CollectLocalSyncFilesOptions { + // Applies the ownerScoped filter (see SyncPathConfig.ownerScoped above). + // Pull must NEVER set this — pull is the one place a peer's owner file is + // supposed to be materialized locally (D-004, + // .ai/runs/2026-08-03-sync-conflict-markers-echo/03-decisions.md); only + // push's own "what do I offer as my local snapshot" collection sets it, so + // a machine never re-offers a peer's file it merely pulled as if it were + // its own change (the Defect B echo/last-writer-wins race). + ownerFilter?: boolean; +} + +function collectLocalSyncFiles(config: RunConfig, options: CollectLocalSyncFilesOptions = {}): LocalSyncFile[] { const results: LocalSyncFile[] = []; + const ownerFileName = options.ownerFilter && config.profile ? `${config.profile}.json` : null; for (const entry of config.syncPaths) { const absoluteSource = resolveWorkspacePath(config.rootDir, entry.source); @@ -47,6 +71,19 @@ function collectLocalSyncFiles(config: RunConfig): LocalSyncFile[] { continue; } + if (kind === "directory" && entry.ownerScoped && ownerFileName) { + const ownerAbsolutePath = path.join(absoluteSource, ownerFileName); + if (existsSync(ownerAbsolutePath) && statSync(ownerAbsolutePath).isFile()) { + results.push({ + absolutePath: ownerAbsolutePath, + localRelativePath: normalizeLocalRelativePath(config.rootDir, ownerAbsolutePath), + remoteRelativePath: path.posix.join(destination, ownerFileName), + content: readFileSync(ownerAbsolutePath, "utf8") + }); + } + continue; + } + for (const nestedFile of walkFiles(absoluteSource)) { const nestedRelative = path.relative(absoluteSource, nestedFile).replace(/\\/g, "/"); results.push({ @@ -61,6 +98,55 @@ function collectLocalSyncFiles(config: RunConfig): LocalSyncFile[] { return results.sort((left, right) => left.remoteRelativePath.localeCompare(right.remoteRelativePath)); } +// Push-only companion to collectLocalSyncFiles' ownerFilter. Filtering the +// LOCAL snapshot alone is not enough: push's 3-way merge visits every path +// in `localFiles keys UNION baseFiles keys` (src/memory-sync/push.ts's +// applySnapshotToWorkingCopy), and the state store's base snapshot still +// legitimately carries a peer's ownerScoped file — it was written there by +// a prior pull's `stateStore.replaceBaseSnapshots(remoteMap)`. Left +// unfiltered, that foreign key survives in baseFiles alone (base non-null, +// local now absent because collectLocalSyncFiles dropped it, remote +// possibly having moved on since) and still gets visited: base !== local +// and base !== remote trips the genuine-conflict fallback, which would +// spuriously flag a "conflict" and write marker content combining an empty +// local half with the peer's real remote content — actively corrupting a +// file this machine never touched. Call this once, right where +// collectLocalSyncFiles' PUSH collection is also called, so both the +// "current" snapshot and anything newly enqueued from it stay consistent. +function filterOwnerScopedBaseMap( + config: RunConfig, + baseMap: Record +): Record { + if (!config.profile) { + return baseMap; + } + + const ownerFileName = `${config.profile}.json`; + const ownerScopedDestinations = config.syncPaths + .filter( + (entry) => + entry.ownerScoped && resolveSyncPathKind(resolveWorkspacePath(config.rootDir, entry.source), entry) === "directory" + ) + .map((entry) => normalizeRemoteRelativePath(entry.destination || entry.source)); + + if (ownerScopedDestinations.length === 0) { + return baseMap; + } + + const result: Record = {}; + for (const [key, value] of Object.entries(baseMap)) { + const owningDestination = ownerScopedDestinations.find( + (destination) => key === destination || key.startsWith(`${destination}/`) + ); + if (owningDestination && key !== path.posix.join(owningDestination, ownerFileName)) { + continue; + } + result[key] = value; + } + + return result; +} + function mapRemotePathToLocalAbsolute(config: RunConfig, remoteRelativePath: string): string | null { const normalizedRemotePath = normalizeRemoteRelativePath(remoteRelativePath); @@ -159,6 +245,7 @@ function isHiddenEntryName(name: string): boolean { module.exports = { collectLocalSyncFiles, + filterOwnerScopedBaseMap, mapRemotePathToLocalAbsolute, normalizeRemoteRelativePath, toRepositoryRelativePath diff --git a/packages/agent-memory-sync/src/memory-sync/merge.ts b/packages/agent-memory-sync/src/memory-sync/merge.ts index e8e0e3f..35ea919 100644 --- a/packages/agent-memory-sync/src/memory-sync/merge.ts +++ b/packages/agent-memory-sync/src/memory-sync/merge.ts @@ -11,6 +11,30 @@ interface MergeResult { conflict: boolean; } +// Detects a previous pass's inline conflict markers surviving inside a +// winning payload (`<<<<<<< local` / `=======` / `>>>>>>> remote`, each +// checked as a literal line prefix, line-anchored — not a whole-content +// match, since the real marker lines carry a label/content suffix). Used to +// keep every conflict:false return path below honest: none of them may hand +// back marker-carrying content while still claiming "clean". See +// .ai/runs/2026-08-03-sync-conflict-markers-echo/01-plan.md (Teil 1) for the +// pull-then-push cascade this closes: a genuine conflict on pull writes +// markers to the local file and a clean base; the very next push then saw +// remote === base (nothing else changed the remote in between) and took the +// "local wins" fast path below, silently re-labeling that marker-carrying +// local content as a clean win with conflicts=0. +function hasConflictMarkers(content: string | null): boolean { + if (content === null) { + return false; + } + + return content + .split("\n") + .some( + (line) => line.startsWith("<<<<<<< ") || line.startsWith("=======") || line.startsWith(">>>>>>> ") + ); +} + function mergeText(input: MergeInput): MergeResult { const { base, local, remote, strategy } = input; @@ -19,16 +43,16 @@ function mergeText(input: MergeInput): MergeResult { } if (local === base) { - return { content: remote, status: "remote", conflict: false }; + return { content: remote, status: "remote", conflict: hasConflictMarkers(remote) }; } if (remote === base) { - return { content: local, status: "local", conflict: false }; + return { content: local, status: "local", conflict: hasConflictMarkers(local) }; } const appendMerge = mergeAppendOnly(base, local, remote); if (appendMerge) { - return { content: appendMerge, status: "merged", conflict: false }; + return { content: appendMerge, status: "merged", conflict: hasConflictMarkers(appendMerge) }; } if (strategy === "local-wins") { @@ -84,5 +108,6 @@ function mergeAppendOnly(base: string | null, local: string | null, remote: stri } module.exports = { + hasConflictMarkers, mergeText }; diff --git a/packages/agent-memory-sync/src/memory-sync/push.ts b/packages/agent-memory-sync/src/memory-sync/push.ts index 2c11c98..36d588b 100644 --- a/packages/agent-memory-sync/src/memory-sync/push.ts +++ b/packages/agent-memory-sync/src/memory-sync/push.ts @@ -1,5 +1,6 @@ const { collectLocalSyncFiles, + filterOwnerScopedBaseMap, toRepositoryRelativePath } = require("./config"); const { RemoteUnavailableError } = require("../errors"); @@ -46,14 +47,26 @@ async function performPush(config: PushConfig, options: PushOptions) { const stateStore = new StateStore(config.stateDir, config.profile); stateStore.ensure(); - const currentLocalFiles = collectLocalSyncFiles(config); + // ownerFilter: true — this is the PUSH-side collection of "what is my + // local snapshot", the only place Defect B's echo (a peer's ownerScoped + // file, materialized locally by a prior pull, getting offered back as + // this machine's own change) can originate. Pull's own collectLocalSyncFiles + // call (src/memory-sync/pull.ts) deliberately omits this option — see + // config.ts's CollectLocalSyncFilesOptions and D-004 in + // .ai/runs/2026-08-03-sync-conflict-markers-echo/03-decisions.md. + const currentLocalFiles = collectLocalSyncFiles(config, { ownerFilter: true }); const currentLocalMap = Object.fromEntries( currentLocalFiles.map((file: { remoteRelativePath: string; content: string }) => [ file.remoteRelativePath, file.content ]) ); - const currentBaseMap = stateStore.readBaseSnapshots(); + // Strips any foreign ownerScoped file (e.g. a peer's machine-state/frictions + // file, materialized locally by a prior pull) out of the base snapshot + // too — filtering currentLocalMap above is not sufficient on its own, + // since applySnapshotToWorkingCopy's targetPaths is localFiles keys UNION + // baseFiles keys; see filterOwnerScopedBaseMap's own comment in config.ts. + const currentBaseMap = filterOwnerScopedBaseMap(config, stateStore.readBaseSnapshots()); const queuedSnapshots = stateStore.listQueuedSnapshots(); const snapshots = [ diff --git a/packages/agent-memory-sync/tests/integration/cross-machine-profiles.test.ts b/packages/agent-memory-sync/tests/integration/cross-machine-profiles.test.ts index a725393..05d6542 100644 --- a/packages/agent-memory-sync/tests/integration/cross-machine-profiles.test.ts +++ b/packages/agent-memory-sync/tests/integration/cross-machine-profiles.test.ts @@ -218,3 +218,39 @@ test("all committed profiles (macbook, mac-mini, linux, linux.example) declare t ); } }); + +// Pins Defect B's fix (agent-tasks 06d09cde / .ai/runs/2026-08-03-sync-conflict-markers-echo, +// D-002/D-003): machine-state and frictions are one-owner-file-per-machine +// destinations, so push must never re-offer a peer's file it only pulled — +// ownerScoped: true on both entries in every real machine profile is what +// makes collectLocalSyncFiles' ownerFilter (src/memory-sync/config.ts) +// actually engage. Deliberately scoped to the same 3 real profiles as the +// machine-state pin above, not linux.example.json — the template documents +// the convention but was never a live sync target, so it carries no +// machine-state entry at all and this task's brief only requires the flag on +// "die 3 committeten Profilen". +test("macbook, mac-mini, and linux profiles set ownerScoped: true on both their machine-state and frictions entries", () => { + const profileFiles = ["macbook.json", "mac-mini.json", "linux.json"]; + const settingsByFile = Object.fromEntries( + profileFiles.map((file) => [file, JSON.parse(readText(path.join(PROFILES_DIR, file)))]) + ); + + function findEntriesByDestination( + syncPaths: Array> | undefined, + destination: string + ): Array> { + return (syncPaths || []).filter((entry) => entry.destination === destination); + } + + for (const file of profileFiles) { + for (const destination of ["machine-state", "frictions"]) { + const [entry] = findEntriesByDestination(settingsByFile[file].syncPaths, destination); + assert.ok(entry, `profiles/${file} must declare a syncPaths entry with destination '${destination}'`); + assert.equal( + entry.ownerScoped, + true, + `profiles/${file}'s '${destination}' entry must set ownerScoped: true, got: ${JSON.stringify(entry.ownerScoped)}` + ); + } + } +}); diff --git a/packages/agent-memory-sync/tests/integration/owner-scoped-push.test.ts b/packages/agent-memory-sync/tests/integration/owner-scoped-push.test.ts new file mode 100644 index 0000000..7d19751 --- /dev/null +++ b/packages/agent-memory-sync/tests/integration/owner-scoped-push.test.ts @@ -0,0 +1,183 @@ +// Repro B (agent-tasks 06d09cde / .ai/runs/2026-08-03-sync-conflict-markers-echo). +// +// Defect: a directory-kind syncPaths entry (e.g. machine-state, frictions) +// has no ownership concept — collectLocalSyncFiles (src/memory-sync/config.ts) +// offers every file under the directory, including a peer's file this +// machine merely materialized via a prior `pull`. A subsequent `push` then +// re-offers that peer's file as if it were this machine's own local change +// (an echo) and can win a last-writer-wins race against the peer's own, +// newer push. +// +// Fix (Teil 2, D-002/D-003/D-004 in +// .ai/runs/2026-08-03-sync-conflict-markers-echo/03-decisions.md): an +// optional `ownerScoped: true` on a directory syncPaths entry restricts what +// PUSH offers from that directory to exactly `.json` — this +// machine's own file, named after its own `profile` config field. Pull is +// untouched (D-004) — a peer's file is still pulled/materialized locally, +// exactly like today. +// +// This test proves the echo is eliminated even in the adversarial case where +// it would otherwise matter most: machine B's locally-materialized copy of +// machine A's file is STALE (A pushed again after B's last pull) — without +// the fix, B's push could overwrite A's newer remote content with B's stale +// local copy. With the fix, B never offers A's file at all, regardless of +// staleness. +const test = require("node:test"); +const assert = require("node:assert/strict"); +const path = require("node:path"); +const { + cloneRemote, + createSandbox, + fileExists, + initBareRemote, + readText, + runCli, + writeProjectConfig, + writeText +} = require("../helpers/cli.ts"); + +function ownerScopedConfig( + workspaceRoot: string, + remoteDir: string, + stateDir: string, + profile: string, + machineStateSource: string +) { + return { + profile, + rootDir: workspaceRoot, + remoteUrl: remoteDir, + branch: "main", + repositorySubdir: "shared", + stateDir, + conflictStrategy: "inline-markers", + syncPaths: [ + { source: "MEMORY.md", destination: "MEMORY.md", kind: "file" }, + { source: machineStateSource, destination: "machine-state", kind: "directory", ownerScoped: true } + ] + }; +} + +test( + "push never re-offers a peer's ownerScoped file that was only materialized locally by a prior pull, " + + "even when that local copy is stale", + () => { + const root = createSandbox("owner-scoped-echo"); + const remoteDir = initBareRemote(root); + + // Machine A: profile "machine-a", writes its own machine-state file and + // pushes it. + const workspaceA = path.join(root, "workspace-a"); + const stateDirA = path.join(root, "state-a"); + const configPathA = path.join(root, "config-a.json"); + const machineStateSourceA = path.join(root, "machine-a-harness-state"); + + writeText(path.join(workspaceA, "MEMORY.md"), "machine a memory\n"); + writeText(path.join(machineStateSourceA, "machine-a.json"), '{"v":1}\n'); + writeProjectConfig( + configPathA, + ownerScopedConfig(workspaceA, remoteDir, stateDirA, "machine-a", machineStateSourceA) + ); + + // The profile name is passed positionally (not "default") because the + // CLI's [profile] positional argument overrides the config file's + // "profile" field in resolveRunConfig's merge order (overrides applied + // last) — the same "pass BOTH --config and the profile name" pattern + // profiles/mac-mini.json etc. document, and now load-bearing: the + // ownerScoped filter's `.json` filename comes from this field. + const pushA1 = runCli(["run", "machine-a", "--config", configPathA, "--mode", "push", "--output", "json"]); + const pushA1Payload = JSON.parse(pushA1.stdout).runs[0]; + assert.equal(pushA1Payload.status, "applied"); + assert.ok( + pushA1Payload.appliedFiles.includes("machine-state/machine-a.json"), + `expected machine-state/machine-a.json in appliedFiles: ${JSON.stringify(pushA1Payload.appliedFiles)}` + ); + + // Machine B: profile "machine-b", pulls — this materializes A's file + // locally under B's own machine-state source directory (pull is + // unaffected by ownerScoped, D-004). + const workspaceB = path.join(root, "workspace-b"); + const stateDirB = path.join(root, "state-b"); + const configPathB = path.join(root, "config-b.json"); + const machineStateSourceB = path.join(root, "machine-b-harness-state"); + + writeProjectConfig( + configPathB, + ownerScopedConfig(workspaceB, remoteDir, stateDirB, "machine-b", machineStateSourceB) + ); + + const pullB = runCli(["run", "machine-b", "--config", configPathB, "--mode", "pull", "--output", "json"]); + const pullBPayload = JSON.parse(pullB.stdout).runs[0]; + assert.ok( + pullBPayload.appliedFiles.includes("machine-state/machine-a.json"), + `expected pull to materialize the peer file locally: ${JSON.stringify(pullBPayload.appliedFiles)}` + ); + assert.equal(readText(path.join(machineStateSourceB, "machine-a.json")), '{"v":1}\n'); + + // A advances again, so B's locally-materialized copy of A's file is now + // stale relative to the remote — the case where an echo could actually + // clobber A's newer content via a last-writer-wins race. + writeText(path.join(machineStateSourceA, "machine-a.json"), '{"v":2}\n'); + const pushA2 = runCli(["run", "machine-a", "--config", configPathA, "--mode", "push", "--output", "json"]); + assert.equal(JSON.parse(pushA2.stdout).runs[0].status, "applied"); + + // B now writes its OWN machine-state file (the legitimate case that must + // keep working) and pushes. B's stale local copy of A's file is still + // sitting under machineStateSourceB, untouched. + writeText(path.join(machineStateSourceB, "machine-b.json"), '{"v":1}\n'); + const pushB = runCli(["run", "machine-b", "--config", configPathB, "--mode", "push", "--output", "json"]); + const pushBPayload = JSON.parse(pushB.stdout).runs[0]; + + assert.equal(pushBPayload.status, "applied"); + assert.ok( + pushBPayload.appliedFiles.includes("machine-state/machine-b.json"), + `negative control: B's own file must still be pushed: ${JSON.stringify(pushBPayload.appliedFiles)}` + ); + assert.ok( + !pushBPayload.appliedFiles.includes("machine-state/machine-a.json"), + `B must not echo-push A's file: ${JSON.stringify(pushBPayload.appliedFiles)}` + ); + assert.ok( + !pushBPayload.conflictFiles.includes("machine-state/machine-a.json"), + `B must not even attempt a merge over A's file: ${JSON.stringify(pushBPayload.conflictFiles)}` + ); + + // The remote must still hold A's latest content — not overwritten by + // B's stale echo — and must now also carry B's own file. + const inspection = cloneRemote(remoteDir, root, "inspect-after-b-push"); + assert.equal( + readText(path.join(inspection, "shared", "machine-state", "machine-a.json")), + '{"v":2}\n', + "A's newer remote content must survive untouched by B's push" + ); + assert.equal(readText(path.join(inspection, "shared", "machine-state", "machine-b.json")), '{"v":1}\n'); + } +); + +test("push tolerates an ownerScoped directory whose own .json does not exist locally yet", () => { + const root = createSandbox("owner-scoped-missing-own-file"); + const remoteDir = initBareRemote(root); + const workspaceRoot = path.join(root, "workspace"); + const stateDir = path.join(root, "state"); + const configPath = path.join(root, "config.json"); + const machineStateSource = path.join(root, "harness-state"); + + writeText(path.join(workspaceRoot, "MEMORY.md"), "seed\n"); + // machineStateSource exists (so the directory-existence check passes) but + // has no .json in it yet. + writeText(path.join(machineStateSource, "someone-elses.json"), '{"v":1}\n'); + writeProjectConfig(configPath, ownerScopedConfig(workspaceRoot, remoteDir, stateDir, "this-machine", machineStateSource)); + + const result = runCli(["run", "this-machine", "--config", configPath, "--mode", "push", "--output", "json"]); + const payload = JSON.parse(result.stdout).runs[0]; + + assert.equal(payload.status, "applied"); + assert.ok(payload.appliedFiles.some((f: string) => f.endsWith("MEMORY.md"))); + assert.ok( + !payload.appliedFiles.some((f: string) => f.startsWith("machine-state/")), + `no own file present yet — nothing under machine-state/ should be offered: ${JSON.stringify(payload.appliedFiles)}` + ); + + const inspection = cloneRemote(remoteDir, root, "inspect-no-own-file"); + assert.equal(fileExists(path.join(inspection, "shared", "machine-state", "someone-elses.json")), false); +}); diff --git a/packages/agent-memory-sync/tests/integration/push-conflict-marker-honesty.test.ts b/packages/agent-memory-sync/tests/integration/push-conflict-marker-honesty.test.ts new file mode 100644 index 0000000..633d2b2 --- /dev/null +++ b/packages/agent-memory-sync/tests/integration/push-conflict-marker-honesty.test.ts @@ -0,0 +1,135 @@ +// Repro A (agent-tasks 06d09cde / .ai/runs/2026-08-03-sync-conflict-markers-echo). +// +// Live incident this pins: `run --mode sync` runs performPull then +// performPush back-to-back (src/commands/run.ts executeMode). A genuine +// conflict on pull writes inline conflict markers to the local file +// (src/memory-sync/pull.ts) and — correctly — sets the state store's base +// snapshot to the CLEAN remote content it just fetched +// (stateStore.replaceBaseSnapshots(remoteMap)). The very next push in that +// same sync then reads that marker-corrupted file back off local disk +// (collectLocalSyncFiles) as its "local" snapshot; since nothing else +// changed the remote in between, push's own 3-way merge sees +// `remote === base` and takes the "local wins" fast path +// (src/memory-sync/merge.ts) — which, before this fix, returned +// `conflict: false` unconditionally, so the marker-carrying content was +// pushed to the remote AND committed to the local base snapshot while the +// run reported a clean 0-conflict outcome. Two rapid remote pushes plus a +// concurrent local edit produced exactly this on the mac mini on +// 2026-08-03 (see 00-goal.md's "Reproduktion" section). +// +// This test drives pull and push as two separate CLI invocations against the +// SAME stateDir/rootDir/remote (the task brief explicitly allows "im selben +// Prozess/StateStore" — every relevant artifact pull leaves behind, the +// marker-corrupted local file and the state store's base snapshot, is on +// disk, so two sequential CLI calls reproduce the identical on-disk state +// transition a single in-process `--mode sync` would). Splitting the calls +// also lets this test inspect the PUSH step's own JSON report in isolation — +// asserting only on a combined `--mode sync` result would not distinguish +// the bug from the fix, because pull's OWN conflict detection for the +// initial marker-producing conflict was never broken (only the SUBSEQUENT +// push's re-labeling of that already-marker-carrying content was). +const test = require("node:test"); +const assert = require("node:assert/strict"); +const path = require("node:path"); +const { + cloneRemote, + createSandbox, + git, + initBareRemote, + readText, + runCli, + writeProjectConfig, + writeText +} = require("../helpers/cli.ts"); + +function createConfig(workspaceRoot: string, remoteDir: string, stateDir: string) { + return { + rootDir: workspaceRoot, + remoteUrl: remoteDir, + branch: "main", + repositorySubdir: "shared", + stateDir, + conflictStrategy: "inline-markers", + syncPaths: [{ source: "MEMORY.md", destination: "MEMORY.md", kind: "file" }] + }; +} + +test( + "a push immediately following a pull that produced local conflict markers reports the file as a " + + "conflict, not silently as a clean local win", + () => { + const root = createSandbox("push-conflict-marker-honesty"); + const remoteDir = initBareRemote(root); + const workspaceRoot = path.join(root, "workspace"); + const stateDir = path.join(root, "state"); + const configPath = path.join(root, "config.json"); + + writeText(path.join(workspaceRoot, "MEMORY.md"), "base\n"); + writeProjectConfig(configPath, createConfig(workspaceRoot, remoteDir, stateDir)); + + // Establish "base\n" as both the remote content and this workspace's + // known base snapshot. + const seed = runCli(["run", "default", "--config", configPath, "--mode", "push", "--output", "json"]); + assert.equal(JSON.parse(seed.stdout).runs[0].status, "applied"); + + // A peer fully replaces the remote file (not an append — the base/local/ + // remote three-way append merge must not silently absorb this), without + // this workspace ever pulling it. + const peerCheckout = cloneRemote(remoteDir, root, "peer"); + writeText(path.join(peerCheckout, "shared", "MEMORY.md"), "remote v2\n"); + git(["add", "."], peerCheckout); + git(["commit", "-m", "remote replaced"], peerCheckout); + git(["push", "origin", "HEAD:main"], peerCheckout); + + // Meanwhile this workspace also fully replaces its local copy, with + // content unrelated to the remote's replacement — base, local, and + // remote now all three differ and are not append-compatible, so pull + // must hit the genuine single-pass conflict fallback. + writeText(path.join(workspaceRoot, "MEMORY.md"), "local v2\n"); + + const pullResult = runCli(["run", "default", "--config", configPath, "--mode", "pull", "--output", "json"]); + const pullPayload = JSON.parse(pullResult.stdout).runs[0]; + assert.ok( + pullPayload.conflictFiles.includes("MEMORY.md"), + `sanity: pull's own genuine conflict must be reported, got: ${JSON.stringify(pullPayload.conflictFiles)}` + ); + + const afterPull = readText(path.join(workspaceRoot, "MEMORY.md")); + assert.match(afterPull, /<<<<<<< local/, "sanity: pull must have written inline conflict markers locally"); + assert.match(afterPull, /local v2/); + assert.match(afterPull, /remote v2/); + assert.match(afterPull, />>>>>>> remote/); + + // Nothing else touches the remote between the pull and this push, so + // push's own 3-way merge sees remote === base and takes the "local + // wins" fast path with the marker-carrying local content as the winner + // — the exact blind spot this task closes. + const pushResult = runCli(["run", "default", "--config", configPath, "--mode", "push", "--output", "json"]); + const pushPayload = JSON.parse(pushResult.stdout).runs[0]; + + assert.ok( + pushPayload.appliedFiles.includes("MEMORY.md"), + `expected MEMORY.md among push's appliedFiles: ${JSON.stringify(pushPayload.appliedFiles)}` + ); + assert.ok( + pushPayload.conflictFiles.includes("MEMORY.md"), + "push must report the marker-carrying file as a conflict, not silently as a clean local win " + + `(conflicts=0 hides that a real conflict landed on the remote); got conflictFiles: ${JSON.stringify( + pushPayload.conflictFiles + )}` + ); + + // The payload itself is intentionally not rewritten by this fix — only + // the conflict flag becomes honest. The marker content still reaches + // the remote (identical to what a genuine single-pass conflict already + // does, and already covered by watch-mirror-delete.test.ts's pinned + // negative control) — but now correctly flagged as a conflict instead + // of hidden behind conflicts=0. + const inspection = cloneRemote(remoteDir, root, "inspect-after-push"); + const remoteContent = readText(path.join(inspection, "shared", "MEMORY.md")); + assert.match(remoteContent, /<<<<<<< local/); + assert.match(remoteContent, /local v2/); + assert.match(remoteContent, /remote v2/); + assert.match(remoteContent, />>>>>>> remote/); + } +); diff --git a/packages/agent-memory-sync/tests/unit/merge.test.ts b/packages/agent-memory-sync/tests/unit/merge.test.ts new file mode 100644 index 0000000..f387937 --- /dev/null +++ b/packages/agent-memory-sync/tests/unit/merge.test.ts @@ -0,0 +1,131 @@ +// Unit coverage for mergeText's marker-honesty guard (Teil 1, agent-tasks +// 06d09cde / .ai/runs/2026-08-03-sync-conflict-markers-echo). +// +// Root defect: both fast paths (`local === base` -> remote wins, +// `remote === base` -> local wins) and the appendOnly merge success path +// unconditionally returned `conflict: false`, even when the winning content +// itself already carried inline conflict markers left over from an earlier +// pass (e.g. a genuine conflict on `pull` writes `<<<<<<< local` / +// `=======` / `>>>>>>> remote` to the local file, then the very next `push` +// in the same sync sees `remote === base` and silently re-labels that +// marker-carrying local content as a clean "local wins" with conflicts=0 — +// see push-conflict-marker-honesty.test.ts for the end-to-end repro). This +// file pins the fix at the unit level: hasConflictMarkers() itself, and each +// of the three conflict:false paths upgrading to conflict:true when their +// returned content carries markers, while leaving the actual designed +// single-pass conflict fallback (marker construction, already +// conflict:true) and the untouched `unchanged`/strategy paths alone. +const test = require("node:test"); +const assert = require("node:assert/strict"); +const { hasConflictMarkers, mergeText } = require("../../src/memory-sync/merge"); + +// ─── hasConflictMarkers ────────────────────────────────────────────────────── + +test("hasConflictMarkers: null content has no markers", () => { + assert.equal(hasConflictMarkers(null), false); +}); + +test("hasConflictMarkers: plain content without markers", () => { + assert.equal(hasConflictMarkers("just some ordinary text\nwith multiple lines\n"), false); +}); + +test("hasConflictMarkers: detects a '<<<<<<< local' line", () => { + assert.equal(hasConflictMarkers("before\n<<<<<<< local\nafter\n"), true); +}); + +test("hasConflictMarkers: detects a '=======' line", () => { + assert.equal(hasConflictMarkers("before\n=======\nafter\n"), true); +}); + +test("hasConflictMarkers: detects a '>>>>>>> remote' line", () => { + assert.equal(hasConflictMarkers("before\n>>>>>>> remote\nafter\n"), true); +}); + +test("hasConflictMarkers: a marker substring that is not at the start of a line does not count (line-anchored)", () => { + assert.equal(hasConflictMarkers("some text mentioning <<<<<<< local mid-line\n"), false); + assert.equal(hasConflictMarkers("text ======= mid-line\n"), false); + assert.equal(hasConflictMarkers("text >>>>>>> remote mid-line\n"), false); +}); + +test("hasConflictMarkers: recognizes the exact three-line marker block mergeText itself constructs", () => { + const content = ["<<<<<<< local", "local content", "=======", "remote content", ">>>>>>> remote"].join("\n"); + assert.equal(hasConflictMarkers(content), true); +}); + +// ─── mergeText: fast paths upgrade conflict:false -> conflict:true ────────── + +test("mergeText: local === base (remote wins) with a clean remote stays conflict:false", () => { + const result = mergeText({ base: "same\n", local: "same\n", remote: "remote update\n", strategy: "inline-markers" }); + assert.equal(result.status, "remote"); + assert.equal(result.content, "remote update\n"); + assert.equal(result.conflict, false); +}); + +test("mergeText: local === base (remote wins) is upgraded to conflict:true when the remote winner already carries markers", () => { + const markerRemote = ["<<<<<<< local", "stale local", "=======", "stale remote", ">>>>>>> remote"].join("\n"); + const result = mergeText({ base: "same\n", local: "same\n", remote: markerRemote, strategy: "inline-markers" }); + assert.equal(result.status, "remote"); + assert.equal(result.content, markerRemote, "payload must not be rewritten, only the conflict flag"); + assert.equal(result.conflict, true); +}); + +test("mergeText: remote === base (local wins) with clean local stays conflict:false", () => { + const result = mergeText({ base: "same\n", local: "local edit\n", remote: "same\n", strategy: "inline-markers" }); + assert.equal(result.status, "local"); + assert.equal(result.content, "local edit\n"); + assert.equal(result.conflict, false); +}); + +test("mergeText: remote === base (local wins) is upgraded to conflict:true when the local winner already carries markers", () => { + const markerLocal = ["<<<<<<< local", "stale local", "=======", "stale remote", ">>>>>>> remote"].join("\n"); + const result = mergeText({ base: "same\n", local: markerLocal, remote: "same\n", strategy: "inline-markers" }); + assert.equal(result.status, "local"); + assert.equal(result.content, markerLocal, "payload must not be rewritten, only the conflict flag"); + assert.equal(result.conflict, true); +}); + +// ─── mergeText: appendOnly success path upgrades too ──────────────────────── + +test("mergeText: appendOnly merge of two clean, non-overlapping suffixes stays conflict:false", () => { + const result = mergeText({ + base: "base\n", + local: "base\nlocal addition\n", + remote: "base\nremote addition\n", + strategy: "inline-markers" + }); + assert.equal(result.status, "merged"); + assert.equal(result.content, "base\nremote addition\nlocal addition\n"); + assert.equal(result.conflict, false); +}); + +test("mergeText: appendOnly merge is upgraded to conflict:true when the merged result carries markers (e.g. a marker-carrying local suffix)", () => { + // local's suffix (everything after base) itself already contains a marker + // line, simulating a prior conflict's leftover content being appended to + // again rather than replaced outright. + const local = "base\n<<<<<<< local\nstale\n=======\nstale2\n>>>>>>> remote\n"; + const remote = "base\nremote addition\n"; + const result = mergeText({ base: "base\n", local, remote, strategy: "inline-markers" }); + assert.equal(result.status, "merged"); + assert.equal(hasConflictMarkers(result.content), true, "sanity: the merged content really does carry markers"); + assert.equal(result.conflict, true); +}); + +// ─── negative controls: paths the guard must NOT touch ────────────────────── + +test("mergeText: unchanged (local === remote) stays conflict:false even when both already carry markers (nothing changed, not a new conflict)", () => { + const markerContent = ["<<<<<<< local", "x", "=======", "y", ">>>>>>> remote"].join("\n"); + const result = mergeText({ base: "irrelevant\n", local: markerContent, remote: markerContent, strategy: "inline-markers" }); + assert.equal(result.status, "unchanged"); + assert.equal(result.conflict, false); +}); + +test("mergeText: genuine single-pass conflict (no clean fast path, no append merge) still builds markers and reports conflict:true, unchanged by this fix", () => { + const result = mergeText({ base: "base\n", local: "local replaced\n", remote: "remote replaced\n", strategy: "inline-markers" }); + assert.equal(result.status, "conflict"); + assert.equal(result.conflict, true); + assert.match(result.content, /<<<<<<< local/); + assert.match(result.content, /local replaced/); + assert.match(result.content, /=======/); + assert.match(result.content, /remote replaced/); + assert.match(result.content, />>>>>>> remote/); +}); From 3d6715cf6dae6285dc6f0b8af2e55d94e3b76cb4 Mon Sep 17 00:00:00 2001 From: Lan Nguyen Si Date: Mon, 3 Aug 2026 20:04:51 +0200 Subject: [PATCH 2/2] fix(memory-sync): address review findings (visible own-file-drop warning, precise marker detection, queue-replay owner filter) Review of f51ff9f (agent-tasks 06d09cde) raised one HIGH + two MEDIUM + two LOW; all fixed here (the local===remote unchanged-path LOW is consciously accepted, its pin unchanged): HIGH (silent own-state data loss): owner-writes-only made config.profile load-bearing; a resolved profile that doesn't match the machine's owner filename silently published nothing for its own machine-state/frictions. Now collectLocalSyncFiles emits a visible warning (surfaced via the operation's notes line) when an ownerScoped directory holds peer files but not this machine's own .json; still tolerant (no exception) when the directory is empty. CLI profile-resolution semantics are unchanged (minimal fix). MEDIUM (marker false-positive): hasConflictMarkers narrowed to the unambiguous '<<<<<<< ' opener line only, so setext '=======' headings, '===' dividers, and deep '>>>>>>> ' blockquotes in ordinary memory Markdown are no longer flagged as conflicts; an inherited full block still carries the opener and is detected. MEDIUM (queue-replay echo): queued snapshots' localFiles/baseFiles are now routed through filterOwnerScopedBaseMap on replay, so a snapshot enqueued before this machine adopted ownerScoped can no longer echo a peer's file. LOW: local-wins/remote-wins strategy branches upgrade conflict:false->true on a marker-carrying winner (invariant completeness); corrected the misleading 'profile is a cosmetic label' comments in all three profiles. Tests: 9 new/changed (own-file-drop warning + empty-dir control, 5 Markdown exclusion units, 2 queue-replay strip tests, 2 wins-strategy honesty units); full suite 137 pass / 0 fail; build + typecheck clean; coverage 95.14/76.80/ 93.55 (not worsened). Refs: agent-memory task 06d09cde Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01GbC4gdPEf4T1YD8LZV9GA7 --- .../agent-memory-sync/profiles/linux.json | 16 +- .../agent-memory-sync/profiles/mac-mini.json | 45 +++-- .../agent-memory-sync/profiles/macbook.json | 45 +++-- .../src/memory-sync/config.ts | 32 +++ .../src/memory-sync/merge.ts | 39 ++-- .../agent-memory-sync/src/memory-sync/push.ts | 122 ++++++++---- .../integration/owner-scoped-push.test.ts | 186 +++++++++++++++++- .../tests/unit/merge.test.ts | 91 ++++++++- 8 files changed, 490 insertions(+), 86 deletions(-) diff --git a/packages/agent-memory-sync/profiles/linux.json b/packages/agent-memory-sync/profiles/linux.json index c8f3d32..e6b1f5f 100644 --- a/packages/agent-memory-sync/profiles/linux.json +++ b/packages/agent-memory-sync/profiles/linux.json @@ -41,8 +41,22 @@ "D-004) for the echo/last-writer-wins race this closes. Pull is", "unaffected — a peer's file is still materialized locally as before.", "", + "The 'profile' field ('linux' below) is NOT a cosmetic label, despite", + "what linux.example.json's template comment (referenced above) may", + "suggest — corrected post Fix-Runde agent-tasks 06d09cde. It is exactly", + "the '.json' filename the ownerScoped filter just above looks", + "for. Since the CLI's [profile] positional argument overrides this", + "field and always defaults to 'default' when omitted, the Activation", + "command below is not just self-documenting: omitting the positional,", + "or passing anything other than 'linux', makes push look for", + "'default.json' in machine-state/frictions instead of 'linux.json'; it", + "won't find it, and — since this machine's real files ARE present", + "alongside it — push now logs a visible warning and publishes no", + "machine-state/frictions state for that run, instead of the pre-fix", + "silent no-op.", + "", "Activation: pass BOTH --config (this file) and the profile name", - "positionally:", + "positionally, exactly as shown — it must match 'linux' below:", " agent-memory-sync run linux --config profiles/linux.json" ], "profile": "linux", diff --git a/packages/agent-memory-sync/profiles/mac-mini.json b/packages/agent-memory-sync/profiles/mac-mini.json index 4ef6144..a379944 100644 --- a/packages/agent-memory-sync/profiles/mac-mini.json +++ b/packages/agent-memory-sync/profiles/mac-mini.json @@ -27,15 +27,25 @@ "top-level trees in the same bare repo and never see each other's", "pushes (a live cross-machine E2E test caught this: mini `pull` after a", "MacBook `push` reported applied=0). Both profiles now use the same", - "'pandora' value. The 'profile' field below is a DIFFERENT, unrelated", - "setting from repositorySubdir. Precisely: resolveRunConfig() only", - "derives stateDir from 'profile' (.agent-memory-sync/) when", - "stateDir is NOT set explicitly — this file DOES set stateDir", - "explicitly (below), so 'profile' currently has NO effect on any file", - "path at all; it only ends up recorded as a cosmetic label inside this", - "machine's state.json. Either way it never touches the remote, so it's", - "safe for it to differ (or coincide) across machines — it does differ", - "here: 'mac-mini' vs. macbook.json's 'macbook'.", + "'pandora' value. The 'profile' field below is a DIFFERENT setting from", + "repositorySubdir, and is NOT a cosmetic label (corrected post Fix-Runde", + "agent-tasks 06d09cde — an earlier revision of this comment claimed it", + "was). resolveRunConfig() only derives stateDir from 'profile'", + "(.agent-memory-sync/) when stateDir is NOT set explicitly —", + "this file DOES set stateDir explicitly (below), so 'profile' still has", + "no effect on stateDir specifically. But collectLocalSyncFiles' push-side", + "ownerScoped filter (src/memory-sync/config.ts) derives the owner", + "filename for the machine-state/frictions entries below from THIS field:", + "push only ever offers '.json', so it MUST equal this machine's", + "own filename in those directories (mac-mini.json here) or push silently", + "— now: with a visible warning instead — finds no matching owner file", + "and publishes no machine-state/frictions state for this machine. See", + "the Activation note near the end of this comment: the CLI's [profile]", + "positional argument overrides this field and is the more common way to", + "get this mismatch. It still never touches the remote path/tree", + "(repositorySubdir alone determines that), so the raw value is still", + "safe to differ across machines, as it does here: 'mac-mini' vs.", + "macbook.json's 'macbook' — each machine's own real filename.", "", "syncPaths' FIRST entry is a directory entry covering the ENTIRE rootDir", "(source '.'), so every file agent-memory-sync finds there is synced —", @@ -106,11 +116,18 @@ "Activation: pass BOTH --config (this file) and the 'mac-mini' profile", "argument, e.g. `agent-memory-sync run mac-mini --config profiles/mac-mini.json`.", "The CLI's [profile] positional argument always defaults to 'default' and", - "overrides this file's 'profile' field when omitted. Since stateDir is", - "set explicitly below, that override has NO effect on where state files", - "(queue/, base/, tmp/) land here specifically — pass the positional", - "argument anyway for a self-documenting invocation and a correct", - "'profile' label in this run's output/state.json. See docs/machine-setup.md.", + "overrides this file's 'profile' field when omitted — this is now", + "load-bearing, not just a labeling nicety (see the ownerScoped paragraph", + "above): the resolved profile also selects which '.json' push", + "offers from the machine-state/frictions entries. Omitting the positional", + "(or passing anything other than 'mac-mini') makes push look for", + "'default.json' there instead of 'mac-mini.json'; it won't find it, and —", + "since this machine's real machine-state/frictions files ARE present", + "alongside it — push now logs a visible warning and publishes no", + "machine-state/frictions state for that run, instead of the pre-fix", + "silent no-op. It still has no effect on where state files (queue/,", + "base/, tmp/) land here, since stateDir is set explicitly below. Always", + "pass the positional argument exactly as shown. See docs/machine-setup.md.", "", "Reminder (see docs/machine-setup.md / README.md #systemd-unit): `watch`", "alone is not sufficient even on the mini if you also run it here for", diff --git a/packages/agent-memory-sync/profiles/macbook.json b/packages/agent-memory-sync/profiles/macbook.json index 978bfa3..6c199d6 100644 --- a/packages/agent-memory-sync/profiles/macbook.json +++ b/packages/agent-memory-sync/profiles/macbook.json @@ -19,15 +19,25 @@ "top-level trees in the same bare repo and never see each other's", "pushes (a live cross-machine E2E test caught this: mini `pull` after a", "MacBook `push` reported applied=0). Both profiles now use the same", - "'pandora' value. The 'profile' field below is a DIFFERENT, unrelated", - "setting from repositorySubdir. Precisely: resolveRunConfig() only", - "derives stateDir from 'profile' (.agent-memory-sync/) when", - "stateDir is NOT set explicitly \u2014 this file DOES set stateDir", - "explicitly (below), so 'profile' currently has NO effect on any file", - "path at all; it only ends up recorded as a cosmetic label inside this", - "machine's state.json. Either way it never touches the remote, so it's", - "safe for it to differ (or coincide) across machines \u2014 it does differ", - "here: 'macbook' vs. mac-mini.json's 'mac-mini'.", + "'pandora' value. The 'profile' field below is a DIFFERENT setting from", + "repositorySubdir, and is NOT a cosmetic label (corrected post Fix-Runde", + "agent-tasks 06d09cde \u2014 an earlier revision of this comment claimed it", + "was). resolveRunConfig() only derives stateDir from 'profile'", + "(.agent-memory-sync/) when stateDir is NOT set explicitly \u2014", + "this file DOES set stateDir explicitly (below), so 'profile' still has", + "no effect on stateDir specifically. But collectLocalSyncFiles' push-side", + "ownerScoped filter (src/memory-sync/config.ts) derives the owner", + "filename for the machine-state/frictions entries below from THIS field:", + "push only ever offers '.json', so it MUST equal this machine's", + "own filename in those directories (macbook.json here) or push silently", + "\u2014 now: with a visible warning instead \u2014 finds no matching owner file", + "and publishes no machine-state/frictions state for this machine. See", + "the Activation note near the end of this comment: the CLI's [profile]", + "positional argument overrides this field and is the more common way to", + "get this mismatch. It still never touches the remote path/tree", + "(repositorySubdir alone determines that), so the raw value is still", + "safe to differ across machines, as it does here: 'macbook' vs.", + "mac-mini.json's 'mac-mini' \u2014 each machine's own real filename.", "", "syncPaths' FIRST entry is a directory entry covering the ENTIRE rootDir", "(source '.'), so every file agent-memory-sync finds there is synced \u2014", @@ -96,11 +106,18 @@ "Activation: pass BOTH --config (this file) and the 'macbook' profile", "argument, e.g. `agent-memory-sync run macbook --config profiles/macbook.json`.", "The CLI's [profile] positional argument always defaults to 'default' and", - "overrides this file's 'profile' field when omitted. Since stateDir is", - "set explicitly below, that override has NO effect on where state files", - "(queue/, base/, tmp/) land here specifically — pass the positional", - "argument anyway for a self-documenting invocation and a correct", - "'profile' label in this run's output/state.json. See docs/machine-setup.md." + "overrides this file's 'profile' field when omitted — this is now", + "load-bearing, not just a labeling nicety (see the ownerScoped paragraph", + "above): the resolved profile also selects which '.json' push", + "offers from the machine-state/frictions entries. Omitting the positional", + "(or passing anything other than 'macbook') makes push look for", + "'default.json' there instead of 'macbook.json'; it won't find it, and —", + "since this machine's real machine-state/frictions files ARE present", + "alongside it — push now logs a visible warning and publishes no", + "machine-state/frictions state for that run, instead of the pre-fix", + "silent no-op. It still has no effect on where state files (queue/,", + "base/, tmp/) land here, since stateDir is set explicitly below. Always", + "pass the positional argument exactly as shown. See docs/machine-setup.md." ], "profile": "macbook", "rootDir": "/Users/lan/.claude/projects/-Users-lan-git-pandora/memory", diff --git a/packages/agent-memory-sync/src/memory-sync/config.ts b/packages/agent-memory-sync/src/memory-sync/config.ts index 5aaa263..e2687e7 100644 --- a/packages/agent-memory-sync/src/memory-sync/config.ts +++ b/packages/agent-memory-sync/src/memory-sync/config.ts @@ -43,6 +43,23 @@ interface CollectLocalSyncFilesOptions { // a machine never re-offers a peer's file it merely pulled as if it were // its own change (the Defect B echo/last-writer-wins race). ownerFilter?: boolean; + // Fix-Runde HIGH finding (05-review-findings.md, agent-tasks 06d09cde): + // when an ownerScoped directory has OTHER files but not this machine's own + // `.json`, the pre-fix code silently offered nothing for that + // destination — a real data-loss path, reachable whenever the resolved + // `config.profile` doesn't match the machine's actual owner filename (the + // CLI's [profile] positional defaults to 'default' and overrides the + // config file's 'profile' field when omitted — run.ts's `.argument` + // default plus loader.ts's override-order). Rather than staying silent, + // that situation now pushes a warning string into this caller-supplied + // array (an out-parameter, not a return-shape change, so callers that + // don't pass it — i.e. pull, which never sets ownerFilter either — + // continue to receive plain LocalSyncFile[] back, untouched). The caller + // (push.ts) surfaces any collected warning via the same `notes` array + // every other push/pull diagnostic already uses (see preview.ts's + // summarizeOperation, which renders `notes=...` in text output, and the + // JSON payload's own `notes` field). + warnings?: string[]; } function collectLocalSyncFiles(config: RunConfig, options: CollectLocalSyncFilesOptions = {}): LocalSyncFile[] { @@ -80,6 +97,21 @@ function collectLocalSyncFiles(config: RunConfig, options: CollectLocalSyncFiles remoteRelativePath: path.posix.join(destination, ownerFileName), content: readFileSync(ownerAbsolutePath, "utf8") }); + } else { + // Own file absent. Stay tolerant (no exception — a brand-new + // machine's first run legitimately has no .json yet), but + // only stay SILENT when the directory is genuinely empty of other + // content too. If other files ARE present, this machine has + // something to compare against and is about to publish nothing for + // this destination — that is the silent-data-loss path the HIGH + // finding flagged, so it becomes a visible warning instead. + const peerFiles = walkFiles(absoluteSource); + if (peerFiles.length > 0 && options.warnings) { + options.warnings.push( + `profile '${config.profile}': own file '${ownerFileName}' not found among ${peerFiles.length} file(s) in '${absoluteSource}'; ` + + `this machine will publish no '${destination}' state — check the profile positional matches this machine` + ); + } } continue; } diff --git a/packages/agent-memory-sync/src/memory-sync/merge.ts b/packages/agent-memory-sync/src/memory-sync/merge.ts index 35ea919..0316fe5 100644 --- a/packages/agent-memory-sync/src/memory-sync/merge.ts +++ b/packages/agent-memory-sync/src/memory-sync/merge.ts @@ -12,27 +12,34 @@ interface MergeResult { } // Detects a previous pass's inline conflict markers surviving inside a -// winning payload (`<<<<<<< local` / `=======` / `>>>>>>> remote`, each -// checked as a literal line prefix, line-anchored — not a whole-content -// match, since the real marker lines carry a label/content suffix). Used to -// keep every conflict:false return path below honest: none of them may hand -// back marker-carrying content while still claiming "clean". See +// winning payload. Used to keep every conflict:false return path below +// honest: none of them may hand back marker-carrying content while still +// claiming "clean". See // .ai/runs/2026-08-03-sync-conflict-markers-echo/01-plan.md (Teil 1) for the // pull-then-push cascade this closes: a genuine conflict on pull writes // markers to the local file and a clean base; the very next push then saw // remote === base (nothing else changed the remote in between) and took the // "local wins" fast path below, silently re-labeling that marker-carrying // local content as a clean win with conflicts=0. +// +// Only the `<<<<<<< ` opener line (checked as a literal, line-anchored +// prefix) is checked — deliberately NOT the bare `=======`/`>>>>>>> ` lines +// the block below also writes. mergeText only ever emits the full +// three-line block together (see the "conflict" fallback below), so the +// opener alone is already an unambiguous signal that inherited marker +// content is present; requiring it also is what keeps this from +// false-positiving on ordinary Markdown that legitimately starts a line +// with 7+ `=` (a setext H1 underline, an `====`-style section divider) or +// `>>>>>>> ` (a deeply nested blockquote/reply-quote line) — content agent +// memory files carry routinely and that earlier revisions of this check +// mis-flagged as conflict:true. Fix-Runde 05-review-findings.md MEDIUM +// finding #2 (agent-tasks 06d09cde). function hasConflictMarkers(content: string | null): boolean { if (content === null) { return false; } - return content - .split("\n") - .some( - (line) => line.startsWith("<<<<<<< ") || line.startsWith("=======") || line.startsWith(">>>>>>> ") - ); + return content.split("\n").some((line) => line.startsWith("<<<<<<< ")); } function mergeText(input: MergeInput): MergeResult { @@ -55,12 +62,20 @@ function mergeText(input: MergeInput): MergeResult { return { content: appendMerge, status: "merged", conflict: hasConflictMarkers(appendMerge) }; } + // Both wins-strategy branches upgrade conflict:false -> conflict:true when + // their picked winner already carries markers, mirroring the fast paths + // and the appendOnly success path above (Invariant-Vollstaendigkeit: every + // conflict:false return must hold for marker-free content). Unreachable + // today via the deployed inline-markers strategy, but local-wins/ + // remote-wins are still a public MergeInput.strategy value the honesty + // invariant must hold for. Fix-Runde 05-review-findings.md LOW finding #4 + // (agent-tasks 06d09cde). if (strategy === "local-wins") { - return { content: local, status: "conflict", conflict: false }; + return { content: local, status: "conflict", conflict: hasConflictMarkers(local) }; } if (strategy === "remote-wins") { - return { content: remote, status: "conflict", conflict: false }; + return { content: remote, status: "conflict", conflict: hasConflictMarkers(remote) }; } return { diff --git a/packages/agent-memory-sync/src/memory-sync/push.ts b/packages/agent-memory-sync/src/memory-sync/push.ts index 36d588b..a854d16 100644 --- a/packages/agent-memory-sync/src/memory-sync/push.ts +++ b/packages/agent-memory-sync/src/memory-sync/push.ts @@ -54,7 +54,11 @@ async function performPush(config: PushConfig, options: PushOptions) { // call (src/memory-sync/pull.ts) deliberately omits this option — see // config.ts's CollectLocalSyncFilesOptions and D-004 in // .ai/runs/2026-08-03-sync-conflict-markers-echo/03-decisions.md. - const currentLocalFiles = collectLocalSyncFiles(config, { ownerFilter: true }); + const ownerScopedWarnings: string[] = []; + const currentLocalFiles = collectLocalSyncFiles(config, { + ownerFilter: true, + warnings: ownerScopedWarnings + }); const currentLocalMap = Object.fromEntries( currentLocalFiles.map((file: { remoteRelativePath: string; content: string }) => [ file.remoteRelativePath, @@ -70,10 +74,24 @@ async function performPush(config: PushConfig, options: PushOptions) { const queuedSnapshots = stateStore.listQueuedSnapshots(); const snapshots = [ + // Fix-Runde MEDIUM finding #3 (05-review-findings.md, agent-tasks + // 06d09cde): a snapshot enqueued BEFORE this machine's profile picked up + // ownerScoped:true (or before this fix shipped at all) can still carry a + // peer's ownerScoped file in its stored localFiles/baseFiles — it was + // captured verbatim from an older, unfiltered collectLocalSyncFiles/ + // readBaseSnapshots() call. Replaying it verbatim would re-introduce + // exactly the echo Fix 2/D-002-D-004 closed for the "current" snapshot, + // just via the queue instead of a live collection. Route both maps + // through the same filterOwnerScopedBaseMap used for currentBaseMap + // below so a stale queued peer file is stripped here too, not just on + // freshly collected snapshots. The `as Record` cast is + // safe: filterOwnerScopedBaseMap only ever drops keys, it never turns an + // existing string value into null, and localFiles never held null values + // to begin with. ...queuedSnapshots.map((entry: { id: string; data: { localFiles: Record; baseFiles: Record } }) => ({ id: entry.id, - localFiles: entry.data.localFiles, - baseFiles: entry.data.baseFiles, + localFiles: filterOwnerScopedBaseMap(config, entry.data.localFiles) as Record, + baseFiles: filterOwnerScopedBaseMap(config, entry.data.baseFiles), message: `sync(queue): replay ${entry.id}` })), { @@ -93,30 +111,36 @@ async function performPush(config: PushConfig, options: PushOptions) { if (options.dryRun) { if (!reachability.reachable) { - return { - kind: "push", - status: "dry-run", - remoteHeadBefore: null, - remoteHeadAfter: null, - appliedFiles: unique(Object.keys(snapshots[snapshots.length - 1]?.localFiles || {})), - mergedFiles: [], - conflictFiles: [], - queuedSnapshotId: null, - notes: [ - `remote unreachable (${reachability.reason}); this run would enqueue a snapshot instead of pushing immediately` - ] - }; + return appendNotes( + { + kind: "push", + status: "dry-run", + remoteHeadBefore: null, + remoteHeadAfter: null, + appliedFiles: unique(Object.keys(snapshots[snapshots.length - 1]?.localFiles || {})), + mergedFiles: [], + conflictFiles: [], + queuedSnapshotId: null, + notes: [ + `remote unreachable (${reachability.reason}); this run would enqueue a snapshot instead of pushing immediately` + ] + }, + ownerScopedWarnings + ); } - return previewPush(config, snapshots); + return appendNotes(previewPush(config, snapshots), ownerScopedWarnings); } if (!reachability.reachable) { - return enqueueCurrentSnapshot( - stateStore, - currentLocalMap, - currentBaseMap, - `remote unreachable (${reachability.reason}); stored the current local snapshot for replay on the next successful run` + return appendNotes( + enqueueCurrentSnapshot( + stateStore, + currentLocalMap, + currentBaseMap, + `remote unreachable (${reachability.reason}); stored the current local snapshot for replay on the next successful run` + ), + ownerScopedWarnings ); } @@ -157,17 +181,20 @@ async function performPush(config: PushConfig, options: PushOptions) { stateStore.removeQueuedSnapshot(queuedSnapshot.id); } - return { - kind: "push", - status: "applied", - remoteHeadBefore: workingCopy.remoteHead, - remoteHeadAfter, - appliedFiles: unique(appliedFiles), - mergedFiles: unique(mergedFiles), - conflictFiles: unique(conflictFiles), - queuedSnapshotId, - notes: queuedSnapshots.length > 0 ? [`replayed ${queuedSnapshots.length} queued snapshot(s)`] : [] - }; + return appendNotes( + { + kind: "push", + status: "applied", + remoteHeadBefore: workingCopy.remoteHead, + remoteHeadAfter, + appliedFiles: unique(appliedFiles), + mergedFiles: unique(mergedFiles), + conflictFiles: unique(conflictFiles), + queuedSnapshotId, + notes: queuedSnapshots.length > 0 ? [`replayed ${queuedSnapshots.length} queued snapshot(s)`] : [] + }, + ownerScopedWarnings + ); } catch (error) { // Only a RemoteUnavailableError (thrown exclusively from // GitClient.lookupRemoteHead and GitClient.push — see errors.ts) is @@ -186,15 +213,34 @@ async function performPush(config: PushConfig, options: PushOptions) { throw error; } - return enqueueCurrentSnapshot( - stateStore, - currentLocalMap, - currentBaseMap, - "remote unavailable; stored the current local snapshot for replay on the next successful run" + return appendNotes( + enqueueCurrentSnapshot( + stateStore, + currentLocalMap, + currentBaseMap, + "remote unavailable; stored the current local snapshot for replay on the next successful run" + ), + ownerScopedWarnings ); } } +// Fix-Runde HIGH finding (05-review-findings.md, agent-tasks 06d09cde): +// merges collectLocalSyncFiles' ownerScoped "own file missing among peer +// files" warnings (see config.ts's CollectLocalSyncFilesResult.warnings) +// into whatever `notes` array a given result already carries, on every +// return path below — dry-run, queued (both the reachability-precheck skip +// and the catch-all git-failure fallback), and a real applied push all still +// need to surface the warning, since it describes THIS machine's local +// collection state, independent of whether the push itself succeeded. +function appendNotes(result: T, extraNotes: string[]): T { + if (extraNotes.length === 0) { + return result; + } + + return { ...result, notes: [...(result.notes || []), ...extraNotes] }; +} + // Shared by the reachability-precheck skip path and the catch-all fallback // below: stash the current local state as a new queued snapshot (existing // queued snapshots are left untouched — they are only cleared after a diff --git a/packages/agent-memory-sync/tests/integration/owner-scoped-push.test.ts b/packages/agent-memory-sync/tests/integration/owner-scoped-push.test.ts index 7d19751..4366d59 100644 --- a/packages/agent-memory-sync/tests/integration/owner-scoped-push.test.ts +++ b/packages/agent-memory-sync/tests/integration/owner-scoped-push.test.ts @@ -24,6 +24,7 @@ // staleness. const test = require("node:test"); const assert = require("node:assert/strict"); +const { mkdirSync } = require("node:fs"); const path = require("node:path"); const { cloneRemote, @@ -35,6 +36,7 @@ const { writeProjectConfig, writeText } = require("../helpers/cli.ts"); +const { StateStore } = require("../../src/memory-sync/state-store"); function ownerScopedConfig( workspaceRoot: string, @@ -141,6 +143,15 @@ test( !pushBPayload.conflictFiles.includes("machine-state/machine-a.json"), `B must not even attempt a merge over A's file: ${JSON.stringify(pushBPayload.conflictFiles)}` ); + // Negative control for the Fix-Runde HIGH finding's warning (05-review- + // findings.md, agent-tasks 06d09cde): B's own file (machine-b.json) IS + // present alongside A's stale copy, so no "own file not found" warning + // should fire here — the warning is for the missing-own-file case only, + // see the dedicated tests below. + assert.ok( + !(pushBPayload.notes || []).some((note: string) => note.includes("own file")), + `negative control: B's own file is present, no own-file-missing warning expected: ${JSON.stringify(pushBPayload.notes)}` + ); // The remote must still hold A's latest content — not overwritten by // B's stale echo — and must now also carry B's own file. @@ -154,7 +165,17 @@ test( } ); -test("push tolerates an ownerScoped directory whose own .json does not exist locally yet", () => { +// Fix-Runde HIGH finding (05-review-findings.md, agent-tasks 06d09cde): the +// original version of this test (title unchanged below, semantics extended) +// pinned tolerance — no exception — for a missing own file. It did NOT pin +// that this is a real, silent data-loss path whenever `config.profile` +// resolves to something other than this machine's actual owner filename +// (e.g. the CLI's [profile] positional defaulting to 'default' when a real +// machine invocation omits it — run.ts/loader.ts's override order). The +// tolerance (no exception) is preserved unchanged; a visible warning is now +// also asserted, per D-007 (03-decisions.md): fix the silence, not the CLI +// resolution semantics. +test("push tolerates an ownerScoped directory whose own .json does not exist locally yet, and now emits a visible warning when peer files are present", () => { const root = createSandbox("owner-scoped-missing-own-file"); const remoteDir = initBareRemote(root); const workspaceRoot = path.join(root, "workspace"); @@ -164,14 +185,14 @@ test("push tolerates an ownerScoped directory whose own .json does not writeText(path.join(workspaceRoot, "MEMORY.md"), "seed\n"); // machineStateSource exists (so the directory-existence check passes) but - // has no .json in it yet. + // has no .json in it yet — only a peer's file. writeText(path.join(machineStateSource, "someone-elses.json"), '{"v":1}\n'); writeProjectConfig(configPath, ownerScopedConfig(workspaceRoot, remoteDir, stateDir, "this-machine", machineStateSource)); const result = runCli(["run", "this-machine", "--config", configPath, "--mode", "push", "--output", "json"]); const payload = JSON.parse(result.stdout).runs[0]; - assert.equal(payload.status, "applied"); + assert.equal(payload.status, "applied", "tolerance preserved: no exception, still applies the rest of the push"); assert.ok(payload.appliedFiles.some((f: string) => f.endsWith("MEMORY.md"))); assert.ok( !payload.appliedFiles.some((f: string) => f.startsWith("machine-state/")), @@ -180,4 +201,163 @@ test("push tolerates an ownerScoped directory whose own .json does not const inspection = cloneRemote(remoteDir, root, "inspect-no-own-file"); assert.equal(fileExists(path.join(inspection, "shared", "machine-state", "someone-elses.json")), false); + + const notesText = (payload.notes || []).join(" "); + assert.match( + notesText, + /own file 'this-machine\.json' not found among 1 file\(s\)/, + `expected the own-file-missing warning naming the profile's owner filename and peer count: ${JSON.stringify(payload.notes)}` + ); + assert.match(notesText, /this machine will publish no 'machine-state' state/); + assert.match(notesText, /check the profile positional matches this machine/); }); + +// Negative/tolerance companion: a directory that exists but has genuinely NO +// files at all (not even a peer's) must stay silent — there is nothing to +// warn about, this is the ordinary brand-new-machine first-run shape. +test("push stays silent (no warning) for an ownerScoped directory that exists but is completely empty", () => { + const root = createSandbox("owner-scoped-empty-dir"); + const remoteDir = initBareRemote(root); + const workspaceRoot = path.join(root, "workspace"); + const stateDir = path.join(root, "state"); + const configPath = path.join(root, "config.json"); + const machineStateSource = path.join(root, "harness-state"); + + writeText(path.join(workspaceRoot, "MEMORY.md"), "seed\n"); + mkdirSync(machineStateSource, { recursive: true }); + writeProjectConfig(configPath, ownerScopedConfig(workspaceRoot, remoteDir, stateDir, "this-machine", machineStateSource)); + + const result = runCli(["run", "this-machine", "--config", configPath, "--mode", "push", "--output", "json"]); + const payload = JSON.parse(result.stdout).runs[0]; + + assert.equal(payload.status, "applied"); + assert.ok( + !payload.appliedFiles.some((f: string) => f.startsWith("machine-state/")), + `empty directory — nothing under machine-state/ should be offered: ${JSON.stringify(payload.appliedFiles)}` + ); + assert.ok( + !(payload.notes || []).some((note: string) => note.includes("own file")), + `an empty directory has nothing to warn about: ${JSON.stringify(payload.notes)}` + ); +}); + +// ─── Fix 3 (MEDIUM): queue replay must apply the same owner filter ───────── +// +// push.ts's currentLocalMap/currentBaseMap (the "current" snapshot) are +// filtered via collectLocalSyncFiles' ownerFilter + filterOwnerScopedBaseMap +// — but a snapshot enqueued BEFORE this machine's profile picked up +// ownerScoped:true (or before this fix shipped) is stored verbatim and, pre +// Fix 3, was replayed verbatim too: push.ts:71-85 mapped queuedSnapshots +// straight from entry.data.localFiles/baseFiles without going through +// filterOwnerScopedBaseMap. These two tests bypass a real push's collection +// step entirely and enqueue a stale snapshot directly via StateStore, which +// is the only way to reproduce a pre-fix/pre-deploy queue entry +// deterministically. +test("push replay strips a stale queued snapshot's peer ownerScoped file out of localFiles, so it is never re-offered", () => { + const root = createSandbox("owner-scoped-queue-local-leak"); + const remoteDir = initBareRemote(root); + const workspaceRoot = path.join(root, "workspace"); + const stateDir = path.join(root, "state"); + const configPath = path.join(root, "config.json"); + const machineStateSource = path.join(root, "harness-state"); + + writeText(path.join(workspaceRoot, "MEMORY.md"), "b's memory\n"); + writeText(path.join(machineStateSource, "machine-b.json"), '{"v":"b-own"}\n'); + writeProjectConfig( + configPath, + ownerScopedConfig(workspaceRoot, remoteDir, stateDir, "machine-b", machineStateSource) + ); + + // Directly enqueue a stale snapshot carrying a peer's ownerScoped file in + // localFiles, simulating a pre-fix/pre-deploy capture. + const stateStore = new StateStore(stateDir, "machine-b"); + stateStore.enqueueSnapshot({ + localFiles: { "machine-state/machine-a.json": '{"v":"stale-peer-local"}\n' }, + baseFiles: {} + }); + + const result = runCli(["run", "machine-b", "--config", configPath, "--mode", "push", "--output", "json"]); + const payload = JSON.parse(result.stdout).runs[0]; + + assert.equal(payload.status, "applied"); + assert.match((payload.notes || []).join(" "), /replayed 1 queued snapshot/); + assert.ok( + payload.appliedFiles.includes("machine-state/machine-b.json"), + `own file must still be pushed: ${JSON.stringify(payload.appliedFiles)}` + ); + assert.ok( + !payload.appliedFiles.includes("machine-state/machine-a.json"), + `stale queued peer file must NOT be replayed/offered: ${JSON.stringify(payload.appliedFiles)}` + ); + + const inspection = cloneRemote(remoteDir, root, "inspect-queue-local-leak"); + assert.equal(fileExists(path.join(inspection, "shared", "machine-state", "machine-a.json")), false); +}); + +test( + "push replay strips a stale queued snapshot's peer ownerScoped file out of baseFiles too, preventing a " + + "spurious conflict-marker corruption of a file this machine never touched", + () => { + const root = createSandbox("owner-scoped-queue-base-leak"); + const remoteDir = initBareRemote(root); + + // Seed the remote with A's real, current machine-state file via an + // ordinary push from A's own profile first. + const workspaceA = path.join(root, "workspace-a"); + const stateDirA = path.join(root, "state-a"); + const configPathA = path.join(root, "config-a.json"); + const machineStateSourceA = path.join(root, "machine-a-harness-state"); + writeText(path.join(workspaceA, "MEMORY.md"), "a's memory\n"); + writeText(path.join(machineStateSourceA, "machine-a.json"), '{"v":"a-real-content"}\n'); + writeProjectConfig( + configPathA, + ownerScopedConfig(workspaceA, remoteDir, stateDirA, "machine-a", machineStateSourceA) + ); + const pushA = runCli(["run", "machine-a", "--config", configPathA, "--mode", "push", "--output", "json"]); + assert.equal(JSON.parse(pushA.stdout).runs[0].status, "applied"); + + // Machine B never pulled/touched A's file. A stale queued snapshot + // carries A's file ONLY in baseFiles (not localFiles) — the shape that, + // pre-fix, drove mergeText's genuine-conflict fallback (base non-null, + // local null, remote A's real content) and would have written a marker + // block combining an empty local half with A's real remote content, + // corrupting a file B never touched (see config.ts's + // filterOwnerScopedBaseMap comment for the mechanics). + const workspaceB = path.join(root, "workspace-b"); + const stateDirB = path.join(root, "state-b"); + const configPathB = path.join(root, "config-b.json"); + const machineStateSourceB = path.join(root, "machine-b-harness-state"); + writeText(path.join(workspaceB, "MEMORY.md"), "b's memory\n"); + writeText(path.join(machineStateSourceB, "machine-b.json"), '{"v":"b-own"}\n'); + writeProjectConfig( + configPathB, + ownerScopedConfig(workspaceB, remoteDir, stateDirB, "machine-b", machineStateSourceB) + ); + + const stateStoreB = new StateStore(stateDirB, "machine-b"); + stateStoreB.enqueueSnapshot({ + localFiles: {}, + baseFiles: { "machine-state/machine-a.json": '{"v":"stale-base-snapshot"}\n' } + }); + + const pushB = runCli(["run", "machine-b", "--config", configPathB, "--mode", "push", "--output", "json"]); + const pushBPayload = JSON.parse(pushB.stdout).runs[0]; + + assert.equal(pushBPayload.status, "applied"); + assert.ok( + !pushBPayload.appliedFiles.includes("machine-state/machine-a.json"), + `B must not touch A's file at all: ${JSON.stringify(pushBPayload.appliedFiles)}` + ); + assert.ok( + !pushBPayload.conflictFiles.includes("machine-state/machine-a.json"), + `B must not spuriously conflict over A's file: ${JSON.stringify(pushBPayload.conflictFiles)}` + ); + + const inspection = cloneRemote(remoteDir, root, "inspect-queue-base-leak"); + assert.equal( + readText(path.join(inspection, "shared", "machine-state", "machine-a.json")), + '{"v":"a-real-content"}\n', + "A's remote content must survive completely untouched — no marker corruption from B's stale queued base entry" + ); + } +); diff --git a/packages/agent-memory-sync/tests/unit/merge.test.ts b/packages/agent-memory-sync/tests/unit/merge.test.ts index f387937..3832571 100644 --- a/packages/agent-memory-sync/tests/unit/merge.test.ts +++ b/packages/agent-memory-sync/tests/unit/merge.test.ts @@ -15,6 +15,20 @@ // returned content carries markers, while leaving the actual designed // single-pass conflict fallback (marker construction, already // conflict:true) and the untouched `unchanged`/strategy paths alone. +// +// Fix-Runde (05-review-findings.md, agent-tasks 06d09cde): two follow-up +// clusters added below. +// Fix 2 (MEDIUM, "Markdown-Ausschluss"): the first cut of +// hasConflictMarkers also matched a bare `=======` / `>>>>>>> ` line, which +// false-positives on ordinary Markdown (setext H1 underlines, `====` +// dividers, deep blockquotes) that legitimately starts a line that way — +// spurious conflict:true on content that was never actually corrupted. +// Narrowed to the unambiguous `<<<<<<< ` opener alone (mergeText always +// writes the full three-line block together, so the opener is sufficient +// and does not need corroboration from the other two lines). +// Fix 4 (LOW, "wins-Honesty"): the local-wins/remote-wins strategy +// branches still returned unconditional conflict:false, breaking the same +// invariant on an unreachable-today-but-still-public code path. const test = require("node:test"); const assert = require("node:assert/strict"); const { hasConflictMarkers, mergeText } = require("../../src/memory-sync/merge"); @@ -33,12 +47,47 @@ test("hasConflictMarkers: detects a '<<<<<<< local' line", () => { assert.equal(hasConflictMarkers("before\n<<<<<<< local\nafter\n"), true); }); -test("hasConflictMarkers: detects a '=======' line", () => { - assert.equal(hasConflictMarkers("before\n=======\nafter\n"), true); +// Fix 2 (MEDIUM, Markdown-Ausschluss): a bare '=======' or '>>>>>>> ' line, +// with no accompanying '<<<<<<< ' opener anywhere in the content, is NOT a +// conflict marker on its own — it's what a setext H1 underline, an +// '===='-style section divider, or a deeply nested blockquote look like in +// ordinary agent-memory Markdown. The pre-fix implementation matched these +// two line prefixes unconditionally and produced spurious conflict:true on +// such content. These two cases replace what used to assert `true` here. +test("hasConflictMarkers: a lone '=======' line with no '<<<<<<< ' opener (e.g. a setext H1 underline) is NOT a conflict marker", () => { + assert.equal(hasConflictMarkers("before\n=======\nafter\n"), false); +}); + +test("hasConflictMarkers: a lone '>>>>>>> ' line with no '<<<<<<< ' opener (e.g. a deep blockquote) is NOT a conflict marker", () => { + assert.equal(hasConflictMarkers("before\n>>>>>>> remote\nafter\n"), false); +}); + +test("hasConflictMarkers: a setext-style H1 underline in ordinary memory Markdown is not flagged", () => { + assert.equal(hasConflictMarkers("Overview\n=======\nnotes"), false); +}); + +test("hasConflictMarkers: an '====' style section divider (no opener anywhere) is not flagged", () => { + assert.equal(hasConflictMarkers("Section one\n\n====\n\nSection two\n"), false); }); -test("hasConflictMarkers: detects a '>>>>>>> remote' line", () => { - assert.equal(hasConflictMarkers("before\n>>>>>>> remote\nafter\n"), true); +test("hasConflictMarkers: a deeply nested blockquote line starting with '>>>>>>> ' (no opener anywhere) is not flagged", () => { + assert.equal(hasConflictMarkers("some reply\n>>>>>>> quoted from someone\nmore text\n"), false); +}); + +test("hasConflictMarkers: a real inherited conflict block (opener present) is still flagged true even amid lone '=======' /'>>>>>>> '-style Markdown elsewhere", () => { + const content = [ + "Notes", + "=======", + "<<<<<<< local", + "local content", + "=======", + "remote content", + ">>>>>>> remote", + "", + "quoted reply:", + ">>>>>>> someone else" + ].join("\n"); + assert.equal(hasConflictMarkers(content), true); }); test("hasConflictMarkers: a marker substring that is not at the start of a line does not count (line-anchored)", () => { @@ -129,3 +178,37 @@ test("mergeText: genuine single-pass conflict (no clean fast path, no append mer assert.match(result.content, /remote replaced/); assert.match(result.content, />>>>>>> remote/); }); + +// ─── Fix 4: local-wins/remote-wins strategy branches upgrade too ──────────── +// +// Unreachable via the deployed inline-markers conflictStrategy, but +// local-wins/remote-wins are still public MergeInput.strategy values; both +// branches previously returned unconditional conflict:false even when their +// picked winner already carried inherited markers, breaking the same +// honesty invariant the fast paths and appendOnly path above were fixed for. + +test("mergeText: local-wins strategy is upgraded to conflict:true when the local winner already carries markers", () => { + const markerLocal = ["<<<<<<< local", "stale local", "=======", "stale remote", ">>>>>>> remote"].join("\n"); + const result = mergeText({ + base: "base\n", + local: markerLocal, + remote: "remote replaced\n", + strategy: "local-wins" + }); + assert.equal(result.status, "conflict"); + assert.equal(result.content, markerLocal, "payload must not be rewritten, only the conflict flag"); + assert.equal(result.conflict, true); +}); + +test("mergeText: remote-wins strategy is upgraded to conflict:true when the remote winner already carries markers", () => { + const markerRemote = ["<<<<<<< local", "stale local", "=======", "stale remote", ">>>>>>> remote"].join("\n"); + const result = mergeText({ + base: "base\n", + local: "local replaced\n", + remote: markerRemote, + strategy: "remote-wins" + }); + assert.equal(result.status, "conflict"); + assert.equal(result.content, markerRemote, "payload must not be rewritten, only the conflict flag"); + assert.equal(result.conflict, true); +});