fix(skills): route sync RPC to onSync handler and persist state to memory#183
Merged
Conversation
…ce 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.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
📝 WalkthroughWalkthroughAdds bounded background memory-write worker and helper invoked after cron/tick/sync handlers, routes skills RPCs to Changes
Sequence DiagramsequenceDiagram
actor Trigger as Trigger (Cron / Tick / RPC)
participant EventLoop as Event Loop
participant QJS as QJS Skill Instance
participant Helper as persist_state_to_memory()
participant Worker as Memory Write Worker
participant Memory as MemoryClient
Trigger->>EventLoop: deliver message (cron/tick/rpc)
EventLoop->>QJS: invoke handler (onCron/onTick/onSync)
QJS-->>EventLoop: handler returns (Ok / Err)
alt handler Ok
EventLoop->>Helper: call with ops_state & title_suffix
Helper->>Worker: try_send MemoryWriteJob (bounded channel)
Worker->>Memory: store_skill_sync(job) (background)
Memory-->>Worker: success/failure
Worker-->>EventLoop: log result
else handler Err
EventLoop-->>EventLoop: skip persistence (warn)
end
EventLoop-->>Trigger: complete
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Member
Author
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/openhuman/skills/schemas.rs (1)
772-780:⚠️ Potential issue | 🟡 MinorAdd a debug checkpoint log for the new
skills.syncroute.The routing fix is correct, but this changed critical path currently has no trace/debug log to help diagnose sync routing issues at runtime.
💡 Proposed patch
fn handle_skills_sync(params: Map<String, Value>) -> ControllerFuture { Box::pin(async move { let p: SkillIdParams = serde_json::from_value(Value::Object(params)).map_err(|e| e.to_string())?; let engine = require_engine()?; + log::debug!( + "[skills.sync] routing skill '{}' to RPC method 'skill/sync'", + p.skill_id + ); engine .rpc(&p.skill_id, "skill/sync", serde_json::json!({})) .await }) }As per coding guidelines,
src/**/*.rs: Use log/tracing at debug or trace level in Rust for critical checkpoints (entry/exit points, branch decisions, external calls, retries/timeouts, state transitions, error handling paths).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/openhuman/skills/schemas.rs` around lines 772 - 780, The new skills.sync route handler handle_skills_sync lacks any debug/tracing logs; add a debug-level tracing/log::debug call at the entry of handle_skills_sync that logs the incoming SkillIdParams (or at least p.skill_id) after deserialization and another debug before calling engine.rpc (including p.skill_id and the method "skill/sync"); use the existing require_engine and engine.rpc symbols to locate the code and ensure logs avoid sensitive data and use debug/trace level per project guidelines.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/openhuman/skills/qjs_skill_instance/event_loop.rs`:
- Around line 324-331: The code currently calls persist_state_to_memory
unconditionally after calling lifecycle handlers like handle_cron_trigger, which
can persist stale state on errors; change the lifecycle invocations (e.g.,
handle_cron_trigger and the onSync/onTick calls referenced) to return/propagate
a Result and only call persist_state_to_memory when the call succeeded (i.e.,
match or if let Ok(_) for the Result). Specifically, update the call sites that
pass skill_id, ops_state, memory_client and the schedule_id log (the lines
around handle_cron_trigger and the other onSync/onTick blocks) to check the
handler result before invoking persist_state_to_memory so persistence happens
only on success. Ensure error branches do not call persist_state_to_memory and
still log/handle the error appropriately.
- Around line 51-63: The current fire-and-forget tokio::spawn in
persist_state_to_memory (the block that calls client.store_skill_sync(...)) can
accumulate tasks; change this to use a bounded concurrency mechanism such as a
shared JoinSet or a bounded mpsc channel with a single background worker to
apply backpressure: create a task queue for store_skill_sync calls, push the
title/content payloads into the channel (or submit into a JoinSet with a max
concurrency limit) and have a dedicated task consume and await
client.store_skill_sync so you limit concurrent in-flight writes; preserve the
existing logging (debug/info/warn) around client.store_skill_sync and ensure
errors are awaited/handled rather than leaked by unbounded tokio::spawn.
In `@tests/skills_sync_memory_test.rs`:
- Around line 82-83: Tests are race-prone because multiple tests call
set_global_engine(...) which mutates the process-wide GLOBAL_ENGINE
concurrently; fix by serializing these tests the same way other files do (e.g.,
annotate the test functions or the test module with the serialization attribute
used elsewhere such as #[serial] or #[serial_test::serial]) so only one test
mutates GLOBAL_ENGINE at a time; update tests in skills_sync_memory_test.rs to
use that serial attribute on the affected tests or module to prevent
cross-wiring of engine instances.
- Around line 317-357: The test currently calls engine.rpc(skill_id,
"skill/sync", ...) directly which bypasses the controller and won't catch a
regression in handle_skills_sync; replace that direct call with invoking the
registered controller via try_invoke_registered_rpc("openhuman.skills_sync",
params) (or the test helper that invokes registered RPC handlers) using the same
parameters/skill_id payload so the test exercises handle_skills_sync and
verifies it routes "skill/sync" instead of "skill/tick" (update the section
around the engine.rpc call and ensure the subsequent assertions remain the
same).
---
Outside diff comments:
In `@src/openhuman/skills/schemas.rs`:
- Around line 772-780: The new skills.sync route handler handle_skills_sync
lacks any debug/tracing logs; add a debug-level tracing/log::debug call at the
entry of handle_skills_sync that logs the incoming SkillIdParams (or at least
p.skill_id) after deserialization and another debug before calling engine.rpc
(including p.skill_id and the method "skill/sync"); use the existing
require_engine and engine.rpc symbols to locate the code and ensure logs avoid
sensitive data and use debug/trace level per project guidelines.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d6583312-530b-4a2b-84ed-9f290a85b56f
📒 Files selected for processing (6)
scripts/debug-notion-sync-memory.shsrc/openhuman/skills/qjs_skill_instance/event_loop.rssrc/openhuman/skills/schemas.rstests/skills_debug_e2e.rstests/skills_notion_live.rstests/skills_sync_memory_test.rs
…e 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.
- 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.
- 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.
- 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.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/skills_sync_memory_test.rs (1)
36-61: Improve robustness of skills directory discovery.The hardcoded relative paths may not work in all CI/development environments. Consider documenting the expected directory structure or failing more explicitly when the directory is not found.
The current fallback logic is reasonable for local development but could be fragile.
💡 Suggestion: Add a clearer error message
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 p.exists() { + return Some(p); + } + eprintln!("SKILL_DEBUG_DIR={} does not exist", dir); + return None; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/skills_sync_memory_test.rs` around lines 36 - 61, Update try_find_skills_dir to return Result<PathBuf, anyhow::Error> (or Result<PathBuf, String>) instead of Option and produce a clear error when the skills directory cannot be located: keep the existing SKILL_DEBUG_DIR check and candidate search (the candidate array and cwd/parent search using std::env::current_dir and std::fs::read_dir), but collect the attempted paths and include them plus the SKILL_DEBUG_DIR value in the Err returned (use canonicalize when returning a found path); this makes callers of try_find_skills_dir receive an explicit failure with a descriptive message rather than silently getting None.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/skills_sync_memory_test.rs`:
- Around line 369-378: The test iterates over documents and asserts each title
contains "periodic sync", which will either be a no-op if doc_array is empty or
falsely fail if unrelated docs exist; update the check in
tests/skills_sync_memory_test.rs so that you first handle the empty-case (allow
empty doc_array when the skill publishes no state) and otherwise assert that at
least one document matches the expected "periodic sync" condition (or assert
each present doc is from the sync handler) by guarding the loop with an if/else
or using a any() check over doc_array titles to confirm existence of a "periodic
sync" title rather than blindly asserting every title contains that substring.
---
Nitpick comments:
In `@tests/skills_sync_memory_test.rs`:
- Around line 36-61: Update try_find_skills_dir to return Result<PathBuf,
anyhow::Error> (or Result<PathBuf, String>) instead of Option and produce a
clear error when the skills directory cannot be located: keep the existing
SKILL_DEBUG_DIR check and candidate search (the candidate array and cwd/parent
search using std::env::current_dir and std::fs::read_dir), but collect the
attempted paths and include them plus the SKILL_DEBUG_DIR value in the Err
returned (use canonicalize when returning a found path); this makes callers of
try_find_skills_dir receive an explicit failure with a descriptive message
rather than silently getting None.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 96af3c05-eb72-469c-b869-77752271997c
📒 Files selected for processing (4)
src/openhuman/skills/qjs_skill_instance/event_loop.rssrc/openhuman/skills/schemas.rstests/json_rpc_e2e.rstests/skills_sync_memory_test.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- src/openhuman/skills/schemas.rs
Comment on lines
+369
to
+378
| 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}" | ||
| ); | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Assertion may incorrectly fail when skill publishes no state.
If the skill has no published state after sync, persist_state_to_memory correctly skips the write (empty snapshot → early return). However, this assertion iterates over doc_array which could be empty, meaning no assertion runs. If a document is present from a previous test artifact or other source, the assertion would fail on a non-"periodic sync" title.
Consider adding a guard or adjusting the logic:
💡 Proposed fix
let namespace = format!("skill-{}", skill_id);
let docs = memory_client
.list_documents(Some(&namespace))
.await
.expect("list_documents");
+ let has_published_state = engine
+ .get_skill_state(skill_id)
+ .map(|s| !s.state.is_empty())
+ .unwrap_or(false);
+
if let Some(doc_array) = docs.get("documents").and_then(|d| d.as_array()) {
+ if has_published_state {
+ assert!(
+ !doc_array.is_empty(),
+ "Expected memory documents after sync with published state"
+ );
+ }
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}"
);
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/skills_sync_memory_test.rs` around lines 369 - 378, The test iterates
over documents and asserts each title contains "periodic sync", which will
either be a no-op if doc_array is empty or falsely fail if unrelated docs exist;
update the check in tests/skills_sync_memory_test.rs so that you first handle
the empty-case (allow empty doc_array when the skill publishes no state) and
otherwise assert that at least one document matches the expected "periodic sync"
condition (or assert each present doc is from the sync handler) by guarding the
loop with an if/else or using a any() check over doc_array titles to confirm
existence of a "periodic sync" title rather than blindly asserting every title
contains that substring.
- 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.
- 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.
- 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.
… 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.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
tests/skills_sync_memory_test.rs (1)
375-384:⚠️ Potential issue | 🟡 MinorAssertion logic still has edge-case gaps.
This loop has two issues:
- Empty
doc_array: If no documents exist, the loop body never runs and the test passes without validating anything.- Unrelated documents: If documents exist from other sources (e.g., prior test artifacts), the assertion fails on non-"periodic sync" titles.
Consider guarding with an explicit check:
🛠️ Proposed fix
if let Some(doc_array) = docs.get("documents").and_then(|d| d.as_array()) { + // Only validate titles if skill published state (otherwise no docs expected) + let has_published_state = engine + .get_skill_state(skill_id) + .map(|s| !s.state.is_empty()) + .unwrap_or(false); + + if has_published_state && !doc_array.is_empty() { + // At least one doc should contain "periodic sync" from onSync handler + let has_sync_doc = doc_array.iter().any(|doc| { + doc.get("title") + .and_then(|t| t.as_str()) + .map(|t| t.contains("periodic sync")) + .unwrap_or(false) + }); + assert!( + has_sync_doc, + "Expected at least one document with 'periodic sync' title from skill/sync handler" + ); + } + 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}" - ); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/skills_sync_memory_test.rs` around lines 375 - 384, The loop over docs.get("documents").and_then(|d| d.as_array()) can pass vacuously or fail on unrelated docs; change the logic to first assert that doc_array is Some and not empty, then iterate and collect titles (from the existing doc.get("title") -> title variable) and assert that at least one title contains "periodic sync" rather than asserting every title; update the code around docs/doc_array/title to return a clear test failure if no matching title is found.
🧹 Nitpick comments (1)
src/openhuman/skills/registry_ops.rs (1)
311-338: Synchronous recursive copy may block the async runtime.
copy_dir_recursiveuses synchronousstd::fsoperations. When called fromskill_install(an async function), this could block the Tokio runtime thread if copying large skill directories.Consider using
tokio::task::spawn_blockingortokio::fsfor the copy operation:♻️ Async-safe alternative
+async fn copy_dir_recursive_async(src: &Path, dst: &Path) -> Result<(), String> { + let src = src.to_path_buf(); + let dst = dst.to_path_buf(); + tokio::task::spawn_blocking(move || copy_dir_recursive(&src, &dst)) + .await + .map_err(|e| format!("copy task panicked: {e}"))? +}Then in
skill_install:- copy_dir_recursive(&local_skill, &skill_dir)?; + copy_dir_recursive_async(&local_skill, &skill_dir).await?;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/openhuman/skills/registry_ops.rs` around lines 311 - 338, copy_dir_recursive performs blocking std::fs operations and can stall the async runtime when called from the async function skill_install; move the whole recursive copy work into a blocking task or convert it to async: either (a) wrap the current copy_dir_recursive call with tokio::task::spawn_blocking and await the join, or (b) rewrite copy_dir_recursive to use tokio::fs async APIs (tokio::fs::create_dir_all, read_dir, copy, etc.) and call that from skill_install; update references to the function name copy_dir_recursive (or replace with an async copy_dir_recursive_async) and ensure proper error mapping back to the existing Result<String, ...> usage in skill_install.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/openhuman/skills/registry_ops.rs`:
- Around line 27-30: The is_local_path function only checks for Unix '/' and
"file://" URIs so Windows absolute paths (e.g. "C:\...") are misclassified;
update is_local_path to also detect Windows drive-letter absolute paths by
either using std::path::Path::new(url).is_absolute() or adding a drive-letter
check (match r"^[A-Za-z]:[\\/]"). Keep existing checks for '/' and "file://",
but ensure Path::is_absolute or the regex covers Windows formats (both backslash
and forward slash) so Windows registry file paths are treated as local.
---
Duplicate comments:
In `@tests/skills_sync_memory_test.rs`:
- Around line 375-384: The loop over docs.get("documents").and_then(|d|
d.as_array()) can pass vacuously or fail on unrelated docs; change the logic to
first assert that doc_array is Some and not empty, then iterate and collect
titles (from the existing doc.get("title") -> title variable) and assert that at
least one title contains "periodic sync" rather than asserting every title;
update the code around docs/doc_array/title to return a clear test failure if no
matching title is found.
---
Nitpick comments:
In `@src/openhuman/skills/registry_ops.rs`:
- Around line 311-338: copy_dir_recursive performs blocking std::fs operations
and can stall the async runtime when called from the async function
skill_install; move the whole recursive copy work into a blocking task or
convert it to async: either (a) wrap the current copy_dir_recursive call with
tokio::task::spawn_blocking and await the join, or (b) rewrite
copy_dir_recursive to use tokio::fs async APIs (tokio::fs::create_dir_all,
read_dir, copy, etc.) and call that from skill_install; update references to the
function name copy_dir_recursive (or replace with an async
copy_dir_recursive_async) and ensure proper error mapping back to the existing
Result<String, ...> usage in skill_install.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7487f606-5d17-458e-b5de-b2f164bf21c3
📒 Files selected for processing (7)
.env.examplesrc/openhuman/skills/qjs_engine.rssrc/openhuman/skills/registry_ops.rstests/skills_debug_e2e.rstests/skills_notion_live.rstests/skills_rpc_e2e.rstests/skills_sync_memory_test.rs
✅ Files skipped from review due to trivial changes (1)
- tests/skills_debug_e2e.rs
Comment on lines
+27
to
+30
| /// Check if a URL is a local file path (absolute path or file:// URI). | ||
| fn is_local_path(url: &str) -> bool { | ||
| url.starts_with('/') || url.starts_with("file://") | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if Windows support is expected/documented
rg -i 'windows|win32|msvc' Cargo.toml README.md .github/ -l 2>/dev/null | head -5
rg -n 'cfg.*windows' src/openhuman/skills/Repository: tinyhumansai/openhuman
Length of output: 182
🏁 Script executed:
# Check Cargo.toml for supported targets
cat Cargo.toml | grep -A 20 -E '\[.*target.*\]|target\..*|windows|win32'Repository: tinyhumansai/openhuman
Length of output: 794
🏁 Script executed:
# Check README.md for Windows references context
rg -B 2 -A 2 -i 'windows' README.mdRepository: tinyhumansai/openhuman
Length of output: 711
🏁 Script executed:
# Check how is_local_path is actually used in the codebase
rg 'is_local_path' src/ -B 2 -A 2Repository: tinyhumansai/openhuman
Length of output: 1230
🏁 Script executed:
# Check if there are other path handling functions that might indicate Windows support approach
rg 'path|Path|PathBuf|normalize' src/openhuman/skills/registry_ops.rs -n | head -20Repository: tinyhumansai/openhuman
Length of output: 1289
🏁 Script executed:
# Check the installer-smoke test to see if it tests Windows path handling
rg -B 5 -A 5 'installer-smoke' .github/workflows/ -A 20Repository: tinyhumansai/openhuman
Length of output: 48
🏁 Script executed:
# Check if there's any Windows path handling elsewhere in the codebase
rg 'cfg.*windows|target_os.*windows|\\\\|drive|C:' src/ --type rsRepository: tinyhumansai/openhuman
Length of output: 92
🏁 Script executed:
# Check the skill_install function to see how paths are actually passed to is_local_path
sed -n '233,260p' src/openhuman/skills/registry_ops.rsRepository: tinyhumansai/openhuman
Length of output: 1377
🏁 Script executed:
# Check environment variable usage for paths on Windows
rg 'SKILLS_LOCAL_DIR|env::var' src/ -B 2 -A 2 --type rsRepository: tinyhumansai/openhuman
Length of output: 92
🏁 Script executed:
# Check how registry_fetch is called to understand if Windows paths could be passed
rg 'registry_fetch' src/ -B 3 -A 3Repository: tinyhumansai/openhuman
Length of output: 4706
🏁 Script executed:
# Check if file:// URI handling works on Windows for file paths
rg 'file://' src/ -B 2 -A 2Repository: tinyhumansai/openhuman
Length of output: 3099
🏁 Script executed:
# Check if there are any other path normalization utilities in the codebase
rg 'normalize|canonicalize|path.*unix|path.*windows' src/ -iRepository: tinyhumansai/openhuman
Length of output: 45613
Windows users cannot use absolute paths for local registry files. The is_local_path() function only recognizes Unix absolute paths (/) and file:// URIs. Windows paths like C:\path\to\registry.json will be treated as remote URLs, causing HTTP fetch failures.
Consider supporting Windows drive letters:
fn is_local_path(url: &str) -> bool {
- url.starts_with('/') || url.starts_with("file://")
+ url.starts_with('/')
+ || url.starts_with("file://")
+ || (cfg!(windows) && url.len() > 1 && url.chars().nth(1) == Some(':'))
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| /// Check if a URL is a local file path (absolute path or file:// URI). | |
| fn is_local_path(url: &str) -> bool { | |
| url.starts_with('/') || url.starts_with("file://") | |
| } | |
| /// Check if a URL is a local file path (absolute path or file:// URI). | |
| fn is_local_path(url: &str) -> bool { | |
| url.starts_with('/') | |
| || url.starts_with("file://") | |
| || (cfg!(windows) && url.len() > 1 && url.chars().nth(1) == Some(':')) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/openhuman/skills/registry_ops.rs` around lines 27 - 30, The is_local_path
function only checks for Unix '/' and "file://" URIs so Windows absolute paths
(e.g. "C:\...") are misclassified; update is_local_path to also detect Windows
drive-letter absolute paths by either using
std::path::Path::new(url).is_absolute() or adding a drive-letter check (match
r"^[A-Za-z]:[\\/]"). Keep existing checks for '/' and "file://", but ensure
Path::is_absolute or the regex covers Windows formats (both backslash and
forward slash) so Windows registry file paths are treated as local.
9 tasks
This was referenced Apr 6, 2026
AusAgentSmith
pushed a commit
to AusAgentSmith/openhuman
that referenced
this pull request
May 23, 2026
…mory (tinyhumansai#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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
handle_skills_sync()was sending"skill/tick"instead of"skill/sync", causing sync requests to callonTick()(which most skills don't implement) instead ofonSync(), and completely skipping memory persistencepersist_state_to_memory()helper to eliminate code duplication across the three sync pathsRoot Cause
schemas.rs:778mappedopenhuman.skills_syncRPC →"skill/tick"→SkillMessage::Tick→onTick()(no memory write). The correct"skill/sync"handler existed in the event loop with full memory persistence but was dead code — nothing ever routed to it.Test plan
cargo checkpassescargo test --test skills_sync_memory_test— 3 tests pass (sync, tick, routing regression)cargo test --test skills_debug_e2e— full lifecycle with new sync step passesRUN_LIVE_NOTION=1 cargo test --test skills_notion_live -- --nocapture— live Notion sync writes to memorybash scripts/debug-notion-sync-memory.sh— end-to-end live verification🤖 Generated with Claude Code
Co-Authored-By: Claude Opus 4.6 (1M context) noreply@anthropic.com
Summary by CodeRabbit
Tests
New Features
Bug Fixes
Chores