Files
openhuman/tests/skills_sync_memory_test.rs
T
c134d96056 fix(skills): route sync RPC to onSync handler and persist state to memory (#183)
* feat(debug): add script for Notion sync memory verification and enhance memory persistence

- Introduced `debug-notion-sync-memory.sh` to facilitate live testing of the Notion skill with memory verification.
- The script validates the full flow from skill start to memory persistence, ensuring required environment variables are set.
- Updated `handle_skills_sync` to change the RPC method from `skill/tick` to `skill/sync` for better clarity in the sync process.
- Implemented `persist_state_to_memory` function to ensure published ops state is saved to memory after sync, cron, and tick events.
- Enhanced end-to-end tests to validate memory persistence during skill sync and tick operations, ensuring robust functionality.

This update improves the debugging process for the Notion skill and enhances the overall reliability of memory operations.

* style: apply cargo fmt formatting

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* feat(memory): implement background memory-write worker for skill state persistence

- Introduced a `MemoryWriteJob` struct to encapsulate the details of memory write operations.
- Added a bounded background worker using `tokio::spawn` to handle memory writes asynchronously, improving performance and responsiveness.
- Updated `persist_state_to_memory` to queue memory write jobs instead of executing them directly, allowing for better flow control and error handling.
- Enhanced logging for memory persistence operations to provide clearer insights into success and failure cases.
- Modified event handling in `handle_message` to ensure state persistence occurs only after successful operations, reducing the risk of data loss.

This update significantly enhances the memory management capabilities of the skill instance, ensuring more reliable state persistence.

* feat(skills): enhance skills synchronization and memory persistence

- Added detailed debug logging in `handle_skills_sync` to track skill synchronization events, improving observability during skill operations.
- Updated `persist_state_to_memory` to include a memory write transaction, ensuring state persistence is handled more robustly and efficiently.
- Enhanced event handling in `handle_message` to ensure memory writes are queued correctly, improving the reliability of state persistence during skill operations.

These changes improve the overall functionality and reliability of skills synchronization and memory management.

* feat(skills): enhance skills synchronization and memory persistence

- Added detailed debug logging in `handle_skills_sync` to track skill synchronization events, improving observability during skill operations.
- Updated `persist_state_to_memory` to include a `memory_write_tx` parameter, allowing for more efficient state persistence in the event loop.
- Enhanced tests for skills synchronization to ensure robust memory handling and state persistence during skill operations.

This update improves the reliability and traceability of skills synchronization processes, ensuring better memory management and debugging capabilities.

* refactor(tests): update JSON-RPC sync handling in end-to-end tests

- Enhanced comments in `json_rpc_skills_runtime_start_tools_call_stop` to clarify the sync process routing through the `skill/sync` RPC.
- Updated assertions in `skills_sync_rpc_calls_on_sync_not_on_tick` to ensure proper handling of the new sync flow, verifying that the `skills_sync` method routes correctly to `onSync`.
- Improved logging for better observability during skill synchronization tests, ensuring that the expected behavior aligns with the updated RPC structure.

These changes improve the clarity and reliability of the end-to-end tests related to skills synchronization.

* feat(env): enhance environment configuration for skills development

- Updated `.env.example` to include `SKILLS_LOCAL_DIR` for local skills source directory, allowing developers to specify a path for skill discovery and installation.
- Improved comments in the environment file to clarify the usage of `SKILLS_REGISTRY_URL` for both remote and local paths, enhancing the development experience.
- Refactored `qjs_engine.rs` to prioritize the `SKILLS_LOCAL_DIR` for skill source directory resolution, improving local development workflows.
- Added utility functions in `registry_ops.rs` to support local file path handling for skill registries, enhancing flexibility in skill management.

These changes streamline the development process for skills by providing clearer configuration options and improving local development capabilities.

* feat(tests): enhance skills directory discovery in test files

- Updated `try_find_skills_dir` function across multiple test files to include support for the `SKILLS_LOCAL_DIR` environment variable, improving the flexibility of skills directory resolution.
- Enhanced comments to clarify the order of directory search priorities, ensuring better understanding for developers.
- Improved error handling and logging for cases where the specified directory does not exist, aiding in debugging and test reliability.

These changes streamline the skills directory discovery process in tests, enhancing the overall development experience.

* feat(tests): enhance skills directory discovery in test files

- Updated `try_find_skills_dir` function to include support for the `SKILLS_LOCAL_DIR` environment variable, allowing for more flexible skills directory resolution.
- Improved documentation to clarify the priority order for skills directory discovery.
- Refactored multiple test files to utilize the updated skills directory discovery logic, enhancing consistency and maintainability across tests.

These changes streamline the skills directory discovery process in tests, improving the overall testing framework.

* refactor(tests): streamline memory client verification in Notion live tests

- Updated the memory client verification process in `notion_live_with_real_data` to utilize `MemoryClient::new_local()` for improved clarity and consistency.
- Enhanced comments to clarify the memory store check location and removed redundant error handling for workspace directory creation.
- Simplified the error logging to focus on the memory client creation failure, improving readability and maintainability of the test code.

These changes enhance the reliability of memory verification in the Notion live tests, ensuring a clearer understanding of the memory client initialization process.

---------

Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
2026-04-01 22:00:57 -07:00

389 lines
14 KiB
Rust

//! Integration test: skill sync → memory persistence.
//!
//! Verifies that calling `skill/sync` via RPC:
//! 1. Invokes the skill's `onSync()` JS handler
//! 2. Persists published state to the local memory store
//!
//! Also tests that cron-triggered syncs and tick persist to memory.
//!
//! Run:
//! cargo test --test skills_sync_memory_test -- --nocapture
use std::path::{Path, PathBuf};
use std::sync::{Arc, Mutex, OnceLock};
use std::time::Duration;
use serde_json::json;
use tempfile::tempdir;
use openhuman_core::core::all::try_invoke_registered_rpc;
use openhuman_core::openhuman::memory::MemoryClient;
use openhuman_core::openhuman::skills::qjs_engine::{set_global_engine, RuntimeEngine};
/// Serializes tests in this binary: `set_global_engine` mutates the process-wide
/// GLOBAL_ENGINE, so parallel tests would cross-wire engine instances.
static ENV_LOCK: OnceLock<Mutex<()>> = OnceLock::new();
fn env_lock() -> std::sync::MutexGuard<'static, ()> {
ENV_LOCK
.get_or_init(|| Mutex::new(()))
.lock()
.expect("skills_sync_memory_test env lock poisoned")
}
// ── Helpers ──────────────────────────────────────────────────────────────────
fn try_find_skills_dir() -> Option<PathBuf> {
if let Ok(dir) = std::env::var("SKILL_DEBUG_DIR") {
let p = PathBuf::from(&dir);
return if p.exists() { Some(p) } else { None };
}
if let Ok(dir) = std::env::var("SKILLS_LOCAL_DIR") {
let p = PathBuf::from(&dir);
if p.exists() {
return Some(p);
}
}
let cwd = std::env::current_dir().expect("cwd");
for candidate in &[
"../openhuman-skills/skills",
"openhuman-skills/skills",
"../alphahuman/skills/skills",
] {
let p = cwd.join(candidate);
if p.exists() {
return Some(p.canonicalize().unwrap());
}
}
if let Some(parent) = cwd.parent() {
for entry in std::fs::read_dir(parent).into_iter().flatten().flatten() {
let c = entry.path().join("skills/skills");
if c.join("example-skill/manifest.json").exists() {
return Some(c.canonicalize().unwrap());
}
}
}
None
}
macro_rules! require_skills_dir {
() => {
match try_find_skills_dir() {
Some(dir) => dir,
None => {
eprintln!("SKIPPED: no skills directory available (set SKILL_DEBUG_DIR for CI)");
return;
}
}
};
}
async fn create_engine_with_memory(
skills_dir: &Path,
data_dir: &Path,
workspace_dir: &Path,
) -> (Arc<RuntimeEngine>, Arc<MemoryClient>) {
let engine =
RuntimeEngine::new(data_dir.to_path_buf()).expect("RuntimeEngine::new should succeed");
let engine = Arc::new(engine);
engine.set_skills_source_dir(skills_dir.to_path_buf());
// Create a MemoryClient pointing at the temp workspace
let memory_client =
MemoryClient::from_workspace_dir(workspace_dir.to_path_buf()).expect("MemoryClient");
let memory_client = Arc::new(memory_client);
// Wire the memory client into the engine so event_loop can use it
engine.set_memory_client(memory_client.clone());
// Set as global so RPC handlers can find it
set_global_engine(engine.clone());
(engine, memory_client)
}
// ── Tests ────────────────────────────────────────────────────────────────────
/// Test that `skill/sync` RPC triggers `onSync()` and persists state to memory.
#[tokio::test]
async fn sync_rpc_persists_to_memory() {
let _lock = env_lock();
let _ = env_logger::builder()
.filter_level(log::LevelFilter::Debug)
.is_test(true)
.try_init();
let skills_dir = require_skills_dir!();
let tmp = tempdir().expect("tempdir");
let data_dir = tmp.path().join("skills_data");
let workspace_dir = tmp.path().join("workspace");
std::fs::create_dir_all(&data_dir).unwrap();
std::fs::create_dir_all(&workspace_dir).unwrap();
let skill_id = "example-skill";
eprintln!("\n=== sync_rpc_persists_to_memory ===");
eprintln!(" Skills dir: {}", skills_dir.display());
eprintln!(" Data dir: {}", data_dir.display());
eprintln!(" Workspace dir: {}", workspace_dir.display());
let (engine, memory_client) =
create_engine_with_memory(&skills_dir, &data_dir, &workspace_dir).await;
// ── Start skill ──
eprintln!("\n--- Start skill '{skill_id}' ---");
let snap = engine.start_skill(skill_id).await.expect("start");
eprintln!(" Status: {:?}, tools: {}", snap.status, snap.tools.len());
eprintln!(
" Published state keys: {:?}",
snap.state.keys().collect::<Vec<_>>()
);
// ── Verify no memory documents exist yet ──
eprintln!("\n--- Check memory before sync ---");
let namespace = format!("skill-{}", skill_id);
let before = memory_client
.list_documents(Some(&namespace))
.await
.expect("list_documents");
let before_count = before
.get("documents")
.and_then(|d| d.as_array())
.map(|a| a.len())
.unwrap_or(0);
eprintln!(" Documents in '{namespace}' before sync: {before_count}");
// ── Call skill/sync via RPC ──
eprintln!("\n--- Call skill/sync RPC ---");
let sync_result = tokio::time::timeout(
Duration::from_secs(30),
engine.rpc(skill_id, "skill/sync", json!({})),
)
.await;
match &sync_result {
Ok(Ok(val)) => eprintln!(" skill/sync returned: {val}"),
Ok(Err(e)) => eprintln!(" skill/sync error: {e} (may be expected for example-skill)"),
Err(_) => panic!("skill/sync TIMED OUT"),
}
// ── Wait for fire-and-forget memory persistence ──
eprintln!("\n--- Waiting for memory persistence (2s) ---");
tokio::time::sleep(Duration::from_secs(2)).await;
// ── Verify documents were created ──
eprintln!("\n--- Check memory after sync ---");
let after = memory_client
.list_documents(Some(&namespace))
.await
.expect("list_documents");
let after_docs = after
.get("documents")
.and_then(|d| d.as_array())
.cloned()
.unwrap_or_default();
eprintln!(
" Documents in '{namespace}' after sync: {}",
after_docs.len()
);
for doc in &after_docs {
let title = doc.get("title").and_then(|t| t.as_str()).unwrap_or("?");
let doc_id = doc
.get("documentId")
.and_then(|d| d.as_str())
.unwrap_or("?");
eprintln!(" - title: {title}, doc_id: {doc_id}");
}
// The example-skill may or may not publish state during onSync.
// If it does, we should see at least one document.
// If it doesn't (no state.set calls in onSync), the snapshot will be empty
// and no document is created — that's correct behavior.
//
// We check published_state to know what to expect.
let final_state = engine.get_skill_state(skill_id);
let has_published_state = final_state
.as_ref()
.map(|s| !s.state.is_empty())
.unwrap_or(false);
eprintln!(
" Skill has published state: {} ({} keys)",
has_published_state,
final_state.as_ref().map(|s| s.state.len()).unwrap_or(0)
);
if has_published_state {
assert!(
after_docs.len() > before_count,
"Expected at least one new document in namespace '{namespace}' after sync, \
but found {} (before: {before_count}). Published state is non-empty, \
so store_skill_sync should have been called.",
after_docs.len()
);
eprintln!(" PASS: Memory document created after sync");
} else {
eprintln!(" NOTE: Skill has no published state — no memory write expected");
eprintln!(" (This is correct behavior; persist_state_to_memory skips empty state)");
}
// ── Cleanup ──
let _ = engine.stop_skill(skill_id).await;
eprintln!("\n=== sync_rpc_persists_to_memory COMPLETE ===\n");
}
/// Test that `skill/tick` also persists state to memory.
#[tokio::test]
async fn tick_persists_to_memory() {
let _lock = env_lock();
let _ = env_logger::builder()
.filter_level(log::LevelFilter::Debug)
.is_test(true)
.try_init();
let skills_dir = require_skills_dir!();
let tmp = tempdir().expect("tempdir");
let data_dir = tmp.path().join("skills_data");
let workspace_dir = tmp.path().join("workspace");
std::fs::create_dir_all(&data_dir).unwrap();
std::fs::create_dir_all(&workspace_dir).unwrap();
let skill_id = "example-skill";
eprintln!("\n=== tick_persists_to_memory ===");
let (engine, memory_client) =
create_engine_with_memory(&skills_dir, &data_dir, &workspace_dir).await;
// Start skill
let snap = engine.start_skill(skill_id).await.expect("start");
eprintln!(" Started: {:?}", snap.status);
// Call skill/tick
let tick_result = tokio::time::timeout(
Duration::from_secs(15),
engine.rpc(skill_id, "skill/tick", json!({})),
)
.await;
match &tick_result {
Ok(Ok(val)) => eprintln!(" skill/tick returned: {val}"),
Ok(Err(e)) => eprintln!(" skill/tick error: {e}"),
Err(_) => panic!("skill/tick TIMED OUT"),
}
// Wait for persistence
tokio::time::sleep(Duration::from_secs(2)).await;
// Check memory
let namespace = format!("skill-{}", skill_id);
let docs = memory_client
.list_documents(Some(&namespace))
.await
.expect("list_documents");
let doc_count = docs
.get("documents")
.and_then(|d| d.as_array())
.map(|a| a.len())
.unwrap_or(0);
eprintln!(" Documents in '{namespace}' after tick: {doc_count}");
let has_published_state = engine
.get_skill_state(skill_id)
.map(|s| !s.state.is_empty())
.unwrap_or(false);
if has_published_state {
assert!(
doc_count > 0,
"Expected memory documents after tick with published state"
);
eprintln!(" PASS: Memory persisted after tick");
} else {
eprintln!(" NOTE: No published state — no memory write expected");
}
let _ = engine.stop_skill(skill_id).await;
eprintln!("=== tick_persists_to_memory COMPLETE ===\n");
}
/// Verify that the `skills_sync` RPC schema routes to `skill/sync` (not `skill/tick`).
/// This is a regression test for the routing bug where `handle_skills_sync` sent
/// `"skill/tick"` instead of `"skill/sync"`.
#[tokio::test]
async fn skills_sync_rpc_calls_on_sync_not_on_tick() {
let _lock = env_lock();
let _ = env_logger::builder()
.filter_level(log::LevelFilter::Debug)
.is_test(true)
.try_init();
let skills_dir = require_skills_dir!();
let tmp = tempdir().expect("tempdir");
let data_dir = tmp.path().join("skills_data");
let workspace_dir = tmp.path().join("workspace");
std::fs::create_dir_all(&data_dir).unwrap();
std::fs::create_dir_all(&workspace_dir).unwrap();
let skill_id = "example-skill";
eprintln!("\n=== skills_sync_rpc_calls_on_sync_not_on_tick ===");
let (engine, memory_client) =
create_engine_with_memory(&skills_dir, &data_dir, &workspace_dir).await;
let snap = engine.start_skill(skill_id).await.expect("start");
eprintln!(" Started: {:?}", snap.status);
// Exercise the full controller path via try_invoke_registered_rpc so we
// verify handle_skills_sync routes to "skill/sync" (not "skill/tick").
// This catches regressions in the controller layer that engine.rpc() would bypass.
let rpc_params: serde_json::Map<String, serde_json::Value> =
serde_json::from_value(json!({"skill_id": skill_id})).unwrap();
let sync_result = tokio::time::timeout(
Duration::from_secs(15),
try_invoke_registered_rpc("openhuman.skills_sync", rpc_params),
)
.await;
match &sync_result {
Ok(Some(Ok(val))) => {
eprintln!(" openhuman.skills_sync returned: {val}");
// The result comes from handle_js_call("onSync") which returns the
// JS value (typically null/undefined). The old buggy path returned
// {"ok": true} from handle_js_void_call("onTick").
}
Ok(Some(Err(e))) => {
eprintln!(" openhuman.skills_sync error: {e}");
// An error from onSync not being defined is still acceptable —
// the important thing is it tried onSync, not onTick.
}
Ok(None) => {
panic!("openhuman.skills_sync not found in registered controllers");
}
Err(_) => panic!("openhuman.skills_sync TIMED OUT"),
}
// Also verify the namespace gets a title with "periodic sync" (not "tick sync")
tokio::time::sleep(Duration::from_secs(2)).await;
let namespace = format!("skill-{}", skill_id);
let docs = memory_client
.list_documents(Some(&namespace))
.await
.expect("list_documents");
if let Some(doc_array) = docs.get("documents").and_then(|d| d.as_array()) {
for doc in doc_array {
let title = doc.get("title").and_then(|t| t.as_str()).unwrap_or("?");
eprintln!(" Memory doc title: {title}");
assert!(
title.contains("periodic sync"),
"Expected title to contain 'periodic sync' (from skill/sync handler), got: {title}"
);
}
}
let _ = engine.stop_skill(skill_id).await;
eprintln!("=== skills_sync_rpc_calls_on_sync_not_on_tick COMPLETE ===\n");
}