fix(memory-sync): only queue failures we attribute to the remote

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 `<sha>`. 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 `<sha>`. 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. `<sha>` 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 <remote> <ref>` 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

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.
 -->

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 <short-sha>` 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 <sha>` (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 <sha> --config profiles/<name>.json --dry-run
 
 # Roll back a single file — --path is relative to repositorySubdir, and

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
+      // <ref>` 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

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<string, unknown>): UserConfig {
   const normalized: UserConfig = {};
   const aliasMap: Record<string, keyof UserConfig> = {

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
 };