fix(cli): address PR review findings on omnigraph update (MR-612)

- Refuse non-interactive update without --yes (Devin Review P1):
  bail with a clear error instead of silently replacing binaries when
  stdin is not a TTY. Matches gh / rustup / apt-get posture.
- Persist 24h cooldown on refresh failures (cubic P2): the hidden
  `__refresh-update-cache` subprocess now writes a fresh
  `checked_at_unix` even when the GitHub API call fails (keeping the
  previously-known tag), so a transient outage doesn't spam refreshes.
- Make the checksum-mismatch test platform-correct (cubic P2): compare
  byte-for-byte against the snapshotted original binary instead of
  ELF/shebang magic, which excluded macOS arm64 (Mach-O).
- Bump cli-reference top-level family count 17 -> 18 (cubic P3).
- Add a new integration test asserting non-interactive bail-out.

Co-Authored-By: Ragnor Comerford <ragnor.comerford@gmail.com>

crates/omnigraph-cli/src/update.rs

@@ -115,7 +115,15 @@ pub async fn run(args: UpdateArgs) -> Result<()> {
         return Ok(());
     }
 
-    if !args.yes && io::stdin().is_terminal() {
+    if !args.yes {
+        // Refuse to silently replace binaries when there is no user at the
+        // terminal — same posture as `gh`, `rustup`, `apt-get`. Scripted /
+        // piped invocations must opt in with `--yes`.
+        if !io::stdin().is_terminal() {
+            bail!(
+                "refusing to update non-interactively; pass --yes to confirm or run from a terminal"
+            );
+        }
         eprint!(
             "Update from v{} to {}? [y/N] ",
             current_version, latest_tag

crates/omnigraph-cli/src/version_check.rs

@@ -17,7 +17,7 @@
 
 use std::fs;
 use std::io::{self, IsTerminal, Write};
-use std::path::PathBuf;
+use std::path::{Path, PathBuf};
 use std::process::{Command, Stdio};
 use std::time::{Duration, SystemTime, UNIX_EPOCH};
 
@@ -82,30 +82,62 @@ pub fn maybe_notify(current_version: &str, subcommand_skips_check: bool) {
 }
 
 /// Body of the hidden `__refresh-update-cache` subcommand. Performs a single
-/// network call to the GitHub Releases API and writes the cache file. Errors
-/// are surfaced (the caller is the child process; failures only affect that
-/// child's exit code).
+/// network call to the GitHub Releases API and writes the cache file.
+///
+/// On a transient failure (network down, rate-limited, …) we still touch the
+/// cache file with the previously-known `latest_version` (or the running
+/// binary's own version as a fallback) so the 24h cooldown is honored and the
+/// parent process doesn't spawn a fresh refresh on every invocation.
 pub async fn refresh_cache_subcommand() -> Result<()> {
     let cache_path = cache_file_path().ok_or_else(|| {
         color_eyre::eyre::eyre!("could not resolve cache directory (HOME unset?)")
     })?;
+    let previous = read_cache(&cache_path);
     let client = reqwest::Client::builder()
         .user_agent(concat!("omnigraph-cli/", env!("CARGO_PKG_VERSION")))
         .timeout(Duration::from_secs(10))
         .build()
         .context("build reqwest client")?;
-    let tag = fetch_latest_stable_tag(&client).await?;
-    let version = tag.trim().trim_start_matches('v').to_string();
-    if version.is_empty() {
-        bail!("release metadata missing tag_name");
+    match fetch_latest_stable_tag(&client).await {
+        Ok(tag) => {
+            let version = tag.trim().trim_start_matches('v').to_string();
+            if version.is_empty() {
+                touch_cooldown(&cache_path, previous.as_ref());
+                bail!("release metadata missing tag_name");
+            }
+            let entry = CacheEntry {
+                checked_at_unix: unix_now(),
+                latest_version: version,
+                repo_slug: REPO_SLUG.to_string(),
+            };
+            write_cache(&cache_path, &entry)?;
+            Ok(())
+        }
+        Err(err) => {
+            // Persist a fresh `checked_at_unix` even on failure so we honor
+            // the 24h cooldown across transient outages. We keep whatever
+            // `latest_version` we already knew (or fall back to the running
+            // binary's own version, which never produces a false "newer
+            // available" notice).
+            touch_cooldown(&cache_path, previous.as_ref());
+            Err(err)
+        }
     }
+}
+
+/// Write a fresh `checked_at_unix` timestamp to the cache while keeping the
+/// previously-known `latest_version` (or, when there is none, recording the
+/// current binary's version so `version_is_newer` doesn't fire spuriously).
+fn touch_cooldown(cache_path: &Path, previous: Option<&CacheEntry>) {
+    let latest_version = previous
+        .map(|p| p.latest_version.clone())
+        .unwrap_or_else(|| env!("CARGO_PKG_VERSION").to_string());
     let entry = CacheEntry {
         checked_at_unix: unix_now(),
-        latest_version: version,
+        latest_version,
         repo_slug: REPO_SLUG.to_string(),
     };
-    write_cache(&cache_path, &entry)?;
-    Ok(())
+    let _ = write_cache(cache_path, &entry);
 }
 
 fn spawn_detached_refresh() {
@@ -146,12 +178,12 @@ fn libc_setsid() -> i64 {
     unsafe { setsid() as i64 }
 }
 
-fn read_cache(path: &PathBuf) -> Option<CacheEntry> {
+fn read_cache(path: &Path) -> Option<CacheEntry> {
     let raw = fs::read_to_string(path).ok()?;
     serde_json::from_str(&raw).ok()
 }
 
-fn write_cache(path: &PathBuf, entry: &CacheEntry) -> Result<()> {
+fn write_cache(path: &Path, entry: &CacheEntry) -> Result<()> {
     if let Some(parent) = path.parent() {
         fs::create_dir_all(parent)
             .with_context(|| format!("create cache dir {}", parent.display()))?;

crates/omnigraph-cli/tests/update.rs

@@ -369,6 +369,10 @@ fn update_fails_loudly_on_checksum_mismatch() {
     let dir = TempDir::new().unwrap();
     install_real_binary(dir.path());
     install_fake_server(dir.path(), b"#!/bin/sh\necho old-server\n");
+    // Snapshot the original bytes so we can assert byte-for-byte preservation
+    // after a failed update, without making OS-specific assumptions about the
+    // executable format (ELF on Linux, Mach-O on macOS, etc.).
+    let original_omnigraph = fs::read(dir.path().join("omnigraph")).unwrap();
 
     let new_omnigraph = b"NEW-OMNIGRAPH-PAYLOAD";
     let (archive, _digest) = build_release_archive(new_omnigraph, None);
@@ -404,9 +408,14 @@ fn update_fails_loudly_on_checksum_mismatch() {
     );
     // The original binary must not be replaced when the checksum fails.
     let preserved = fs::read(dir.path().join("omnigraph")).unwrap();
-    assert!(
-        preserved.starts_with(b"\x7fELF") || preserved.starts_with(b"#!"),
-        "original binary should still be in place"
+    assert_eq!(
+        preserved, original_omnigraph,
+        "original binary should be preserved byte-for-byte when the checksum fails"
+    );
+    assert_ne!(
+        preserved.as_slice(),
+        &new_omnigraph[..],
+        "rejected payload must not have been written"
     );
 }
 
@@ -450,6 +459,53 @@ fn update_does_not_replace_omnigraph_server_when_not_present() {
     assert!(!dir.path().join("omnigraph-server").exists());
 }
 
+#[test]
+fn update_refuses_to_run_non_interactively_without_yes() {
+    let asset = match current_platform_asset() {
+        Some(a) => a,
+        None => return,
+    };
+    let dir = TempDir::new().unwrap();
+    install_real_binary(dir.path());
+    let original_omnigraph = fs::read(dir.path().join("omnigraph")).unwrap();
+
+    let new_omnigraph = b"NEW-OMNIGRAPH-PAYLOAD";
+    let (archive, digest) = build_release_archive(new_omnigraph, None);
+
+    let fixture = Fixture::start();
+    fixture.route(
+        "/repos/ModernRelay/omnigraph/releases/latest",
+        200,
+        "application/json",
+        release_json("v999.0.0"),
+    );
+    fixture.route(
+        &asset_path("v999.0.0", asset),
+        200,
+        "application/octet-stream",
+        archive,
+    );
+    let stem = asset.trim_end_matches(".tar.gz");
+    fixture.route(
+        &asset_path("v999.0.0", &format!("{stem}.sha256")),
+        200,
+        "text/plain",
+        format!("{digest}  {asset}\n").into_bytes(),
+    );
+
+    // No --yes, no TTY (assert_cmd's stdin is not a terminal): must bail out
+    // before any binary is replaced.
+    let out = run_update(dir.path(), &fixture, &[]);
+    assert!(!out.status.success(), "should refuse non-interactive update without --yes");
+    let stderr = String::from_utf8_lossy(&out.stderr);
+    assert!(
+        stderr.contains("non-interactive") || stderr.contains("--yes"),
+        "expected non-interactive refusal; got: {stderr}"
+    );
+    let preserved = fs::read(dir.path().join("omnigraph")).unwrap();
+    assert_eq!(preserved, original_omnigraph);
+}
+
 #[test]
 fn update_check_handles_missing_release_metadata() {
     if current_platform_asset().is_none() {

docs/cli-reference.md

@@ -2,7 +2,7 @@
 
 A reference for the `omnigraph` binary's command surface and `omnigraph.yaml` schema. For a quick-start guide, see [cli.md](cli.md).
 
-17 top-level command families, 40+ subcommands. All commands accept either a positional `URI`, `--uri`, or a `--target <name>` resolved against `omnigraph.yaml`.
+18 top-level command families, 40+ subcommands. All commands accept either a positional `URI`, `--uri`, or a `--target <name>` resolved against `omnigraph.yaml`.
 
 ## Top-level commands