From c54d7b41166caf252e2018dd42126c35bf39197a Mon Sep 17 00:00:00 2001
From: CodeGhost21 <164498022+CodeGhost21@users.noreply.github.com>
Date: Thu, 9 Jul 2026 05:25:08 +0530
Subject: [PATCH] fix(memory): stop Vault checklist reporting healthy while
 Sync shows Degraded (#4691) (#4703)

---
 src/openhuman/memory/read_rpc/vault.rs | 66 +++++++++++++++++++++++++-
 src/openhuman/memory/read_rpc_tests.rs | 15 +++++-
 2 files changed, 79 insertions(+), 2 deletions(-)

diff --git a/src/openhuman/memory/read_rpc/vault.rs b/src/openhuman/memory/read_rpc/vault.rs
index 10b5930d4..d524362e4 100644
--- a/src/openhuman/memory/read_rpc/vault.rs
+++ b/src/openhuman/memory/read_rpc/vault.rs
@@ -74,7 +74,7 @@ pub async fn vault_health_check_rpc(
         .map_err(|e| format!("vault_health_check pipeline_status: {e}"))?;
 
     let (content_root_abs, exists, readable, writable, obsidian_registered) = fs_probe;
-    let pipeline_healthy = pipeline.value.status != "error" && !pipeline.value.is_paused;
+    let pipeline_healthy = pipeline_is_healthy(&pipeline.value.status);
     let last_sync_ms = pipeline.value.last_sync_ms.max(0);
 
     let resp = VaultHealthCheckResponse {
@@ -101,6 +101,23 @@ pub async fn vault_health_check_rpc(
     Ok(RpcOutcome::single_log(resp, log))
 }
 
+/// Whether the memory pipeline status counts as "healthy" for the Vault setup
+/// checklist.
+///
+/// #4691: the Vault checklist and the Memory Sync panel read the SAME signal
+/// (`pipeline_status_rpc` → `derive_pipeline_status`), so they must never
+/// disagree. The prior denylist (`status != "error" && !is_paused`) let
+/// `"degraded"` slip through as healthy, so the Vault surface reported
+/// "Memory pipeline is healthy" while Memory Sync reported "Degraded".
+///
+/// This is an allowlist of the fully-operational states so any future
+/// non-operational status added to `derive_pipeline_status` defaults to
+/// unhealthy rather than silently reading as healthy. `paused`/`error`/
+/// `degraded` are all excluded.
+fn pipeline_is_healthy(status: &str) -> bool {
+    matches!(status, "idle" | "running" | "syncing")
+}
+
 fn probe_directory_writable(dir: &std::path::Path) -> bool {
     use std::io::Write;
     let ts = std::time::SystemTime::now()
@@ -126,3 +143,50 @@ fn probe_directory_writable(dir: &std::path::Path) -> bool {
         Err(_) => false,
     }
 }
+
+#[cfg(test)]
+mod tests {
+    use super::pipeline_is_healthy;
+
+    // The full set of statuses `derive_pipeline_status` can return. Kept in sync
+    // with `memory_tree::tree::rpc::derive_pipeline_status` so a new status forces
+    // an explicit decision here.
+    const OPERATIONAL: &[&str] = &["idle", "running", "syncing"];
+    const NON_OPERATIONAL: &[&str] = &["paused", "error", "degraded"];
+
+    #[test]
+    fn operational_statuses_are_healthy() {
+        for status in OPERATIONAL {
+            assert!(
+                pipeline_is_healthy(status),
+                "expected `{status}` to be healthy"
+            );
+        }
+    }
+
+    #[test]
+    fn non_operational_statuses_are_not_healthy() {
+        for status in NON_OPERATIONAL {
+            assert!(
+                !pipeline_is_healthy(status),
+                "expected `{status}` to be unhealthy"
+            );
+        }
+    }
+
+    #[test]
+    fn degraded_is_not_healthy_regression_4691() {
+        // #4691: "degraded" previously leaked through the denylist and made the
+        // Vault checklist report "Memory pipeline is healthy" while Memory Sync
+        // reported "Degraded". It must read as unhealthy.
+        assert!(!pipeline_is_healthy("degraded"));
+    }
+
+    #[test]
+    fn unknown_status_defaults_to_unhealthy() {
+        // Allowlist semantics: any future/unexpected status is treated as
+        // unhealthy rather than silently reported as healthy.
+        assert!(!pipeline_is_healthy("boom"));
+        assert!(!pipeline_is_healthy(""));
+    }
+}
diff --git a/src/openhuman/memory/read_rpc_tests.rs b/src/openhuman/memory/read_rpc_tests.rs
index 07ed603e6..07335be71 100644
--- a/src/openhuman/memory/read_rpc_tests.rs
+++ b/src/openhuman/memory/read_rpc_tests.rs
@@ -902,6 +902,11 @@ async fn obsidian_status_blank_override_is_treated_as_none() {
 
 #[tokio::test]
 async fn vault_health_check_reports_missing_content_root_for_fresh_workspace() {
+    // `pipeline_healthy` reads the process-global degraded flags (via
+    // `pipeline_status_rpc` → `current_degraded_state`), which sibling
+    // `memory_tree` tests set and never clear. Serialise + reset to a clean
+    // baseline so the assertion is deterministic. See #4691.
+    let _g = crate::openhuman::memory_tree::health::test_guard();
     let (_tmp, cfg) = test_config();
     let outcome = vault_health_check_rpc(&cfg, None).await.unwrap();
 
@@ -957,7 +962,15 @@ async fn vault_health_check_reports_writable_and_obsidian_registered_when_ready(
     assert!(outcome.value.readable);
     assert!(outcome.value.writable);
     assert!(outcome.value.obsidian_registered);
-    assert!(outcome.value.pipeline_healthy);
+    // Intentionally NOT asserting `pipeline_healthy` here: with a seeded chunk
+    // (total_chunks > 0) the derived status depends on the process-global
+    // degraded flags, which unguarded parallel `memory_tree` extraction/pipeline
+    // tests set and never clear (structure degrades only clears on a *successful*
+    // extraction, which never happens under test). Post-#4691 a leaked "degraded"
+    // correctly reads as unhealthy, so asserting healthy here would be flaky.
+    // The health mapping is covered deterministically by the `pipeline_is_healthy`
+    // unit tests in `read_rpc/vault.rs`; this test covers the filesystem readiness
+    // wiring. See also `memory_tree::tree::rpc::pipeline_status_reports_chunk_aggregates_after_ingest`.
     assert!(outcome.value.last_sync_ms > 0);
     assert!(
         !outcome.logs[0].contains(content_root.to_str().unwrap()),