From 743f747f52599d45ebaa3a5dce452e6295dd1da9 Mon Sep 17 00:00:00 2001 From: Lan Nguyen Si Date: Mon, 27 Jul 2026 07:59:11 +0700 Subject: [PATCH 1/2] feat(memory-sync): warn on unparsable reachability override, resolve short shas Three follow-ups from the activation run (PR #63). AGENT_MEMORY_SYNC_REACHABILITY_CHECK_COMMAND had two silent failure modes. `false` parsed as a JSON boolean, then fell through a falsy check and returned null, so the default ssh probe ran while the operator believed they had disabled it. A genuinely malformed value threw an uncaught SyntaxError that took the whole CLI down. Both now print one visible warning naming the offending value and the expected shape, and fall back to the default probe. `restore ` failed with "could not fetch ref", because git only serves full object names to remotes. Restore now resolves locally first via git rev-parse; the working copy already fetches full branch history, so the common case costs no extra round trip. When a short-looking sha stays unresolved both locally and remotely, the error says to use the full 40-character sha. The watch offline behaviour needed no production change: since PR #65 watch goes through performPush, which already commits locally and skips the push with one log line when the remote is unreachable, while genuine config and data errors still crash loud for the supervisor. Only the fail-loud half was untested and the watch plist template still documented the opposite. Both fixed. Note for both machines: rebuild after merge, and restart the watch and sync launchd services so they pick up the loader change. Refs: 1b63070d-9ea1-4a38-bba0-e58a4678b596 --- packages/agent-memory-sync/README.md | 8 +- ...com.agent-memory-sync.watch.plist.template | 41 ++++--- .../agent-memory-sync/docs/machine-setup.md | 11 +- .../agent-memory-sync/src/commands/restore.ts | 9 +- .../agent-memory-sync/src/config/loader.ts | 53 ++++++++- .../src/memory-sync/git-client.ts | 34 +++++- .../integration/watch-mirror-delete.test.ts | 40 +++++++ .../tests/integration/watch-restore.test.ts | 74 +++++++++++++ .../reachability-check-command-env.test.ts | 101 ++++++++++++++++++ 9 files changed, 346 insertions(+), 25 deletions(-) create mode 100644 packages/agent-memory-sync/tests/unit/reachability-check-command-env.test.ts diff --git a/packages/agent-memory-sync/README.md b/packages/agent-memory-sync/README.md index 9d7f89b..5099646 100644 --- a/packages/agent-memory-sync/README.md +++ b/packages/agent-memory-sync/README.md @@ -195,7 +195,7 @@ Options: --help ``` -A full-tree restore requires `--yes` (or `--dry-run` to preview); a single file via `--path MEMORY.md` does not. Files are written byte-identical to their contents at ``. The command refuses to map a remote path that does not match an entry in `syncPaths`, so a restore cannot scatter files outside the configured workspace. An unknown SHA or a path that did not exist at that commit fails loudly. +A full-tree restore requires `--yes` (or `--dry-run` to preview); a single file via `--path MEMORY.md` does not. Files are written byte-identical to their contents at ``. The command refuses to map a remote path that does not match an entry in `syncPaths`, so a restore cannot scatter files outside the configured workspace. An unknown SHA or a path that did not exist at that commit fails loudly. `` may be abbreviated as long as the commit is reachable from the configured branch — it resolves locally against the branch history the command already fetches, no extra network round-trip; a short sha that is not reachable that way fails loudly with an explicit "use the full 40-character sha" message, since a plain `git fetch ` only ever accepts a full object id from a remote. ```bash # Roll back MEMORY.md to a specific commit @@ -290,7 +290,11 @@ Priority order (highest to lowest): CLI flags > environment variables > config f is left untouched. Tune the timeout with `--reachability-timeout-ms` / `reachabilityTimeoutMs` (default 4000ms), or fully override the probe with `reachabilityCheckCommand` (an argv array; config file / `AGENT_MEMORY_SYNC_REACHABILITY_CHECK_COMMAND` only, no CLI flag — same pattern - as `syncPaths`) + as `syncPaths`). The env form must be a JSON array of non-empty strings (e.g. + `["ssh","-o","BatchMode=yes","host","true"]`); a value that fails to parse that way (invalid + JSON, or valid JSON of the wrong shape — a bare `false`, a string, an object, ...) prints a + visible warning naming the offending value and falls back to the default probe, instead of + either crashing the CLI or silently substituting the default with no explanation - failed pushes (including ones skipped by the reachability precheck) are queued locally in `stateDir/queue` and replayed on the next successful push - append-only concurrent edits are merged automatically; other conflicts default to inline conflict markers diff --git a/packages/agent-memory-sync/docs/launchd/com.agent-memory-sync.watch.plist.template b/packages/agent-memory-sync/docs/launchd/com.agent-memory-sync.watch.plist.template index 3803ebd..0dccf3a 100644 --- a/packages/agent-memory-sync/docs/launchd/com.agent-memory-sync.watch.plist.template +++ b/packages/agent-memory-sync/docs/launchd/com.agent-memory-sync.watch.plist.template @@ -46,22 +46,33 @@ the log paths below) KeepAlive / ThrottleInterval, and why: - `watch` does NOT use the reachability precheck (src/memory-sync/ - reachability.ts) — that precheck only covers `run`'s pull/push/sync. Any - network failure during `watch` — the mini being unreachable, an expired - credential, a rejected push, a filesystem error — surfaces as a - non-zero exit "by design" (see README.md #systemd-unit) with no - precheck softening it. KeepAlive's SuccessfulExit=false mirrors that: - launchd respawns on a crash (non-zero exit) but does not respawn after a - clean shutdown. ThrottleInterval caps the respawn rate so a - persistently failing cause (mini down, bad ssh key, permanently - rejected push) does not spin the CPU in a restart loop — this is - launchd's rough equivalent of systemd's StartLimitIntervalSec/ - StartLimitBurst pairing, just as a single minimum gap between attempts - rather than a burst counter. + `watch`'s push goes through the same base-snapshot-aware `performPush` + (src/memory-sync/push.ts) that `run`'s pull/push/sync use, so it gets + the same reachability precheck (src/memory-sync/reachability.ts): an + unreachable mini, or a push the remote rejects (auth, non-fast-forward, + network), is queued locally (stateDir/queue) and replayed on the next + successful tick or `run`, with a clean exit 0 — no crash, no respawn. + This is a deliberate contract change from watch's earlier behavior + (where any push failure, including a merely unreachable remote, exited + non-zero and relied on launchd to restart the process); see README.md's + `watch` section and docs/machine-setup.md for the exact boundary. A + genuine config/data error (e.g. a required `syncPaths` entry missing) + is the one remaining class of failure that still surfaces as a + non-zero exit "by design" (see README.md #systemd-unit) — that class is + what KeepAlive/ThrottleInterval below are sized for. KeepAlive's + SuccessfulExit=false mirrors that: launchd respawns on a crash + (non-zero exit) but does not respawn after a clean shutdown (including + the clean exit 0 the queued-offline case above now produces). + ThrottleInterval caps the respawn rate so a persistently broken config + does not spin the CPU in a restart loop — this is launchd's rough + equivalent of systemd's StartLimitIntervalSec/StartLimitBurst pairing, + just as a single minimum gap between attempts rather than a burst + counter. - Because `watch` alone gives no offline queueing here, pair this job - with the periodic-sync companion, + Offline edits still only reach the remote once this machine's own + `watch` next runs successfully (or the periodic sync companion below + replays the queue) — `watch` remains edge-triggered and does not pull, + so pair this job with the periodic-sync companion, com.agent-memory-sync.sync.plist.template — see its header and docs/machine-setup.md for why that companion is required, not optional. --> diff --git a/packages/agent-memory-sync/docs/machine-setup.md b/packages/agent-memory-sync/docs/machine-setup.md index 207cafe..4417e89 100644 --- a/packages/agent-memory-sync/docs/machine-setup.md +++ b/packages/agent-memory-sync/docs/machine-setup.md @@ -281,9 +281,14 @@ is `agent-memory-sync restore`: ```bash # Find a commit to roll back to (from any machine, or `ssh mini` + `git log` -# directly against the bare repo). Use the FULL 40-char sha: abbreviated -# shas cannot be fetched from a remote (git only serves full object names), -# so `restore ` fails with "could not fetch ref". +# directly against the bare repo). An abbreviated sha works too as long as +# the commit is reachable from the configured branch: `restore` resolves it +# locally against the branch history its working copy already fetched, no +# extra network round-trip needed. It only falls back to an explicit +# `git fetch origin ` (which only ever accepts a *full* object id from +# a remote) for a sha that history does not already contain — and if that +# still fails, the error says explicitly to use the full 40-char sha instead +# of guessing why a short one did not resolve. agent-memory-sync restore --config profiles/.json --dry-run # Roll back a single file — --path is relative to repositorySubdir, and diff --git a/packages/agent-memory-sync/src/commands/restore.ts b/packages/agent-memory-sync/src/commands/restore.ts index 0a40076..6bab818 100644 --- a/packages/agent-memory-sync/src/commands/restore.ts +++ b/packages/agent-memory-sync/src/commands/restore.ts @@ -89,7 +89,14 @@ function registerRestoreCommand(program: import("commander").Command): void { gitClient.createTempRepoDir(runConfig.stateDir, "restore") ); - gitClient.fetchRef(workingCopy.repoDir, sha); + // Skip the network-only fetchRef path entirely when `sha` (full or + // abbreviated) already resolves against objects prepareWorkingCopy + // just fetched (the whole branch history) — this is what makes an + // abbreviated sha work at all, since fetchRef's own `git fetch origin + // ` cannot resolve one against the remote (see git-client.ts). + if (!gitClient.resolveLocalCommit(workingCopy.repoDir, sha)) { + gitClient.fetchRef(workingCopy.repoDir, sha); + } const targetRepoPaths = options.path ? [normalizeRequestedPath(runConfig.repositorySubdir, options.path)] diff --git a/packages/agent-memory-sync/src/config/loader.ts b/packages/agent-memory-sync/src/config/loader.ts index 8ce3e29..1777c48 100644 --- a/packages/agent-memory-sync/src/config/loader.ts +++ b/packages/agent-memory-sync/src/config/loader.ts @@ -305,14 +305,61 @@ function readEnvConfig(): UserConfig { ); } if (env.AGENT_MEMORY_SYNC_REACHABILITY_CHECK_COMMAND) { - config.reachabilityCheckCommand = normalizeReachabilityCheckCommand( - JSON.parse(env.AGENT_MEMORY_SYNC_REACHABILITY_CHECK_COMMAND) as string[] - ); + // Both failure modes below (invalid JSON syntax, and valid JSON of the + // wrong shape — including a falsy JSON value like `false`, which would + // otherwise slip past normalizeReachabilityCheckCommand's `if (!value)` + // guard silently) are caught here and turned into one visible warning + // instead of either a silent no-op (the live incident: `=false` quietly + // ran the default ssh probe with no explanation) or an uncaught + // exception that would crash the whole CLI invocation over a value only + // ever consulted if the remote turns out unreachable. The override is + // left unset on failure, so config.reachabilityCheckCommand keeps + // falling through to the config file / default below, same as if the + // env var had not been set at all. + try { + const parsedValue = JSON.parse(env.AGENT_MEMORY_SYNC_REACHABILITY_CHECK_COMMAND); + // JSON.parse alone would happily accept any JSON value (a bare + // `false`, a number, a plain object, ...), and normalizeReachability + // CheckCommand's `if (!value) return null;` guard would then silently + // treat any *falsy* one of those (false, 0, "") as "not set" without + // ever reaching its own shape check below — this is exactly how the + // live incident's `=false` slipped through unnoticed. Requiring the + // parsed value to already be `null` or an array here, before handing + // off to the shared normalizer, closes that gap without changing + // normalizeReachabilityCheckCommand's behavior for its other callers + // (config file, `config set`), which is unrelated to this env-only fix. + if (parsedValue !== null && !Array.isArray(parsedValue)) { + throw new CliError( + `must be a JSON array of non-empty strings (argv form) or JSON null, got ${typeof parsedValue}.`, + 3 + ); + } + config.reachabilityCheckCommand = normalizeReachabilityCheckCommand(parsedValue as string[] | null); + } catch (error) { + warnUnparsableReachabilityCheckCommandEnv( + env.AGENT_MEMORY_SYNC_REACHABILITY_CHECK_COMMAND, + error + ); + } } return config; } +// Always printed (not gated by --quiet/--verbose, which are not resolved +// yet at this point in config loading anyway): a misconfigured reachability +// probe override silently substituting the default is exactly the failure +// mode this warning exists to surface, so it must not itself be silenceable. +function warnUnparsableReachabilityCheckCommandEnv(rawValue: string, error: unknown): void { + const reason = error instanceof Error ? error.message : String(error); + process.stderr.write( + `warning: ignoring env AGENT_MEMORY_SYNC_REACHABILITY_CHECK_COMMAND='${rawValue}' ` + + `(${reason}). Expected a JSON array of non-empty strings (argv form), e.g. ` + + `'["ssh","-o","BatchMode=yes","-o","ConnectTimeout=4","host","true"]'. Falling back to ` + + `the default reachability probe.\n` + ); +} + function normalizeUserConfig(raw: Record): UserConfig { const normalized: UserConfig = {}; const aliasMap: Record = { diff --git a/packages/agent-memory-sync/src/memory-sync/git-client.ts b/packages/agent-memory-sync/src/memory-sync/git-client.ts index b9b7023..b7b7725 100644 --- a/packages/agent-memory-sync/src/memory-sync/git-client.ts +++ b/packages/agent-memory-sync/src/memory-sync/git-client.ts @@ -138,11 +138,43 @@ class GitClient { return repoDir; } + // Resolves `ref` (full or abbreviated sha, branch/tag name) against + // objects the working copy already has locally — no network access. + // `restore`'s working copy is always prepared via prepareWorkingCopy(), + // which already ran a full `git fetch origin `, so every commit + // reachable from that branch tip (i.e. any commit `restore` could + // sensibly be asked for) is already present as a local object at this + // point, abbreviated shas included: git resolves an abbreviated ref + // against locally-known objects the same way regardless of how many + // characters were supplied. Returns null (rather than throwing) when the + // ref cannot be resolved locally, so callers can fall back to fetchRef. + resolveLocalCommit(repoDir: string, ref: string): string | null { + const result = this.run(["rev-parse", "--verify", "--quiet", `${ref}^{commit}`], repoDir, true); + if (result.exitCode !== 0) { + return null; + } + return result.stdout.trim() || null; + } + fetchRef(repoDir: string, ref: string): void { const result = this.run(["fetch", "origin", ref], repoDir, true); if (result.exitCode !== 0) { + // `git fetch ` only accepts a ref name or a *full* + // object id from the remote side — an abbreviated commit sha is + // never resolvable that way (unlike local ref resolution, which + // supports abbreviation freely). Surface that explicitly for a + // short-looking hex ref rather than leaving the reader to guess why + // it failed; see resolveLocalCommit above for the case (an + // abbreviated sha reachable from the configured branch) this + // fetchRef fallback path is not even reached for. + const looksLikeShortSha = ref.length < 40 && /^[0-9a-f]+$/i.test(ref); throw new CliError( - `could not fetch ref '${ref}' from remote. Verify the commit/branch exists and the remote is reachable.`, + `could not fetch ref '${ref}' from remote. Verify the commit/branch exists and the remote is reachable.` + + (looksLikeShortSha + ? ` '${ref}' is not reachable from the configured branch's already-fetched history, and an ` + + `abbreviated commit sha cannot be fetched directly from a remote (git only serves full object ` + + `names that way) — use the full 40-character sha instead.` + : ""), 4 ); } diff --git a/packages/agent-memory-sync/tests/integration/watch-mirror-delete.test.ts b/packages/agent-memory-sync/tests/integration/watch-mirror-delete.test.ts index 8a0a670..1fa7955 100644 --- a/packages/agent-memory-sync/tests/integration/watch-mirror-delete.test.ts +++ b/packages/agent-memory-sync/tests/integration/watch-mirror-delete.test.ts @@ -271,6 +271,46 @@ test("watch tick queues locally when the remote is unreachable, then replays the assert.equal(readText(path.join(inspection, "shared", "logs", "trigger.md")), "trigger\n"); }); +// Counterpart to the queue-replay test above (agent-tasks +// 1b63070d-9ea1-4a38-bba0-e58a4678b596): the reachability precheck softens +// watch's push failure handling for one specific class of failure — the +// remote being unreachable — into a clean, non-throwing queue. It must not +// soften anything else. A genuine config/data error (README.md's own +// example: a required `syncPaths` entry missing) is raised by +// collectLocalSyncFiles() before performPush's reachability precheck or its +// try/catch are ever reached (see src/memory-sync/push.ts), so it still +// propagates all the way out to watch's handleSnapshotError and exits +// non-zero — preserving the supervisor-respawn semantics (launchd +// KeepAlive/ThrottleInterval, systemd's StartLimitIntervalSec/-Burst) that +// this class of failure relies on. The remote here is a real, reachable +// bare repo, isolating the assertion from reachability entirely. +test("watch tick with a missing required syncPaths entry still exits non-zero, independent of remote reachability", async () => { + const root = createSandbox("watch-required-syncpath-missing"); + const remoteDir = initBareRemote(root); + const workspaceRoot = path.join(root, "workspace"); + const configPath = path.join(root, "config.json"); + + writeText(path.join(workspaceRoot, "MEMORY.md"), "seed\n"); + writeProjectConfig(configPath, { + ...createConfig(workspaceRoot, remoteDir), + syncPaths: [ + { source: "MEMORY.md", destination: "MEMORY.md", kind: "file" }, + { source: "REQUIRED.md", destination: "REQUIRED.md", kind: "file", required: true } + ] + }); + + const { exitCode, stderr } = await runWatchTick(configPath, () => { + writeText(path.join(workspaceRoot, "MEMORY.md"), "seed\nlocal edit\n"); + }); + + assert.notEqual( + exitCode, + 0, + `watch was expected to crash loudly on a missing required sync path; it exited 0 instead. stderr: ${stderr}` + ); + assert.match(stderr, /required sync path/i); +}); + // Rework finding (LOW, review of this task): pins that a successful tick // updates stateDir/base to the post-merge remote state — the input the next // tick's 3-way merge relies on to correctly leave an unpulled peer file diff --git a/packages/agent-memory-sync/tests/integration/watch-restore.test.ts b/packages/agent-memory-sync/tests/integration/watch-restore.test.ts index 3e3f659..21cbc5a 100644 --- a/packages/agent-memory-sync/tests/integration/watch-restore.test.ts +++ b/packages/agent-memory-sync/tests/integration/watch-restore.test.ts @@ -319,4 +319,78 @@ test("restore rejects an unknown sha with a loud non-zero exit", () => { ); assert.notEqual(result.status, 0); assert.match(result.stderr, /fetch|deadbeef|not.+exist/i); + // Pins the AC-3 hint: `deadbeef` is short (8 hex chars), and unresolvable + // both locally and via an explicit remote fetch (see the short-sha tests + // below for the resolvable case), so the error should say outright that a + // full 40-character sha is required instead of leaving the reader to + // guess why a seemingly valid-looking hex string failed. + assert.match(result.stderr, /40.character sha/i); +}); + +// Reproduces the documented restore-short-sha failure (README.md's restore +// section, docs/machine-setup.md): `git fetch origin ` only accepts a +// ref name or a *full* object id from a remote — an abbreviated commit sha +// is never resolvable that way, so restore used to fail with a bare "could +// not fetch ref" even though the commit is right there in history. +// +// Fix: restore's working copy is prepared via prepareWorkingCopy(), which +// already runs a full `git fetch origin ` and so already has every +// commit reachable from that branch tip as a local object — the short sha +// is resolvable locally (no network round-trip needed) via `git rev-parse +// --verify` before ever attempting the remote-only fetchRef path. This +// pins the common case (restoring from an older commit on the configured +// branch, the only realistic use of `restore`) actually working with a +// short sha, not just a friendlier error message. +test("restore resolves against the already-fetched branch history and restores successfully", () => { + const root = createSandbox("restore-short-sha"); + const remoteDir = initBareRemote(root); + const workspaceRoot = path.join(root, "workspace"); + const configPath = path.join(root, "config.json"); + + writeText(path.join(workspaceRoot, "MEMORY.md"), "snapshot 1\n"); + writeProjectConfig(configPath, createConfig(workspaceRoot, remoteDir)); + + runCli(["run", "default", "--config", configPath, "--mode", "push", "--output", "json"]); + const inspection = cloneRemote(remoteDir, root, "rev1"); + const fullSha = git(["rev-parse", "HEAD"], inspection).trim(); + const shortSha = git(["rev-parse", "--short", "HEAD"], inspection).trim(); + assert.ok(shortSha.length < fullSha.length, "test fixture assumption: short sha must be abbreviated"); + + writeText(path.join(workspaceRoot, "MEMORY.md"), "snapshot 2\n"); + runCli(["run", "default", "--config", configPath, "--mode", "push", "--output", "json"]); + + const result = runCli([ + "restore", + shortSha, + "--config", + configPath, + "--path", + "MEMORY.md", + "--output", + "json" + ]); + const payload = JSON.parse(result.stdout); + assert.equal(payload.restored.length, 1); + assert.equal(readText(path.join(workspaceRoot, "MEMORY.md")), "snapshot 1\n"); +}); + +test("restore that is not reachable from the configured branch fails loudly with an explicit full-sha hint", () => { + const root = createSandbox("restore-short-sha-unreachable"); + const remoteDir = initBareRemote(root); + const workspaceRoot = path.join(root, "workspace"); + const configPath = path.join(root, "config.json"); + + writeText(path.join(workspaceRoot, "MEMORY.md"), "v1\n"); + writeProjectConfig(configPath, createConfig(workspaceRoot, remoteDir)); + runCli(["run", "default", "--config", configPath, "--mode", "push", "--output", "json"]); + + // A well-formed but short hex string that is not a prefix of any object + // reachable from the branch (nor known to the remote at all) — neither + // local resolution nor the fallback remote fetchRef can succeed. + const result = runCli( + ["restore", "0123abc", "--config", configPath, "--yes", "--output", "json"], + { expectFailure: true } + ); + assert.notEqual(result.status, 0); + assert.match(result.stderr, /40.character sha/i); }); diff --git a/packages/agent-memory-sync/tests/unit/reachability-check-command-env.test.ts b/packages/agent-memory-sync/tests/unit/reachability-check-command-env.test.ts new file mode 100644 index 0000000..62de0ce --- /dev/null +++ b/packages/agent-memory-sync/tests/unit/reachability-check-command-env.test.ts @@ -0,0 +1,101 @@ +// Unit tests for the AGENT_MEMORY_SYNC_REACHABILITY_CHECK_COMMAND env +// override's parsing. +// +// Incident (live, 2026-07-22, .ai/runs/2026-07-22-memory-sync-activation/ +// 05-review-findings.md "Delta-Review" section): setting this env var to +// `false` was silently ignored — the value is valid JSON (the boolean +// `false`), so JSON.parse succeeds, but normalizeReachabilityCheckCommand's +// `if (!value) return null;` guard treats any falsy parse result as "not +// set" and returns null with no diagnostic. A syntactically invalid value +// (not valid JSON at all) is worse: it throws an uncaught SyntaxError out of +// readEnvConfig that takes down the whole CLI invocation, not just the +// probe override — for a value the config only *uses* if the remote turns +// out to be unreachable. +// +// The fix: any value that fails to parse into a valid non-empty argv array +// (invalid JSON syntax, or valid JSON of the wrong shape) prints one visible +// warning naming the offending value and the expected format (a JSON array +// of non-empty strings), and the override is treated as unset — falling +// back to the default reachability probe — instead of crashing the CLI or +// silently substituting the default with no explanation. +// +// Hermetic: exercises src/config/loader.ts's resolveRunConfig() directly +// (mirrors tests/unit/reachability.test.ts's style of testing the module +// directly rather than through the CLI), capturing process.stderr.write to +// assert on the warning without spawning a process. + +const test = require("node:test"); +const assert = require("node:assert/strict"); +const { loadConfig, resolveRunConfig } = require("../../src/config/loader"); + +const ENV_KEY = "AGENT_MEMORY_SYNC_REACHABILITY_CHECK_COMMAND"; + +async function resolveWithEnvValue(rawValue: string): Promise<{ + reachabilityCheckCommand: string[] | null; + stderr: string; +}> { + const previous = process.env[ENV_KEY]; + process.env[ENV_KEY] = rawValue; + + const originalWrite = process.stderr.write.bind(process.stderr); + let stderr = ""; + process.stderr.write = ((chunk: string | Uint8Array, ...rest: unknown[]) => { + stderr += typeof chunk === "string" ? chunk : Buffer.from(chunk).toString("utf8"); + return true; + }) as typeof process.stderr.write; + + try { + const loaded = await loadConfig("/nonexistent/agent-memory-sync-config.json"); + const runConfig = resolveRunConfig(loaded, { remoteUrl: "/tmp/does-not-matter.git" }); + return { reachabilityCheckCommand: runConfig.reachabilityCheckCommand, stderr }; + } finally { + process.stderr.write = originalWrite; + if (typeof previous === "undefined") { + delete process.env[ENV_KEY]; + } else { + process.env[ENV_KEY] = previous; + } + } +} + +test(`${ENV_KEY}='false' warns visibly and falls back to the default probe instead of silently substituting it`, async () => { + const { reachabilityCheckCommand, stderr } = await resolveWithEnvValue("false"); + + assert.equal(reachabilityCheckCommand, null, "an unparsable override must not silently apply"); + assert.match(stderr, /warning/i); + assert.match(stderr, new RegExp(ENV_KEY)); + assert.match(stderr, /false/); + assert.match(stderr, /json array/i); +}); + +test(`${ENV_KEY}='not valid json' warns visibly instead of crashing the whole CLI invocation`, async () => { + const { reachabilityCheckCommand, stderr } = await resolveWithEnvValue("not valid json"); + + assert.equal(reachabilityCheckCommand, null); + assert.match(stderr, /warning/i); + assert.match(stderr, /json array/i); +}); + +test(`${ENV_KEY}='["true","5","x"]' with non-array-of-strings shape (a JSON object) warns visibly`, async () => { + const { reachabilityCheckCommand, stderr } = await resolveWithEnvValue('{"not":"an array"}'); + + assert.equal(reachabilityCheckCommand, null); + assert.match(stderr, /warning/i); + assert.match(stderr, /json array/i); +}); + +test(`${ENV_KEY} with a valid JSON array of non-empty strings applies with no warning`, async () => { + const { reachabilityCheckCommand, stderr } = await resolveWithEnvValue( + '["ssh","-o","BatchMode=yes","host","true"]' + ); + + assert.deepEqual(reachabilityCheckCommand, ["ssh", "-o", "BatchMode=yes", "host", "true"]); + assert.doesNotMatch(stderr, /warning/i); +}); + +test(`${ENV_KEY}='[]' (explicit empty array) applies as null with no warning`, async () => { + const { reachabilityCheckCommand, stderr } = await resolveWithEnvValue("[]"); + + assert.equal(reachabilityCheckCommand, null); + assert.doesNotMatch(stderr, /warning/i); +}); From 86e9e3e5a8b72649fe457752d88e354fd4df78a4 Mon Sep 17 00:00:00 2001 From: Lan Nguyen Si Date: Mon, 27 Jul 2026 08:40:40 +0700 Subject: [PATCH 2/2] fix(memory-sync): only queue failures we attribute to the remote Review found the supervisor contract was asserted, not implemented. performPush's catch had no classification and no rethrow, so every error raised inside the try block became "remote unavailable; queued" with exit 0: ENOSPC and EACCES while writing the working copy, a failing commit, a broken git config, prepareWorkingCopy and StateStore failures alike. Under launchd that is the worst possible shape. A machine with a full disk logs a benign queued line every tick, exits 0, is never respawned by KeepAlive, and never syncs again. Invisible by construction. Adds RemoteUnavailableError, thrown from lookupRemoteHead and push. performPush rethrows anything else. Discriminating on exitCode was not viable: the generic git fallback also throws exitCode 4 for any failing subcommand, so the code does not carry the information. Matching on message text was rejected as revert-sensitive. Note precisely what the boundary is, since it is a choice and not a discovery. prepareWorkingCopy also fetches, and that call is equally remote-attributable in principle. It is deliberately NOT attributed, because a fetch failure can just as easily be local, a full disk writing the packfile. So a transient drop in the window after ls-remote succeeds now crashes rather than queues. That is bounded: a sustained outage never reaches the fetch, because the reachability precheck queues first, and schemes the precheck waves through are still caught by ls-remote. It fails loudly and restartably, which is the safe direction. The boundary test uses a stub git binary that forwards everything to real git except commit, which exits 17, so a non-network failure is raised inside the try window after the working copy is already prepared against a reachable remote. Confirmed red before this change: exit 0 with the false "remote unavailable" note. Also from review: the docs named a class ("a config/data error") that was only true of one example, so README, machine-setup and the watch plist header now name the mechanism instead. restore captures the resolved full sha rather than discarding it, so --output json reports the commit actually restored from instead of the abbreviation typed. resolveLocalCommit no longer advertises branch and tag input it was never safe for; the hex precondition enforced in restore is now stated at the method. A test title that contradicted its body is fixed, with the element-shape case it hinted at added, plus a test pinning empty-as-unset. The README documents that convention and, answering the original incident, how to actually disable the probe. Note for both machines: rebuild and restart the watch and sync services after merge. Refs: 1b63070d-9ea1-4a38-bba0-e58a4678b596 --- packages/agent-memory-sync/README.md | 24 +++- ...com.agent-memory-sync.watch.plist.template | 32 +++--- .../agent-memory-sync/docs/machine-setup.md | 13 ++- .../agent-memory-sync/src/commands/restore.ts | 19 +++- packages/agent-memory-sync/src/errors.ts | 22 ++++ .../src/memory-sync/git-client.ts | 35 ++++-- .../agent-memory-sync/src/memory-sync/push.ts | 18 +++ .../integration/watch-mirror-delete.test.ts | 107 ++++++++++++++++-- .../tests/integration/watch-restore.test.ts | 6 + .../reachability-check-command-env.test.ts | 45 +++++++- 10 files changed, 274 insertions(+), 47 deletions(-) diff --git a/packages/agent-memory-sync/README.md b/packages/agent-memory-sync/README.md index 5099646..61972f6 100644 --- a/packages/agent-memory-sync/README.md +++ b/packages/agent-memory-sync/README.md @@ -138,7 +138,7 @@ StartLimitBurst=10 WantedBy=multi-user.target ``` -The `StartLimitIntervalSec` / `StartLimitBurst` pair caps systemd's restart loop for the class of failure that still exits non-zero — a config/data error such as a required `syncPaths` entry missing — so a persistently broken config does not crashloop forever; a network hiccup or a rejected push no longer exits at all (see below), so it never spends this budget. Inspect `journalctl -u agent-memory-sync-watch.service` for the `snapshot push failed: ...` line `watch` writes to stderr before exiting on that remaining class of failure. +The `StartLimitIntervalSec` / `StartLimitBurst` pair caps systemd's restart loop for the failures that still exit non-zero — a config/data error raised before the remote working copy is prepared (e.g. a required `syncPaths` entry missing), or any other git-level failure while preparing/committing that working copy (a full disk, a corrupted git config, a broken commit hook, ...) — so a persistently broken cause does not crashloop forever; a remote that is merely unreachable or rejecting the push (see below) no longer exits at all, so it never spends this budget. Inspect `journalctl -u agent-memory-sync-watch.service` for the `snapshot push failed: ...` line `watch` writes to stderr before exiting on one of those failures. macOS equivalent (LaunchAgent instead of systemd): see [`docs/launchd/com.agent-memory-sync.watch.plist.template`](docs/launchd/com.agent-memory-sync.watch.plist.template) @@ -152,10 +152,16 @@ remote rejects (auth, non-fast-forward, network), is queued locally (`stateDir/queue`) and replayed on the next successful `watch` tick or `run`, with a clean exit `0`. This is a deliberate contract change from an earlier version of `watch`, where any push failure exited non-zero and -relied on launchd/systemd to restart the process; a genuine config/data -error (e.g. a required `syncPaths` entry missing) is the only class of -failure that still exits non-zero and reaches the supervisor-restart path -described above. +relied on launchd/systemd to restart the process. The queue-instead-of-crash +handling is narrow, not a general catch-all: only a failure that +`GitClient.lookupRemoteHead` / `GitClient.push` attributes to the remote +itself (unreachable, rejected, non-fast-forward — see `RemoteUnavailableError` +in `src/errors.ts`) is queued. Every other failure still exits non-zero and +reaches the supervisor-restart path described above — a config/data error +raised before the remote working copy is even prepared (e.g. a required +`syncPaths` entry missing), and any other git-level failure while that +working copy is being prepared or committed (a full disk, a corrupted git +config, a broken commit hook, ...). `watch` still never pulls — it is edge-triggered on local changes only, so a machine that was offline while changes landed elsewhere will not pick them @@ -294,7 +300,13 @@ Priority order (highest to lowest): CLI flags > environment variables > config f `["ssh","-o","BatchMode=yes","host","true"]`); a value that fails to parse that way (invalid JSON, or valid JSON of the wrong shape — a bare `false`, a string, an object, ...) prints a visible warning naming the offending value and falls back to the default probe, instead of - either crashing the CLI or silently substituting the default with no explanation + either crashing the CLI or silently substituting the default with no explanation. An empty + string is the one exception: it is treated as unset, silently, same as the env var not being + set at all — a deliberate convention (an unset/cleared shell variable commonly round-trips as + `""`), not a warning candidate. To actually disable the probe (always treat the remote as + reachable and let the real git operation surface any failure on its own), set + `reachabilityCheckCommand` to a command that always exits `0`, e.g. `["true"]` — there is no + separate on/off switch, this is the supported way to opt out - failed pushes (including ones skipped by the reachability precheck) are queued locally in `stateDir/queue` and replayed on the next successful push - append-only concurrent edits are merged automatically; other conflicts default to inline conflict markers diff --git a/packages/agent-memory-sync/docs/launchd/com.agent-memory-sync.watch.plist.template b/packages/agent-memory-sync/docs/launchd/com.agent-memory-sync.watch.plist.template index 0dccf3a..e83d1f7 100644 --- a/packages/agent-memory-sync/docs/launchd/com.agent-memory-sync.watch.plist.template +++ b/packages/agent-memory-sync/docs/launchd/com.agent-memory-sync.watch.plist.template @@ -55,19 +55,25 @@ This is a deliberate contract change from watch's earlier behavior (where any push failure, including a merely unreachable remote, exited non-zero and relied on launchd to restart the process); see README.md's - `watch` section and docs/machine-setup.md for the exact boundary. A - genuine config/data error (e.g. a required `syncPaths` entry missing) - is the one remaining class of failure that still surfaces as a - non-zero exit "by design" (see README.md #systemd-unit) — that class is - what KeepAlive/ThrottleInterval below are sized for. KeepAlive's - SuccessfulExit=false mirrors that: launchd respawns on a crash - (non-zero exit) but does not respawn after a clean shutdown (including - the clean exit 0 the queued-offline case above now produces). - ThrottleInterval caps the respawn rate so a persistently broken config - does not spin the CPU in a restart loop — this is launchd's rough - equivalent of systemd's StartLimitIntervalSec/StartLimitBurst pairing, - just as a single minimum gap between attempts rather than a burst - counter. + `watch` section and docs/machine-setup.md for the exact boundary. The + queue-instead-of-crash handling is narrow, not a general catch-all: + only a failure GitClient.lookupRemoteHead / GitClient.push attributes + to the remote itself (unreachable, rejected, non-fast-forward — see + RemoteUnavailableError in src/errors.ts) is queued. Errors raised + while collecting local sync files, before the remote working copy is + even prepared (e.g. a required `syncPaths` entry missing), and any + other git-level failure while that working copy is being prepared or + committed (a full disk, a corrupted git config, a broken commit hook, + ...) still surface as a non-zero exit "by design" (see README.md + #systemd-unit) — those are what KeepAlive/ThrottleInterval below are + sized for. KeepAlive's SuccessfulExit=false mirrors that: launchd + respawns on a crash (non-zero exit) but does not respawn after a clean + shutdown (including the clean exit 0 the queued-offline case above now + produces). ThrottleInterval caps the respawn rate so a persistently + broken cause does not spin the CPU in a restart loop — this is + launchd's rough equivalent of systemd's StartLimitIntervalSec/ + StartLimitBurst pairing, just as a single minimum gap between attempts + rather than a burst counter. Offline edits still only reach the remote once this machine's own `watch` next runs successfully (or the periodic sync companion below diff --git a/packages/agent-memory-sync/docs/machine-setup.md b/packages/agent-memory-sync/docs/machine-setup.md index 4417e89..e8cf049 100644 --- a/packages/agent-memory-sync/docs/machine-setup.md +++ b/packages/agent-memory-sync/docs/machine-setup.md @@ -43,9 +43,16 @@ This document wires together the pieces already documented individually deliberate contract change from `watch`'s earlier behavior, where any push failure (including a merely unreachable remote) surfaced as a non-zero exit and relied on launchd/systemd to restart the process; see the - README.md `watch` section for the exact new boundary — a genuine - config/data error (e.g. a required `syncPaths` entry missing) still exits - non-zero, only network/git-push-level failures are now queued instead. + README.md `watch` section for the exact new boundary. The + queue-instead-of-crash handling is narrow, not a general catch-all: only a + failure `GitClient.lookupRemoteHead` / `GitClient.push` attributes to the + remote itself (unreachable, rejected, non-fast-forward — see + `RemoteUnavailableError` in `src/errors.ts`) is queued. Errors raised + while collecting local sync files, before the remote working copy is even + prepared (e.g. a required `syncPaths` entry missing), and any other + git-level failure while that working copy is being prepared or committed + (a full disk, a corrupted git config, a broken commit hook, ...) still + exit non-zero. - **`watch` is edge-triggered and does not pull — this is why the periodic sync job is required, not optional.** `watch` only commits+pushes when *this* machine's local files change; it never reads from the remote. Its diff --git a/packages/agent-memory-sync/src/commands/restore.ts b/packages/agent-memory-sync/src/commands/restore.ts index 6bab818..aeedcf2 100644 --- a/packages/agent-memory-sync/src/commands/restore.ts +++ b/packages/agent-memory-sync/src/commands/restore.ts @@ -94,14 +94,25 @@ function registerRestoreCommand(program: import("commander").Command): void { // just fetched (the whole branch history) — this is what makes an // abbreviated sha work at all, since fetchRef's own `git fetch origin // ` cannot resolve one against the remote (see git-client.ts). - if (!gitClient.resolveLocalCommit(workingCopy.repoDir, sha)) { + // Capture the resolved FULL sha and use it for every subsequent git + // call and in the reported payload below, instead of re-resolving the + // (possibly abbreviated) `sha` the operator typed on every call and + // reporting the abbreviation rather than the commit actually restored + // from. `|| sha` is a defensive fallback for the practically-unreachable + // case where fetchRef succeeds but the ref still doesn't resolve + // locally afterwards (e.g. it named a non-commit object) — preserves + // this file's prior behavior (operate on the as-typed ref) rather than + // introducing a new failure mode for that corner. + let resolvedSha = gitClient.resolveLocalCommit(workingCopy.repoDir, sha); + if (!resolvedSha) { gitClient.fetchRef(workingCopy.repoDir, sha); + resolvedSha = gitClient.resolveLocalCommit(workingCopy.repoDir, sha) || sha; } const targetRepoPaths = options.path ? [normalizeRequestedPath(runConfig.repositorySubdir, options.path)] : gitClient - .listTreePaths(workingCopy.repoDir, sha, runConfig.repositorySubdir) + .listTreePaths(workingCopy.repoDir, resolvedSha, runConfig.repositorySubdir) .filter((p: string) => p.startsWith(`${runConfig.repositorySubdir}/`)); if (targetRepoPaths.length === 0) { @@ -123,7 +134,7 @@ function registerRestoreCommand(program: import("commander").Command): void { ); } - const content = gitClient.showAtRef(workingCopy.repoDir, sha, repoRelativePath); + const content = gitClient.showAtRef(workingCopy.repoDir, resolvedSha, repoRelativePath); if (content === null) { throw new CliError( `file '${repoRelativePath}' does not exist at ${sha}.`, @@ -148,7 +159,7 @@ function registerRestoreCommand(program: import("commander").Command): void { const payload = { command: "restore", - sha, + sha: resolvedSha, dryRun: options.dryRun, repositorySubdir: runConfig.repositorySubdir, restored diff --git a/packages/agent-memory-sync/src/errors.ts b/packages/agent-memory-sync/src/errors.ts index c29337d..122da2c 100644 --- a/packages/agent-memory-sync/src/errors.ts +++ b/packages/agent-memory-sync/src/errors.ts @@ -8,6 +8,27 @@ class CliError extends Error { } } +// A narrow CliError subclass thrown ONLY from the two GitClient operations +// that can fail because the *remote* is unavailable or rejecting (see +// GitClient.lookupRemoteHead / GitClient.push in memory-sync/git-client.ts): +// performPush's catch (memory-sync/push.ts) checks for this specific type +// before converting a failure into a queued-for-replay outcome, so a +// non-network failure elsewhere in the same try block (a full disk, a +// broken commit hook, a corrupted git config, ...) is not misclassified as +// "remote unavailable" and silently swallowed into a benign-looking queue — +// it re-throws instead, preserving fail-loud/supervisor-restart semantics +// for that class of error. A plain CliError (e.g. from GitClient.run's +// generic "git command failed" fallback) is deliberately NOT treated as a +// remote failure by that check, even though it shares the same exitCode 4 — +// the exit code alone does not discriminate why a git subcommand failed, +// only the throw site does. +class RemoteUnavailableError extends CliError { + constructor(message: string, exitCode = 4) { + super(message, exitCode); + this.name = "RemoteUnavailableError"; + } +} + function isCliError(error: unknown): error is CliError { return error instanceof CliError; } @@ -22,6 +43,7 @@ function formatErrorMessage(error: unknown): string { module.exports = { CliError, + RemoteUnavailableError, isCliError, formatErrorMessage }; diff --git a/packages/agent-memory-sync/src/memory-sync/git-client.ts b/packages/agent-memory-sync/src/memory-sync/git-client.ts index b7b7725..7f67f79 100644 --- a/packages/agent-memory-sync/src/memory-sync/git-client.ts +++ b/packages/agent-memory-sync/src/memory-sync/git-client.ts @@ -1,7 +1,7 @@ const { existsSync, mkdirSync, readFileSync, readdirSync, rmSync, statSync, writeFileSync } = require("node:fs"); const { execFileSync } = require("node:child_process"); const path = require("node:path"); -const { CliError } = require("../errors"); +const { CliError, RemoteUnavailableError } = require("../errors"); interface GitCommandResult { stdout: string; @@ -53,9 +53,12 @@ class GitClient { ); if (result.exitCode !== 0) { - throw new CliError( - `could not reach remote git repository '${remoteUrl}'. Check connectivity and repository access.`, - 4 + // RemoteUnavailableError, not plain CliError: this is one of the two + // recognized "the remote itself is the problem" sites performPush's + // catch (memory-sync/push.ts) queues instead of crashing on — see + // errors.ts for why that distinction exists and matters. + throw new RemoteUnavailableError( + `could not reach remote git repository '${remoteUrl}'. Check connectivity and repository access.` ); } @@ -114,16 +117,19 @@ class GitClient { } push(repoDir: string, branch: string): void { + // Both throws below are RemoteUnavailableError, not plain CliError — see + // the errors.ts comment: these are the second of the two recognized + // "the remote is the problem" sites performPush's catch queues instead + // of crashing on (the other is lookupRemoteHead above). const result = this.run(["push", "origin", `HEAD:refs/heads/${branch}`], repoDir, true); if (result.exitCode !== 0) { if (/\[rejected\]|fetch first|non-fast-forward/i.test(result.stderr)) { - throw new CliError( - "remote branch changed during push. Re-run the sync to merge against the latest remote state.", - 4 + throw new RemoteUnavailableError( + "remote branch changed during push. Re-run the sync to merge against the latest remote state." ); } - throw new CliError("git push failed. Check repository access and branch permissions.", 4); + throw new RemoteUnavailableError("git push failed. Check repository access and branch permissions."); } } @@ -138,8 +144,17 @@ class GitClient { return repoDir; } - // Resolves `ref` (full or abbreviated sha, branch/tag name) against - // objects the working copy already has locally — no network access. + // Resolves `ref` — a hex commit sha, full or abbreviated — against objects + // the working copy already has locally, no network access, and returns + // the full sha. Precondition: callers must have already validated `ref` + // looks like a hex sha (restore.ts does this with its own + // `/^[0-9a-f]{4,64}$/i` check before ever reaching this method). This + // does not special-case branch/tag names as a *supported* input even + // though the underlying `git rev-parse ref^{commit}` would resolve one of + // those too (e.g. "HEAD", "HEAD~1", a branch name) — nothing unsafe + // reaches this method today only because its one caller pre-validates the + // hex shape in a different file; do not start passing it unvalidated + // input on the assumption that it accepts anything `git rev-parse` does. // `restore`'s working copy is always prepared via prepareWorkingCopy(), // which already ran a full `git fetch origin `, so every commit // reachable from that branch tip (i.e. any commit `restore` could diff --git a/packages/agent-memory-sync/src/memory-sync/push.ts b/packages/agent-memory-sync/src/memory-sync/push.ts index 97501c4..2c11c98 100644 --- a/packages/agent-memory-sync/src/memory-sync/push.ts +++ b/packages/agent-memory-sync/src/memory-sync/push.ts @@ -2,6 +2,7 @@ const { collectLocalSyncFiles, toRepositoryRelativePath } = require("./config"); +const { RemoteUnavailableError } = require("../errors"); const { GitClient } = require("./git-client"); const { mergeText } = require("./merge"); const { checkRemoteReachable } = require("./reachability"); @@ -155,6 +156,23 @@ async function performPush(config: PushConfig, options: PushOptions) { notes: queuedSnapshots.length > 0 ? [`replayed ${queuedSnapshots.length} queued snapshot(s)`] : [] }; } catch (error) { + // Only a RemoteUnavailableError (thrown exclusively from + // GitClient.lookupRemoteHead and GitClient.push — see errors.ts) is + // queued instead of crashing: those are the two operations that can + // fail because the *remote* is unavailable or rejecting the push. Any + // other error raised inside this try block (prepareWorkingCopy's own + // init/fetch/checkout, applySnapshotToWorkingCopy's file writes, + // commitAll, collectRemoteFiles, any StateStore write — a full disk, a + // broken commit hook, a corrupted git config, ...) re-throws, so it + // still crashes loud and reaches the supervisor-restart path (launchd + // KeepAlive, systemd StartLimit*) instead of being misreported as a + // benign "remote unavailable" queue. See + // tests/integration/watch-mirror-delete.test.ts's "non-network git + // failure inside the push" test for the case this guards against. + if (!(error instanceof RemoteUnavailableError)) { + throw error; + } + return enqueueCurrentSnapshot( stateStore, currentLocalMap, diff --git a/packages/agent-memory-sync/tests/integration/watch-mirror-delete.test.ts b/packages/agent-memory-sync/tests/integration/watch-mirror-delete.test.ts index 1fa7955..5f368c4 100644 --- a/packages/agent-memory-sync/tests/integration/watch-mirror-delete.test.ts +++ b/packages/agent-memory-sync/tests/integration/watch-mirror-delete.test.ts @@ -272,18 +272,17 @@ test("watch tick queues locally when the remote is unreachable, then replays the }); // Counterpart to the queue-replay test above (agent-tasks -// 1b63070d-9ea1-4a38-bba0-e58a4678b596): the reachability precheck softens -// watch's push failure handling for one specific class of failure — the -// remote being unreachable — into a clean, non-throwing queue. It must not -// soften anything else. A genuine config/data error (README.md's own +// 1b63070d-9ea1-4a38-bba0-e58a4678b596). This test only covers the +// pre-try-block boundary: a genuine config/data error (README.md's own // example: a required `syncPaths` entry missing) is raised by // collectLocalSyncFiles() before performPush's reachability precheck or its -// try/catch are ever reached (see src/memory-sync/push.ts), so it still -// propagates all the way out to watch's handleSnapshotError and exits -// non-zero — preserving the supervisor-respawn semantics (launchd -// KeepAlive/ThrottleInterval, systemd's StartLimitIntervalSec/-Burst) that -// this class of failure relies on. The remote here is a real, reachable -// bare repo, isolating the assertion from reachability entirely. +// try/catch are ever reached (see src/memory-sync/push.ts), so it was +// always going to propagate — this class of error never had a "soften it +// into a queue" risk to begin with. See the next test for the boundary that +// actually mattered: a failure raised *inside* performPush's try block, +// where the reachability precheck's queue-instead-of-throw handling lives, +// and which — without RemoteUnavailableError discrimination in push.ts's +// catch — used to swallow indiscriminately. test("watch tick with a missing required syncPaths entry still exits non-zero, independent of remote reachability", async () => { const root = createSandbox("watch-required-syncpath-missing"); const remoteDir = initBareRemote(root); @@ -311,6 +310,94 @@ test("watch tick with a missing required syncPaths entry still exits non-zero, i assert.match(stderr, /required sync path/i); }); +// A tiny `git` stand-in: forwards every subcommand to the real `git` binary +// unmodified, except `commit`, which it forces to fail — simulating a +// non-network git failure that happens *inside* performPush's try block +// (src/memory-sync/push.ts), after prepareWorkingCopy has already +// succeeded (a full disk writing the commit object, a corrupt index, a +// broken commit hook, ...). Written to `root` and made executable so it can +// be pointed at via the `gitBinary` config field. +// A plain POSIX shell script, deliberately NOT a Node script: this repo's +// coverage run sets NODE_V8_COVERAGE-style instrumentation that a `#!/usr/bin/env +// node` child process would inherit and report itself into the coverage +// table (a harmless but noisy artifact, spotted while writing this test) — +// `exec`-ing the real `git` keeps this a single extra process with no such +// side effect. +function writeStubGitFailingOnCommit(root: string): string { + const stubPath = path.join(root, "stub-git-fails-on-commit.sh"); + writeText( + stubPath, + [ + "#!/bin/sh", + 'if [ "$1" = "commit" ]; then', + ' echo "stub-git: forced commit failure (simulated non-network error)" >&2', + " exit 17", + "fi", + 'exec git "$@"', + "" + ].join("\n") + ); + fs.chmodSync(stubPath, 0o755); + return stubPath; +} + +// THE boundary test AC1 actually depends on. Before this rework, performPush +// wrapped everything from prepareWorkingCopy through the final state-store +// writes in one catch-all that swallowed ANY thrown error into +// enqueueCurrentSnapshot's "queued, exit 0" outcome (see the removed +// unconditional catch in src/memory-sync/push.ts) — indistinguishable from +// the genuinely-benign unreachable-remote case the reachability precheck is +// meant to cover. A non-network failure raised inside that try block (a +// full disk, a broken commit hook, a corrupted git config, ...) would have +// logged the same benign "queued locally instead of pushing" line, exited +// 0, and — under launchd's KeepAlive: SuccessfulExit=false — never been +// respawned, and never synced again. Invisible by construction, and the +// opposite of the supervisor semantics this rework promises. +// +// The fix (src/errors.ts's RemoteUnavailableError, thrown only from +// GitClient.lookupRemoteHead and GitClient.push, checked explicitly in +// push.ts's catch) narrows that catch to exactly the two git operations +// that can fail because the *remote* is unavailable/rejecting; everything +// else re-throws. This test forces `git commit` (inside the try block, +// after prepareWorkingCopy has already succeeded against the real, +// reachable bare remote) to fail for a reason that has nothing to do with +// the remote, and pins that watch still exits non-zero for it. +test("watch tick with a non-network git failure inside the push (e.g. commit failing) still exits non-zero, not queued", async () => { + const root = createSandbox("watch-non-network-git-failure"); + const remoteDir = initBareRemote(root); + const workspaceRoot = path.join(root, "workspace"); + const configPath = path.join(root, "config.json"); + + writeText(path.join(workspaceRoot, "MEMORY.md"), "seed\n"); + // Seed the remote with the real git binary first, so the stub (used only + // for the tick under test below) never has to handle first-time-push + // bootstrapping. + writeProjectConfig(configPath, createConfig(workspaceRoot, remoteDir)); + runCli(["run", "default", "--config", configPath, "--mode", "push", "--output", "json"]); + + const stubGitBinary = writeStubGitFailingOnCommit(root); + writeProjectConfig(configPath, { + ...createConfig(workspaceRoot, remoteDir), + gitBinary: stubGitBinary + }); + + const { exitCode, stderr } = await runWatchTick(configPath, () => { + writeText(path.join(workspaceRoot, "MEMORY.md"), "seed\nlocal edit\n"); + }); + + assert.notEqual( + exitCode, + 0, + `watch was expected to crash loudly on a non-network git commit failure; it exited 0 instead ` + + `(queued, masking the failure). stderr: ${stderr}` + ); + assert.doesNotMatch( + stderr, + /queued locally/, + "a non-network git failure must not be reported as a benign queued-instead-of-pushed tick" + ); +}); + // Rework finding (LOW, review of this task): pins that a successful tick // updates stateDir/base to the post-merge remote state — the input the next // tick's 3-way merge relies on to correctly leave an unpulled peer file diff --git a/packages/agent-memory-sync/tests/integration/watch-restore.test.ts b/packages/agent-memory-sync/tests/integration/watch-restore.test.ts index 21cbc5a..eea0e07 100644 --- a/packages/agent-memory-sync/tests/integration/watch-restore.test.ts +++ b/packages/agent-memory-sync/tests/integration/watch-restore.test.ts @@ -372,6 +372,12 @@ test("restore resolves against the already-fetched branch history an const payload = JSON.parse(result.stdout); assert.equal(payload.restored.length, 1); assert.equal(readText(path.join(workspaceRoot, "MEMORY.md")), "snapshot 1\n"); + // Pins that the reported sha is the resolved FULL commit actually restored + // from, not an echo of the abbreviation the operator typed — restore.ts + // captures GitClient.resolveLocalCommit's return value and threads it + // through to the payload instead of discarding it after the truthiness + // check. + assert.equal(payload.sha, fullSha); }); test("restore that is not reachable from the configured branch fails loudly with an explicit full-sha hint", () => { diff --git a/packages/agent-memory-sync/tests/unit/reachability-check-command-env.test.ts b/packages/agent-memory-sync/tests/unit/reachability-check-command-env.test.ts index 62de0ce..d83d6ac 100644 --- a/packages/agent-memory-sync/tests/unit/reachability-check-command-env.test.ts +++ b/packages/agent-memory-sync/tests/unit/reachability-check-command-env.test.ts @@ -76,7 +76,7 @@ test(`${ENV_KEY}='not valid json' warns visibly instead of crashing the whole CL assert.match(stderr, /json array/i); }); -test(`${ENV_KEY}='["true","5","x"]' with non-array-of-strings shape (a JSON object) warns visibly`, async () => { +test(`${ENV_KEY}='{"not":"an array"}' (valid JSON, but not an array at all) warns visibly`, async () => { const { reachabilityCheckCommand, stderr } = await resolveWithEnvValue('{"not":"an array"}'); assert.equal(reachabilityCheckCommand, null); @@ -84,6 +84,30 @@ test(`${ENV_KEY}='["true","5","x"]' with non-array-of-strings shape (a JSON obje assert.match(stderr, /json array/i); }); +// Distinct code path from the object-shaped case above: this value IS a +// JSON array, so it clears this module's own `Array.isArray` gate, and +// instead reaches normalizeReachabilityCheckCommand's own element-shape +// check (loader.ts: `value.some((entry) => typeof entry !== "string" || +// !entry)`), which throws for a non-string / empty-string element. Both +// throw sites are caught by the same try/catch and produce the same +// warning, but exercising this one separately pins branch coverage on the +// element-shape guard specifically, not just the top-level type guard. +test(`${ENV_KEY}='["ssh",""]' (a JSON array, but with an empty-string element) warns visibly`, async () => { + const { reachabilityCheckCommand, stderr } = await resolveWithEnvValue('["ssh",""]'); + + assert.equal(reachabilityCheckCommand, null); + assert.match(stderr, /warning/i); + assert.match(stderr, /json array/i); +}); + +test(`${ENV_KEY}='[1,2]' (a JSON array, but of numbers, not strings) warns visibly`, async () => { + const { reachabilityCheckCommand, stderr } = await resolveWithEnvValue("[1,2]"); + + assert.equal(reachabilityCheckCommand, null); + assert.match(stderr, /warning/i); + assert.match(stderr, /json array/i); +}); + test(`${ENV_KEY} with a valid JSON array of non-empty strings applies with no warning`, async () => { const { reachabilityCheckCommand, stderr } = await resolveWithEnvValue( '["ssh","-o","BatchMode=yes","host","true"]' @@ -99,3 +123,22 @@ test(`${ENV_KEY}='[]' (explicit empty array) applies as null with no warning`, a assert.equal(reachabilityCheckCommand, null); assert.doesNotMatch(stderr, /warning/i); }); + +// Deliberate, documented convention (README.md's Sync behavior section), +// pinned here so it stays a decision rather than an accident: readEnvConfig +// only even looks at this env var inside `if (env.AGENT_MEMORY_SYNC_ +// REACHABILITY_CHECK_COMMAND)`, and an empty string is falsy in JS, so it +// is treated identically to the env var not being set at all — silently, +// with no warning. This is the same *shape* as the incident this file +// otherwise pins (a value that disables the override without saying so), +// but empty-string-as-unset is a common, intentional shell convention (an +// unset or cleared variable often round-trips as ""), unlike a stray +// `false`/`not valid json`/wrong-shaped value, which is far more likely a +// mistake. Hence: silent by design, not a warning candidate — if this ever +// changes, this test should change with it. +test(`${ENV_KEY}='' (empty string) is treated as unset — silent, no warning, no override applied`, async () => { + const { reachabilityCheckCommand, stderr } = await resolveWithEnvValue(""); + + assert.equal(reachabilityCheckCommand, null); + assert.doesNotMatch(stderr, /warning/i); +});