From ca92ad34aa3c578df085a8b8597138b266d9fa86 Mon Sep 17 00:00:00 2001 From: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> Date: Sun, 10 May 2026 21:06:42 +0000 Subject: [PATCH] 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 --- crates/omnigraph-cli/src/update.rs | 10 +++- crates/omnigraph-cli/src/version_check.rs | 58 ++++++++++++++++----- crates/omnigraph-cli/tests/update.rs | 62 +++++++++++++++++++++-- docs/cli-reference.md | 2 +- 4 files changed, 114 insertions(+), 18 deletions(-) diff --git a/crates/omnigraph-cli/src/update.rs b/crates/omnigraph-cli/src/update.rs index 9ccb221..0e1faa7 100644 --- a/crates/omnigraph-cli/src/update.rs +++ b/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 diff --git a/crates/omnigraph-cli/src/version_check.rs b/crates/omnigraph-cli/src/version_check.rs index 953bceb..a848e4c 100644 --- a/crates/omnigraph-cli/src/version_check.rs +++ b/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 { +fn read_cache(path: &Path) -> Option { 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()))?; diff --git a/crates/omnigraph-cli/tests/update.rs b/crates/omnigraph-cli/tests/update.rs index ba15631..ead7510 100644 --- a/crates/omnigraph-cli/tests/update.rs +++ b/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() { diff --git a/docs/cli-reference.md b/docs/cli-reference.md index 0fbf9ac..b1fb353 100644 --- a/docs/cli-reference.md +++ b/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 ` resolved against `omnigraph.yaml`. +18 top-level command families, 40+ subcommands. All commands accept either a positional `URI`, `--uri`, or a `--target ` resolved against `omnigraph.yaml`. ## Top-level commands