diff --git a/packages/agent-memory-sync/README.md b/packages/agent-memory-sync/README.md index 9d7f89b..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 @@ -195,7 +201,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 +296,17 @@ 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. 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 3803ebd..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 @@ -46,22 +46,39 @@ 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 + `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. 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. - 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..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 @@ -281,9 +288,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..aeedcf2 100644 --- a/packages/agent-memory-sync/src/commands/restore.ts +++ b/packages/agent-memory-sync/src/commands/restore.ts @@ -89,12 +89,30 @@ 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). + // 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) { @@ -116,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}.`, @@ -141,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/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/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 b9b7023..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,11 +144,52 @@ class GitClient { return repoDir; } + // 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 + // 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/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 8a0a670..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 @@ -271,6 +271,133 @@ 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). 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 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); + 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); +}); + +// 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 3e3f659..eea0e07 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,84 @@ 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"); + // 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", () => { + 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..d83d6ac --- /dev/null +++ b/packages/agent-memory-sync/tests/unit/reachability-check-command-env.test.ts @@ -0,0 +1,144 @@ +// 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}='{"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); + assert.match(stderr, /warning/i); + 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"]' + ); + + 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); +}); + +// 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); +});