Feat/docs

[senamakel]

Summary

• What changed and why.
• Keep this to 3-6 bullets focused on user-visible or architecture-impacting changes.

Problem

• What issue or risk this PR addresses.
• Include context needed for reviewers to evaluate correctness quickly.

Solution

• How the implementation solves the problem.
• Note important design decisions and tradeoffs.

Submission Checklist

• Unit tests — Vitest (app/) and/or cargo test (core) for logic you add or change
• E2E / integration — Where behavior is user-visible or crosses UI → Tauri → sidecar → JSON-RPC; use existing harnesses (app/test/e2e, mock backend, tests/json_rpc_e2e.rs as appropriate)
• N/A — If truly not applicable, say why (e.g. change is documentation-only)
• Doc comments — /// / //! (Rust), JSDoc or brief file/module headers (TS) on public APIs and non-obvious modules
• Inline comments — Where logic, invariants, or edge cases aren’t clear from names alone (keep them grep-friendly; avoid restating the code)

(Any feature related checklist can go in here)

Impact

• Runtime/platform impact (desktop/mobile/web/CLI), if any.
• Performance, security, migration, or compatibility implications.

Related

• Issue(s):
• Follow-up PR(s)/TODOs:

Summary by CodeRabbit

• New Features

  • Memory ingestion API returning typed extraction results; new memory RPCs and file operations.
  • Skills management: initialize and load skills directory with richer skill metadata.

• Enhancements

  • Background ingestion queue for async document processing.
  • Improved memory retrieval/context, relations, and deterministic schema ordering.
  • CLI: startup banner, improved help handling, and expanded subcommand dispatch.

• Documentation

  • Large-scale module- and API-level docs across core, memory, and skills.

[coderabbitai]

Caution

Review failed

The pull request is closed.

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 4329935b-f7a0-4d4f-9522-a728c5292f94

📥 Commits

Reviewing files that changed from the base of the PR and between e064016 and 6547558.

📒 Files selected for processing (3)

• src/core/jsonrpc.rs
• src/openhuman/skills/qjs_skill_instance/event_loop/mod.rs
• src/openhuman/skills/socket_manager.rs

---

📝 Walkthrough

Walkthrough

Adds extensive documentation across the codebase, expands CLI and JSON-RPC invocation/dispatch logic, introduces a typed memory ingestion API with background ingestion queue and new memory RPC endpoints, and augments skills subsystem (manifest validation, skill loading, QuickJS ops, timers, and runtime helpers).

Changes

Cohort / File(s)	Summary
Core CLI & server src/core/cli.rs, src/core/jsonrpc.rs, src/lib.rs, src/main.rs	Added CLI banner/help handling and manual subcommand flag parsing; expanded JSON‑RPC server docs, request dispatch, schema-based param parsing, SSE/Telegram handlers, router construction, and server bootstrap comments.
Memory ingestion & queue src/openhuman/memory/ingestion.rs, src/openhuman/memory/ingestion_queue.rs, src/openhuman/memory/store/client.rs	Introduced typed ingestion types (ExtractionMode, MemoryIngestionConfig/Request/Result), UnifiedMemory ingest/extract methods, IngestionQueue worker docs, and MemoryClient constructors that enqueue background ingestion jobs.
Memory RPC surface & ops src/openhuman/memory/ops.rs, src/openhuman/memory/rpc_models.rs, src/openhuman/memory/schemas.rs	Added many memory RPC endpoints and ApiEnvelope/ApiMeta DTOs, request/response models, resolved_limit helpers, richer retrieval context formatting, and documented schema helpers.
Memory models, traits & store factories src/openhuman/memory/traits.rs, src/openhuman/memory/store/factories.rs, src/openhuman/memory/store/memory_trait.rs, src/openhuman/memory/store/mod.rs	Documented Memory trait/UnifiedMemory mapping, added factory helpers to construct memory with embedding providers, and clarified timestamp helper behavior.
Embeddings, relex & chunking docs src/openhuman/memory/embeddings.rs, src/openhuman/memory/relex.rs, src/openhuman/memory/chunker.rs, src/openhuman/memory/mod.rs	Large documentation expansion describing embedding providers, ONNX/GLiNER inference, semantic chunking strategy, and module-level memory overview; no signature changes.
Memory cache & response models src/openhuman/memory/response_cache.rs, src/openhuman/memory/rpc_models.rs	Expanded response-cache docs and behavior commentary (TTL/LRU); added ApiEnvelope/ApiMeta types and many RPC model types and serde attributes; removed inline tests from cache file.
Skills: manifests, loading & registry models src/openhuman/skills/manifest.rs, src/openhuman/skills/ops.rs, src/openhuman/skills/registry_types.rs, src/openhuman/skills/registry_ops.rs, src/openhuman/skills/schemas.rs	Enforced non-empty auth.modes via custom deserializer; added exported Skill type and init/load helpers; enriched registry skill model fields/serde defaults; checksum cleanup on install failure and docs for registry RPCs.
Skills: QuickJS ops & event loop src/openhuman/skills/quickjs_libs/..., src/openhuman/skills/qjs_skill_instance/..., src/openhuman/skills/quickjs_libs/qjs_ops/ops_state.rs, src/openhuman/skills/quickjs_libs/qjs_ops/types.rs	Added typed JsMemoryInsertInput with validation/normalization (memory_insert now spawns background ingestion), timer entry fields (delay_ms, is_interval) and next-wait reporting; many event-loop, JS-handler, and helper doc updates and minor timeout/log changes.
Skills subsystem docs & utilities src/openhuman/skills/... (e.g., mod.rs, qjs_engine.rs, socket_manager.rs, types.rs, ping_scheduler.rs, preferences.rs, cron_scheduler.rs, utils.rs, bridge/*, quickjs_libs/*, registry_cache.rs)	Widespread documentation additions clarifying lifecycle, scheduling, preferences resolution, ping semantics, and runtime characteristics; small behavioral clarifications (serde default for setup flag, write-ignore on managed auth persistence, preference save ordering).

Sequence Diagram(s)

sequenceDiagram
    participant Client
    participant HTTP as HTTP Router
    participant RPC as rpc_handler
    participant Invoker as invoke_method
    participant Schema as Schema Store
    participant Memory as UnifiedMemory
    participant Queue as IngestionQueue Worker

    Client->>HTTP: POST /rpc {method, params}
    HTTP->>RPC: route to rpc_handler
    RPC->>Invoker: invoke_method(state, method, params)
    Invoker->>Schema: lookup/validate params (schema)
    alt method is memory/doc.ingest or memory.doc_put
      Invoker->>Memory: call memory RPC (put_doc / ingest_document)
      Memory->>Queue: enqueue ingestion job (put_doc -> send to tx)
      Queue->>Queue: worker rx.recv() -> extract_graph -> persist results
      Queue-->>Memory: persistence/ingest result
    else handled by registered controller
      Invoker->>Invoker: dispatch to registered controller or core::dispatch
    end
    Invoker-->>RPC: return Result<Value, String>
    RPC-->>HTTP: assemble HTTP response
    HTTP-->>Client: 200 / error

Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~50 minutes

Possibly related PRs

• Feat/docs #331 — Extensive documentation changes across same core files; closely related.
• feat: background ingestion queue for memory graph extraction #325 — Adds/changes ingestion background queue and job integration; directly related to IngestionQueue and MemoryClient enqueueing.
• feat(memory): conversation segmentation, event extraction, and user profile accumulation #176 — Related to memory ingestion/retrieval surface and UnifiedMemory ingestion/recall helpers.

Poem

🐰 I hopped through docs and server lanes,
I seeded schemas, queued the brains,
Skills now bloom from files and code,
Ingestion hums along the road—
Happy hops for every new node! 🥕

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 inconclusive)

Check name	Status	Explanation	Resolution
Title check	❓ Inconclusive	The title 'Feat/docs' is vague and generic, using non-descriptive terms that don't convey meaningful information about the comprehensive documentation and API additions across the codebase.	Revise the title to be more specific about the main scope, such as 'Add comprehensive documentation and public APIs for memory, skills, and JSON-RPC systems' or similar to reflect the substantial changes across multiple modules.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 7

Note

Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (9)

src/openhuman/skills/types.rs (1)

1-493: ⚠️ Potential issue | 🔴 Critical

Run cargo fmt to fix formatting issues before merging.

The pipeline shows that cargo fmt --all -- --check failed with formatting differences at multiple locations in this file (lines 1, 10, 33, 55, 137, 172, 211, 238). As per coding guidelines, all Rust code changes must pass cargo fmt before merging.

Run the following to fix:

cargo fmt --all

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/openhuman/skills/types.rs` around lines 1 - 493, The PR failed CI because
this Rust file is not formatted with rustfmt; run "cargo fmt --all" and commit
the formatting changes so the file (including definitions like SkillStatus,
SkillMessage, ToolResult, derive_connection_status and the tests) matches the
project's formatting rules; ensure you re-run "cargo fmt --all" locally (or via
CI) to fix the spacing/line breaks reported and then add/commit the updated file
before pushing.

src/openhuman/memory/relex.rs (1)

1-993: ⚠️ Potential issue | 🔴 Critical

Run cargo fmt to fix formatting issues before merging.

The pipeline shows that cargo fmt --all -- --check failed with formatting differences at lines 1, 224, 291, and 307. As per coding guidelines, all Rust code changes must pass cargo fmt before merging.

Run the following to fix:

cargo fmt --all

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/openhuman/memory/relex.rs` around lines 1 - 993, The PR fails rustfmt
checks (formatting diffs at the top of the file and around functions like
split_whitespace_tokens, encode_prompt, and make_spans_tensors); fix by running
rustfmt/cargo fmt to reformat the file(s) and commit the resulting changes (run
`cargo fmt --all` locally or in CI), then re-run the formatter check to ensure
functions such as split_whitespace_tokens, encode_prompt, make_spans_tensors and
surrounding code are properly formatted per project style.

src/openhuman/skills/manifest.rs (2)

1-101: ⚠️ Potential issue | 🔴 Critical

Fix multiple formatting issues to pass CI.

The pipeline is failing due to cargo fmt differences at multiple locations in the documentation comments (lines 9, 20, 27, 65, 76, 85, 92). There are also blank line inconsistencies around lines 85 and 92.

🔧 Run cargo fmt to fix

cargo fmt --all

This will normalize the rustdoc comment formatting and blank lines throughout the file.

As per coding guidelines: src/**/*.rs: Run cargo fmt and cargo check before merging changes to Rust code.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/openhuman/skills/manifest.rs` around lines 1 - 101, Run rustfmt (cargo
fmt --all) and fix the formatting/blank-line issues in the manifest module so CI
passes; specifically normalize rustdoc and blank lines around the structs and
functions: SkillSetup, SkillAuthType, SkillAuthConfig,
deserialize_non_empty_modes, and SkillAuthMode (ensure doc comments wrap and
spacing matches rustfmt output and remove/add blank lines around fields as
needed), then run cargo check to verify.

---

163-211: ⚠️ Potential issue | 🔴 Critical

Fix formatting issues in method documentation.

Additional formatting issues are flagged at lines 175, 182, and 195 in the method documentation.

Run cargo fmt --all to resolve these alongside the earlier formatting issues.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/openhuman/skills/manifest.rs` around lines 163 - 211, The doc-comments
for SkillManifest methods (from_json, from_path, is_javascript,
supports_current_platform, to_config) contain formatting inconsistencies; run
rustfmt (cargo fmt --all) and reflow/align the /// comment blocks for those
methods to match project style so the triple-slash comments and blank lines are
consistent (especially around is_javascript, supports_current_platform, and
to_config) and then commit the formatted changes.

src/openhuman/skills/qjs_skill_instance/event_loop/webhook_handler.rs (1)

63-83: ⚠️ Potential issue | 🔴 Critical

Fix formatting issues to pass CI.

The pipeline is failing due to cargo fmt differences around lines 65 and 79. There appear to be trailing whitespace or blank line inconsistencies in this block.

🔧 Run cargo fmt to fix

cargo fmt --all

The formatter will normalize the blank lines and trailing whitespace around the status code, headers, and body extraction blocks.

As per coding guidelines: src/**/*.rs: Run cargo fmt and cargo check before merging changes to Rust code.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/openhuman/skills/qjs_skill_instance/event_loop/webhook_handler.rs` around
lines 63 - 83, There are formatting inconsistencies (extra trailing
whitespace/blank lines) around the response extraction block using response_val,
specifically near the status_code and resp_headers handling; run rustfmt (cargo
fmt --all) to normalize spacing and line breaks in the function containing
status_code, resp_headers and the subsequent body extraction so the block around
get("statusCode"), get("headers") and the parsing with serde_json::from_value is
formatted consistently and passes CI.

src/openhuman/skills/quickjs_libs/qjs_ops/ops_state.rs (1)

149-156: ⚠️ Potential issue | 🟡 Minor

Fix formatting to pass CI pipeline.

Multiple lines have formatting issues causing pipeline failures (lines 146, 159, 170, 178, 187). Run cargo fmt to resolve.

As per coding guidelines: src/**/*.rs: Run cargo fmt and cargo check before merging changes to Rust code.

Also applies to: 162-165, 173-173, 181-181, 184-185

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/openhuman/skills/quickjs_libs/qjs_ops/ops_state.rs` around lines 149 -
156, The Rust file has formatting issues around the memory insert validation
(uses input.title, input.content and js_err in ops_state.rs) that break CI; run
cargo fmt to fix formatting across src/**/*.rs and then cargo check to ensure no
diagnostics, making sure the validation block (the input.title.trim().is_empty()
/ input.content.trim().is_empty() checks) and the surrounding lines noted in the
comment (near the memory.insert logic) are properly formatted according to
rustfmt rules before committing.

src/openhuman/skills/qjs_skill_instance/event_loop/rpc_handlers.rs (1)

276-286: ⚠️ Potential issue | 🟡 Minor

Silent failure for OAuth credential write in managed mode differs from primary credential handling.

Line 285 uses let _ = std::fs::write(...) which silently ignores write failures, while lines 263-274 log errors on failure. This inconsistency means:

• If the primary auth_credential.json write fails → user sees error log
• If the back-compat oauth_credential.json write fails → silent failure

If the OAuth credential write fails silently, the skill will appear to complete setup successfully, but credentials won't be restored on restart. Consider logging warnings for consistency.

🔧 Suggested fix for consistency

     // Back-compatibility for managed mode
     if is_managed {
         let oauth_cred_json = serde_json::to_string(
             params
                 .get("credentials")
                 .unwrap_or(&serde_json::Value::Null),
         )
         .unwrap_or_else(|_| "null".to_string());
         let oauth_path = data_dir.join("oauth_credential.json");
-        let _ = std::fs::write(&oauth_path, &oauth_cred_json);
+        if let Err(e) = std::fs::write(&oauth_path, &oauth_cred_json) {
+            log::warn!(
+                "[skill:{}] Failed to persist managed OAuth credential (back-compat): {e}",
+                skill_id
+            );
+        }
     }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/openhuman/skills/qjs_skill_instance/event_loop/rpc_handlers.rs` around
lines 276 - 286, The oauth credential write inside the is_managed block
currently swallows errors via `let _ = std::fs::write(...)`; change it to handle
the Result and log any write failure consistently with the primary credential
handling (the same logger used for the primary `auth_credential.json` write).
Specifically, replace the ignored-write with proper error handling of
`std::fs::write(&oauth_path, &oauth_cred_json)` and call the same error logging
method used earlier (e.g., `process_logger.error` or the module's logger) to
include a clear message with the `oauth_path` and the underlying error so
failures are visible.

src/openhuman/skills/qjs_skill_instance/event_loop/mod.rs (2)

406-419: ⚠️ Potential issue | 🟠 Major

Reset or version the pending-tool JS state on timeout.

This branch drops pending_tool, but the timed-out Promise can still resolve later and flip globalThis.__pendingToolDone/__pendingToolResult. Because those globals are only cleared on the normal read_pending_tool_result() path, the next tool call can observe stale completion state and return the previous call’s result/error. Please clear the JS tracking globals here or gate them with a per-call token before sending the timeout back.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/openhuman/skills/qjs_skill_instance/event_loop/mod.rs` around lines 406 -
419, The timeout branch currently drops pending_tool but leaves the JS-side
tracking globals set, causing later calls to pick up stale state; update the
timeout handling in the pending_tool/ptc branch to either clear or version the
JS globals (globalThis.__pendingToolDone and globalThis.__pendingToolResult)
before sending the timeout reply, or implement a per-call token stored on ptc
and checked by read_pending_tool_result() so late JS resolution is ignored —
ensure the change references the same symbols (pending_tool / ptc,
read_pending_tool_result(), globalThis.__pendingToolDone,
globalThis.__pendingToolResult, tool_execution_timeout_secs(), skill_id) so the
JS state is reset or gated when you call ptc.reply.send(Err(...)).

---

66-73: ⚠️ Potential issue | 🟡 Minor

Remove the stray whitespace here to unblock cargo fmt.

CI is already failing on this block. As per coding guidelines src/**/*.rs: Run cargo fmt and cargo check before merging changes to Rust code.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/openhuman/skills/qjs_skill_instance/event_loop/mod.rs` around lines 66 -
73, There is a stray blank line after the log::debug! call in the
store_skill_sync/event_loop code that breaks cargo fmt; remove the extra blank
line so the comment "// Persist the full state blob to the skill's sync history"
immediately follows the log::debug! block, then run cargo fmt and cargo check to
ensure formatting and type checks pass (refer to log::debug! invocation and the
surrounding store_skill_sync/event_loop function to locate the change).

🟡 Minor comments (15)

src/openhuman/skills/quickjs_libs/mod.rs-3-6 (1)

3-6: ⚠️ Potential issue | 🟡 Minor

Minor typo: lowercase 'i' at start of sentence.

Line 4 has "it includes" where "It includes" would be grammatically correct.

📝 Suggested fix

-//! This module provides a QuickJS JavaScript runtime (via the `rquickjs` crate)
-//! for executing skill JavaScript code. it includes supporting shims and
-//! environment bindings to provide a browser-like or Node-like environment
-//! for skills.
+//! This module provides a QuickJS JavaScript runtime (via the `rquickjs` crate)
+//! for executing skill JavaScript code. It includes supporting shims and
+//! environment bindings to provide a browser-like or Node-like environment
+//! for skills.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/openhuman/skills/quickjs_libs/mod.rs` around lines 3 - 6, Update the
module doc comment in the quickjs_libs module to fix the typo: change "it
includes" to "It includes" so the sentence starts with a capital letter; locate
the top-level doc comment in src/openhuman/skills/quickjs_libs/mod.rs (module
quickjs_libs) and correct that capitalization in the comment block.

src/openhuman/skills/mod.rs-1-9 (1)

1-9: ⚠️ Potential issue | 🟡 Minor

Fix formatting to resolve pipeline failure.

The pipeline reports cargo fmt --all -- --check failed due to formatting differences in the module-level rustdoc comment. Run cargo fmt before merging.

The documentation content itself is good and accurately describes the skills subsystem components.

As per coding guidelines: "Run cargo fmt and cargo check before merging changes to Rust code".

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/openhuman/skills/mod.rs` around lines 1 - 9, The module-level rustdoc in
the skills module is misformatted causing `cargo fmt --all -- --check` to fail;
run `cargo fmt` (or rustfmt) to reformat `src/openhuman/skills/mod.rs` so the
module doc comments for the `skills` module and its list items are properly
wrapped/indented, then re-run `cargo fmt --all -- --check` (and `cargo check`)
before merging; reference the module-level doc for the `skills` module
(top-of-file rustdoc in mod.rs) when locating the text to be reformatted.

src/openhuman/skills/bridge/net.rs-3-6 (1)

3-6: ⚠️ Potential issue | 🟡 Minor

Documentation incorrectly references V8; this codebase uses QuickJS.

The doc comment mentions "V8 isolates" but the skills runtime uses QuickJS (rquickjs), not V8/deno_core. This could mislead future maintainers.

📝 Suggested fix

-//! Provides a synchronous HTTP client bridge designed for environments
-//! where the standard asynchronous `fetch` API may not be suitable,
-//! such as within certain V8 isolates or when strict synchronous
-//! execution is required.
+//! Provides a synchronous HTTP client bridge designed for environments
+//! where the standard asynchronous `fetch` API may not be suitable,
+//! such as within the QuickJS runtime or when strict synchronous
+//! execution is required.

As per coding guidelines: "Skills runtime uses QuickJS (rquickjs) in src/openhuman/skills/, not V8/deno_core".

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/openhuman/skills/bridge/net.rs` around lines 3 - 6, Update the
module-level documentation in net.rs to remove the incorrect reference to "V8
isolates" and instead mention that the skills runtime uses QuickJS (rquickjs);
specifically edit the top doc comment that describes the synchronous HTTP client
bridge (module-level docs in net.rs) to say QuickJS/rquickjs rather than
V8/deno_core so future maintainers are not misled.

src/openhuman/skills/bridge/net.rs-8-9 (1)

8-9: ⚠️ Potential issue | 🟡 Minor

Fix formatting to resolve pipeline failure.

The pipeline reports cargo fmt --all -- --check failed due to formatting differences in import ordering around line 5. Run cargo fmt before merging.

As per coding guidelines: "Run cargo fmt and cargo check before merging changes to Rust code".

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/openhuman/skills/bridge/net.rs` around lines 8 - 9, The imports in
src/openhuman/skills/bridge/net.rs are not formatted/ordered per rustfmt which
caused the cargo fmt check to fail; run cargo fmt (or rustfmt) to reformat the
file so the use statements (e.g., use std::collections::HashMap; and use
serde::Deserialize;) are ordered and formatted correctly, then run cargo check
to ensure no other issues remain before merging.

src/openhuman/memory/store/memory_trait.rs-131-132 (1)

131-132: ⚠️ Potential issue | 🟡 Minor

Fix formatting to pass CI pipeline.

The pipeline is failing due to formatting differences at lines 128, 144, and 156. The blank lines don't conform to cargo fmt standards.

🔧 Run cargo fmt to fix the whitespace issues

Remove the trailing whitespace/blank lines at lines 131, 147, and 159 that are causing cargo fmt to fail.

As per coding guidelines: src/**/*.rs: Run cargo fmt and cargo check before merging changes to Rust code.

Also applies to: 147-147, 159-160

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/openhuman/memory/store/memory_trait.rs` around lines 131 - 132, The CI
fails due to formatting whitespace/blank-line issues around the comment "Simple
keyword-based filtering for episodic matches." in memory_trait.rs; remove the
extra blank lines/trailing whitespace near that comment (the blank lines around
the surrounding blocks at the same area referenced in the review), then run
cargo fmt and cargo check to ensure formatting conforms to rustfmt; confirm
changes touch the comment region in memory_trait.rs (the function/impl block
containing the episodic matching logic) and re-run CI.

src/openhuman/skills/utils.rs-2-6 (1)

2-6: ⚠️ Potential issue | 🟡 Minor

Fix typo in documentation.

Line 6 has a typo: "bridgeing" should be "bridging".

📝 Suggested fix

-//! prevent common asynchronous pitfalls in Rust, such as attempting to start
-//! multiple Tokio runtimes on the same thread or causing deadlocks when
-//! bridgeing between synchronous and asynchronous code.
+//! prevent common asynchronous pitfalls in Rust, such as attempting to start
+//! multiple Tokio runtimes on the same thread or causing deadlocks when
+//! bridging between synchronous and asynchronous code.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/openhuman/skills/utils.rs` around lines 2 - 6, Update the module-level
documentation at the top of utils.rs to correct the typo "bridgeing" to
"bridging" in the doc comment describing safe execution wrappers; locate the
module doc comment (the leading //! block in utils.rs) and replace the
misspelled word so the sentence reads "...bridging between synchronous and
asynchronous code."

src/openhuman/skills/registry_ops.rs-40-43 (1)

40-43: ⚠️ Potential issue | 🟡 Minor

Fix formatting to pass CI pipeline.

The pipeline is failing due to formatting differences around line 37-43. The blank lines around tag_categories(&mut registry) don't conform to cargo fmt standards.

🔧 Run cargo fmt to fix

-        
-        // Ensure category flags are set correctly based on the registry structure
+        // Ensure category flags are set correctly based on the registry structure
         tag_categories(&mut registry);
-        

As per coding guidelines: src/**/*.rs: Run cargo fmt and cargo check before merging changes to Rust code.

🤖 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 40 - 43, Remove the extra
blank lines around the call to tag_categories(&mut registry) in registry_ops.rs
so it matches rustfmt formatting rules; ensure the call sits correctly within
its surrounding block (no stray blank lines before/after) and then run cargo fmt
and cargo check to validate formatting and compilation; reference the
tag_categories function call in your edit to locate and adjust the spacing.

src/openhuman/skills/utils.rs-48-56 (1)

48-56: ⚠️ Potential issue | 🟡 Minor

Documentation references V8 but skills use QuickJS.

The doc comment mentions "V8 isolate operations" but per the coding guidelines, the skills runtime uses QuickJS (rquickjs), not V8/deno_core. Update the documentation to reflect the actual runtime.

📝 Suggested fix

 /// This is particularly useful for avoiding deadlocks when executing async logic
-/// from within contexts that block the current thread, such as certain V8 isolate
-/// operations or synchronous bridge calls.
+/// from within contexts that block the current thread, such as certain QuickJS
+/// runtime operations or synchronous bridge calls.

As per coding guidelines: src/openhuman/skills/**/*.rs: Skills runtime uses QuickJS (rquickjs) in src/openhuman/skills/, not V8/deno_core.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/openhuman/skills/utils.rs` around lines 48 - 56, Update the doc comment
in src/openhuman/skills/utils.rs to replace the incorrect "V8 isolate
operations" / deno_core references with QuickJS (rquickjs): change wording
describing contexts that block the current thread to mention QuickJS/rquickjs
and synchronous bridge calls used by the skills runtime, and ensure the summary
for the function that "safely executes an asynchronous future on a dedicated
background thread" accurately states it avoids deadlocks with rquickjs-based
operations rather than V8.

src/openhuman/skills/qjs_engine.rs-45-47 (1)

45-47: ⚠️ Potential issue | 🟡 Minor

Fix rustdoc formatting to pass CI pipeline.

The pipeline is failing due to rustdoc formatting differences at multiple locations. The multi-line doc comments with /// followed by blank comment lines don't conform to cargo fmt standards.

Run cargo fmt to automatically fix these issues. Typically, the fix involves removing blank lines within doc comment blocks or ensuring consistent spacing.

As per coding guidelines: src/**/*.rs: Run cargo fmt and cargo check before merging changes to Rust code.

Also applies to: 75-77, 177-184, 359-362, 521-523, 578-580, 621-623, 684-685, 691-692, 702-705, 955-957

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/openhuman/skills/qjs_engine.rs` around lines 45 - 47, Remove the stray
blank lines inside the multi-line rustdoc comments (the lines that are just "///
" without content) so the doc blocks are contiguous (e.g., the block beginning
"This struct orchestrates the lifecycle of all JavaScript-based skills" and the
other doc blocks noted in the review), then run cargo fmt to normalize
formatting and cargo check to ensure no warnings/errors; ensure all rustdoc
blocks have consistent "///" lines without empty comment-only lines.

src/openhuman/memory/chunker.rs-42-51 (1)

42-51: ⚠️ Potential issue | 🟡 Minor

Fix formatting to pass CI pipeline.

The pipeline is failing due to formatting differences around lines 41 and 48. The blank lines after comments don't conform to cargo fmt standards.

🔧 Run cargo fmt to fix

     // Rough estimation: 4 characters per token for English text.
     let max_chars = max_tokens * 4;
-    
-    // Step 1: Divide the document into top-level sections based on headings.
+    // Step 1: Divide the document into top-level sections based on headings.
     let sections = split_on_headings(text);
     let mut chunks = Vec::with_capacity(sections.len());

     for (heading, body) in sections {
         let heading: Option<Rc<str>> = heading.map(Rc::from);
-        
-        // Combine heading and body to check initial size.
+        // Combine heading and body to check initial size.
         let full = if let Some(ref h) = heading {

As per coding guidelines: src/**/*.rs: Run cargo fmt and cargo check before merging changes to Rust code.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/openhuman/memory/chunker.rs` around lines 42 - 51, There are extra blank
lines after comments around the chunking logic (near max_chars,
split_on_headings, and the for loop) that fail cargo fmt; remove the stray blank
lines and reformat the block that defines max_chars, calls split_on_headings,
creates chunks, and maps heading to Rc<str> (symbols: max_chars,
split_on_headings, chunks, heading) and then run cargo fmt (and cargo check) to
ensure the file compiles and meets formatting standards.

src/openhuman/skills/ping_scheduler.rs-3-7 (1)

3-7: ⚠️ Potential issue | 🟡 Minor

Top-level ping eligibility docs conflict with actual filter.

The module docs state “all Running skills,” but tick() only includes skills that are both Running and explicitly "connected". This can mislead on-call/debugging behavior.

Proposed wording fix

-//! Every minute, the scheduler pings all skills with a status of `Running` by
+//! Every minute, the scheduler pings skills with status `Running` whose
+//! published `connection_status` is `"connected"` by
 //! sending an RPC `skill/ping` message.

Also applies to: 100-104, 114-120

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/openhuman/skills/ping_scheduler.rs` around lines 3 - 7, The module docs
incorrectly claim we ping "all `Running` skills" but the tick() implementation
filters to Running skills whose published state shows connection_status ==
"connected"; update the documentation (top-level comment and the similar doc
blocks around the other occurrences) to state that pinging is applied only to
skills with status Running and connection_status "connected" (or, alternatively,
remove that additional filter from tick() if you prefer to truly ping all
Running skills) and ensure references to tick(), the connection_status check,
and the published state semantics are consistent; adjust any related tests or
comments to match the chosen behavior.

src/openhuman/memory/rpc_models.rs-248-253 (1)

248-253: ⚠️ Potential issue | 🟡 Minor

Document min_retention and as_of as compatibility no-ops until they are implemented.

The new field docs read like active filters, but the tests in this file already codify them as accepted-and-ignored compatibility params. Please align the rustdoc with the current behavior so clients do not assume these fields affect recall results yet.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/openhuman/memory/rpc_models.rs` around lines 248 - 253, Update the
rustdoc for the fields min_retention and as_of in the struct so they state these
are currently accepted-for-compatibility no-ops (ignored by recall) until
implemented, rather than active filters; mention they are preserved for forward
compatibility and that serde(default) keeps them optional, and reference the
fields by name (min_retention, as_of) so readers know the current behavior.

src/openhuman/memory/ingestion.rs-70-72 (1)

70-72: ⚠️ Potential issue | 🟡 Minor

batch_size is documented as active configuration, but this module never reads it.

That makes the new rustdoc misleading: callers can set config.batchSize and get no behavior change. Either wire it into the extraction loop or document it as reserved/unused for now.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/openhuman/memory/ingestion.rs` around lines 70 - 72, The struct field
batch_size (and its default_default_batch_size) is never used so the new rustdoc
is misleading; update the ingestion logic in src/openhuman/memory/ingestion.rs
to actually use batch_size by replacing any hard-coded chunk/window size in the
extraction/ingestion loop (e.g., where documents are split into batches in the
function that performs embedding extraction or batch processing) with
self.batch_size (or config.batch_size), or alternatively change the field to a
clearly reserved/unused API (add a doc comment like "reserved" and/or
#[serde(skip)] and remove it from public docs); reference the batch_size field
and the default_batch_size helper when wiring it into the batch/chunk handling
logic.

src/openhuman/memory/ingestion.rs-220-223 (1)

220-223: ⚠️ Potential issue | 🟡 Minor

Trim the rustdoc whitespace here; cargo fmt --check is failing on this block.

As per coding guidelines src/**/*.rs: Run cargo fmt and cargo check before merging changes to Rust code.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/openhuman/memory/ingestion.rs` around lines 220 - 223, The rustdoc block
that starts with "Accumulates extraction results across multiple chunks or
units." has extra trailing whitespace/blank lines causing cargo fmt --check to
fail; remove the extra blank line(s) and trailing spaces within that comment so
it becomes a single properly formatted doc comment paragraph, then run cargo fmt
and cargo check to verify; locate the comment above the ingestion logic in
src/openhuman/memory/ingestion.rs (the rustdoc for the accumulator) and trim the
whitespace accordingly.

src/openhuman/memory/rpc_models.rs-7-8 (1)

7-8: ⚠️ Potential issue | 🟡 Minor

Run cargo fmt on this file before merge.

The import ordering/grouping here is still failing --check. As per coding guidelines src/**/*.rs: Run cargo fmt and cargo check before merging changes to Rust code.

Also applies to: 442-446

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/openhuman/memory/rpc_models.rs` around lines 7 - 8, Imports in
src/openhuman/memory/rpc_models.rs are not formatted/ordered per project style
and fail cargo fmt --check; run `cargo fmt` (and then `cargo check`) and ensure
the use statements (e.g., the `use std::collections::BTreeMap;` and `use
serde::{Deserialize, Serialize};`) are ordered/grouped according to rustfmt
defaults (standard library imports first, then external crates) and that the
file is formatted; re-run checks to confirm the lint passes.

🧹 Nitpick comments (3)

src/openhuman/skills/quickjs_libs/qjs_ops/ops_state.rs (1)

25-30: created_at and updated_at fields are parsed but never used.

These fields are defined in JsMemoryInsertInput and deserialized from JavaScript, but they're never propagated to NamespaceDocumentInput (which doesn't have these fields - timestamps are generated server-side in upsert_document). This matches the pattern in src/openhuman/memory/store/client.rs:143-144 where these fields are explicitly ignored with underscore prefixes.

Consider either:

1. Removing these fields from JsMemoryInsertInput if they're not needed
2. Documenting that they're intentionally ignored for API compatibility

♻️ Option 1: Remove unused fields

 struct JsMemoryInsertInput {
     /// Title of the document to be stored in memory.
     title: String,
     /// Content of the document.
     content: String,
     /// Type of the source (e.g., "doc", "chat", "email").
     source_type: Option<String>,
     /// Optional metadata associated with the document.
     metadata: Option<serde_json::Value>,
     /// Priority of the document ("high", "medium", "low").
     priority: Option<String>,
-    /// Unix timestamp when the document was created.
-    created_at: Option<f64>,
-    /// Unix timestamp when the document was last updated.
-    updated_at: Option<f64>,
     /// Optional unique identifier for the document.
     document_id: Option<String>,
 }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/openhuman/skills/quickjs_libs/qjs_ops/ops_state.rs` around lines 25 - 30,
JsMemoryInsertInput currently defines created_at and updated_at but they are
never used or propagated to NamespaceDocumentInput (timestamps are
server-generated in upsert_document); either remove these fields from
JsMemoryInsertInput to avoid dead fields or mark them as intentionally ignored
for API compatibility by renaming to _created_at and _updated_at (or adding a
clear comment) to mirror the pattern used in the memory store client (see the
explicit ignored timestamp fields in client.rs around upsert handling); update
serde/deserialization accordingly and add a brief comment that timestamps are
server-generated so reviewers know this omission is intentional.

src/openhuman/skills/qjs_engine.rs (1)

978-979: Documentation for internal function inside outer function.

The inner fn walk() at line 980 has a doc comment (///), but doc comments on inner functions are not rendered in rustdoc. Consider using a regular comment (//) instead.

📝 Suggested fix

-    /// Internal recursive function to walk the directory tree.
+    // Internal recursive function to walk the directory tree.
     fn walk(

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/openhuman/skills/qjs_engine.rs` around lines 978 - 979, The inner
function walk() is using a doc comment (///) which won't appear in rustdoc
because it's nested; replace the leading triple-slash doc comment with a normal
comment line(s) (//) above the inner fn walk() declaration and adjust
wording/formatting as needed so the intent remains clear without trying to be a
crate-level doc; keep the function name walk and its signature unchanged.

src/openhuman/skills/skill_registry.rs (1)

145-166: Self-origin tool-call API now encodes an invalid state.

With the new SkillSelf isolation rule, a separate skill_id target parameter allows combinations that are guaranteed to fail at runtime. Consider tightening this API so self-origin calls derive target from origin (or split self/external entrypoints) to make invalid combinations unrepresentable.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/openhuman/skills/skill_registry.rs` around lines 145 - 166, The
call_tool_scoped API allows an invalid combination where
ToolCallOrigin::SkillSelf includes a caller_skill_id but the separate skill_id
parameter can point to a different skill, which will always be rejected; to fix
this, change call_tool_scoped so that when origin matches
ToolCallOrigin::SkillSelf you derive the target skill id from the origin (use
caller_skill_id) instead of accepting the external skill_id parameter (or split
into two entrypoints: one for self-calls that takes only origin and one for
external calls that requires an explicit target), and update callers to stop
passing a separate skill_id for self-origin calls; refer to call_tool_scoped and
ToolCallOrigin::SkillSelf to locate the logic to adjust.

🤖 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/memory/embeddings.rs`:
- Around line 117-126: Formatting issues in src/openhuman/memory/embeddings.rs
are failing cargo fmt --check; run cargo fmt to fix whitespace and rustdoc
formatting across the file (including around the function
ensure_fastembed_ort_dylib_path and the other commented regions) and then run
cargo check to ensure no formatting-related warnings remain; commit the
reformatted file so CI passes.

In `@src/openhuman/memory/ops.rs`:
- Around line 354-355: The helper functions resolve_existing_memory_path and
resolve_writable_memory_path claim to confine paths to the memory directory but
currently join against workspace_dir, allowing sibling paths; update these
functions to use the actual memory root resolver (e.g., call
resolve_memory_root()) when joining and validating paths (and ensure
validate_memory_relative_path still checks against that memory root), or
alternatively change the docstrings/messages to say “workspace” instead of
“memory directory” so behavior and docs match—adjust any path joins and
validation logic in resolve_existing_memory_path, resolve_writable_memory_path
(and nearby code in the 375-405 range) to reference resolve_memory_root() and
enforce non-escaping of the memory directory.

In `@src/openhuman/memory/response_cache.rs`:
- Around line 234-236: The tests module "mod tests" is left unclosed causing a
compile error; close the module and replace the placeholder with concrete unit
tests that exercise the ResponseCache API (e.g., create a ResponseCache, call
insert, get/lookup to verify retrieval, trigger eviction behavior and assert
capacity eviction, verify stats counters via stats() and test clear() to ensure
emptiness), or alternatively move those test cases into an integration test
file; ensure the "mod tests" block is properly terminated with a closing brace
and include tests named to reflect operations
(insert/retrieve/evict/stats/clear) referencing the ResponseCache type and its
methods.

In `@src/openhuman/skills/ops.rs`:
- Around line 1-5: Rustfmt failure: run cargo fmt --all to reformat
src/openhuman/skills/ops.rs (fixing rustdoc/comment spacing changes around the
module-level comments and the blocks mentioned) and then run cargo check to
ensure no other issues; update the commit and push. Specifically, reformat the
module docs in ops.rs and any nearby comment blocks referenced (lines ~10-13,
53-56, 75-79) so they match rustfmt style, then re-run cargo check before
pushing the changes.

In `@src/openhuman/skills/qjs_skill_instance/js_handlers.rs`:
- Around line 22-26: There is a formatting/whitespace issue in js_handlers.rs
around the function taking name: &str -> Result<(), String> (and the surrounding
imports, notably the Duration import) causing cargo fmt failures; run cargo fmt
--all and remove the trailing whitespace or extra blank line after the Duration
import and ensure the function signature/assignment (the name.to_string() line)
conforms to rustfmt output so the file passes cargo fmt and cargo check.

In `@src/openhuman/skills/skill_registry.rs`:
- Around line 1-5: The Rust file src/openhuman/skills/skill_registry.rs has
rustfmt/formatting mismatches in several rustdoc/comment blocks (notably around
the top module doc and the regions flagged at lines 33-36, 49-52, 72-75,
285-311, 408-414); run cargo fmt --all (or rustfmt on that file) to normalize
the doc comments and reformat the file, then run cargo check to ensure no
formatting/compile issues remain; specifically reformat the module-level doc and
any multiline comment blocks near the SkillRegistry implementation and its
associated functions/structs so they conform to rustfmt rules before committing.

In `@src/openhuman/skills/socket_manager.rs`:
- Around line 84-89: Run rustfmt on the repository and fix the
rustdoc/whitespace formatting for the offending Rust file: run `cargo fmt --all`
(or `rustfmt`) and then re-check with `cargo fmt -- --check`/`cargo check`;
specifically tidy up the doc comments and spacing around the SocketManager
struct declaration and the other listed regions (near the struct SocketManager
and the ranges referenced) so they conform to rustfmt output, then commit the
formatted changes. Ensure no code logic is altered—only formatting/rustdoc
whitespace adjustments.

---

Outside diff comments:
In `@src/openhuman/memory/relex.rs`:
- Around line 1-993: The PR fails rustfmt checks (formatting diffs at the top of
the file and around functions like split_whitespace_tokens, encode_prompt, and
make_spans_tensors); fix by running rustfmt/cargo fmt to reformat the file(s)
and commit the resulting changes (run `cargo fmt --all` locally or in CI), then
re-run the formatter check to ensure functions such as split_whitespace_tokens,
encode_prompt, make_spans_tensors and surrounding code are properly formatted
per project style.

In `@src/openhuman/skills/manifest.rs`:
- Around line 1-101: Run rustfmt (cargo fmt --all) and fix the
formatting/blank-line issues in the manifest module so CI passes; specifically
normalize rustdoc and blank lines around the structs and functions: SkillSetup,
SkillAuthType, SkillAuthConfig, deserialize_non_empty_modes, and SkillAuthMode
(ensure doc comments wrap and spacing matches rustfmt output and remove/add
blank lines around fields as needed), then run cargo check to verify.
- Around line 163-211: The doc-comments for SkillManifest methods (from_json,
from_path, is_javascript, supports_current_platform, to_config) contain
formatting inconsistencies; run rustfmt (cargo fmt --all) and reflow/align the
/// comment blocks for those methods to match project style so the triple-slash
comments and blank lines are consistent (especially around is_javascript,
supports_current_platform, and to_config) and then commit the formatted changes.

In `@src/openhuman/skills/qjs_skill_instance/event_loop/mod.rs`:
- Around line 406-419: The timeout branch currently drops pending_tool but
leaves the JS-side tracking globals set, causing later calls to pick up stale
state; update the timeout handling in the pending_tool/ptc branch to either
clear or version the JS globals (globalThis.__pendingToolDone and
globalThis.__pendingToolResult) before sending the timeout reply, or implement a
per-call token stored on ptc and checked by read_pending_tool_result() so late
JS resolution is ignored — ensure the change references the same symbols
(pending_tool / ptc, read_pending_tool_result(), globalThis.__pendingToolDone,
globalThis.__pendingToolResult, tool_execution_timeout_secs(), skill_id) so the
JS state is reset or gated when you call ptc.reply.send(Err(...)).
- Around line 66-73: There is a stray blank line after the log::debug! call in
the store_skill_sync/event_loop code that breaks cargo fmt; remove the extra
blank line so the comment "// Persist the full state blob to the skill's sync
history" immediately follows the log::debug! block, then run cargo fmt and cargo
check to ensure formatting and type checks pass (refer to log::debug! invocation
and the surrounding store_skill_sync/event_loop function to locate the change).

In `@src/openhuman/skills/qjs_skill_instance/event_loop/rpc_handlers.rs`:
- Around line 276-286: The oauth credential write inside the is_managed block
currently swallows errors via `let _ = std::fs::write(...)`; change it to handle
the Result and log any write failure consistently with the primary credential
handling (the same logger used for the primary `auth_credential.json` write).
Specifically, replace the ignored-write with proper error handling of
`std::fs::write(&oauth_path, &oauth_cred_json)` and call the same error logging
method used earlier (e.g., `process_logger.error` or the module's logger) to
include a clear message with the `oauth_path` and the underlying error so
failures are visible.

In `@src/openhuman/skills/qjs_skill_instance/event_loop/webhook_handler.rs`:
- Around line 63-83: There are formatting inconsistencies (extra trailing
whitespace/blank lines) around the response extraction block using response_val,
specifically near the status_code and resp_headers handling; run rustfmt (cargo
fmt --all) to normalize spacing and line breaks in the function containing
status_code, resp_headers and the subsequent body extraction so the block around
get("statusCode"), get("headers") and the parsing with serde_json::from_value is
formatted consistently and passes CI.

In `@src/openhuman/skills/quickjs_libs/qjs_ops/ops_state.rs`:
- Around line 149-156: The Rust file has formatting issues around the memory
insert validation (uses input.title, input.content and js_err in ops_state.rs)
that break CI; run cargo fmt to fix formatting across src/**/*.rs and then cargo
check to ensure no diagnostics, making sure the validation block (the
input.title.trim().is_empty() / input.content.trim().is_empty() checks) and the
surrounding lines noted in the comment (near the memory.insert logic) are
properly formatted according to rustfmt rules before committing.

In `@src/openhuman/skills/types.rs`:
- Around line 1-493: The PR failed CI because this Rust file is not formatted
with rustfmt; run "cargo fmt --all" and commit the formatting changes so the
file (including definitions like SkillStatus, SkillMessage, ToolResult,
derive_connection_status and the tests) matches the project's formatting rules;
ensure you re-run "cargo fmt --all" locally (or via CI) to fix the spacing/line
breaks reported and then add/commit the updated file before pushing.

---

Minor comments:
In `@src/openhuman/memory/chunker.rs`:
- Around line 42-51: There are extra blank lines after comments around the
chunking logic (near max_chars, split_on_headings, and the for loop) that fail
cargo fmt; remove the stray blank lines and reformat the block that defines
max_chars, calls split_on_headings, creates chunks, and maps heading to Rc<str>
(symbols: max_chars, split_on_headings, chunks, heading) and then run cargo fmt
(and cargo check) to ensure the file compiles and meets formatting standards.

In `@src/openhuman/memory/ingestion.rs`:
- Around line 70-72: The struct field batch_size (and its
default_default_batch_size) is never used so the new rustdoc is misleading;
update the ingestion logic in src/openhuman/memory/ingestion.rs to actually use
batch_size by replacing any hard-coded chunk/window size in the
extraction/ingestion loop (e.g., where documents are split into batches in the
function that performs embedding extraction or batch processing) with
self.batch_size (or config.batch_size), or alternatively change the field to a
clearly reserved/unused API (add a doc comment like "reserved" and/or
#[serde(skip)] and remove it from public docs); reference the batch_size field
and the default_batch_size helper when wiring it into the batch/chunk handling
logic.
- Around line 220-223: The rustdoc block that starts with "Accumulates
extraction results across multiple chunks or units." has extra trailing
whitespace/blank lines causing cargo fmt --check to fail; remove the extra blank
line(s) and trailing spaces within that comment so it becomes a single properly
formatted doc comment paragraph, then run cargo fmt and cargo check to verify;
locate the comment above the ingestion logic in
src/openhuman/memory/ingestion.rs (the rustdoc for the accumulator) and trim the
whitespace accordingly.

In `@src/openhuman/memory/rpc_models.rs`:
- Around line 248-253: Update the rustdoc for the fields min_retention and as_of
in the struct so they state these are currently accepted-for-compatibility
no-ops (ignored by recall) until implemented, rather than active filters;
mention they are preserved for forward compatibility and that serde(default)
keeps them optional, and reference the fields by name (min_retention, as_of) so
readers know the current behavior.
- Around line 7-8: Imports in src/openhuman/memory/rpc_models.rs are not
formatted/ordered per project style and fail cargo fmt --check; run `cargo fmt`
(and then `cargo check`) and ensure the use statements (e.g., the `use
std::collections::BTreeMap;` and `use serde::{Deserialize, Serialize};`) are
ordered/grouped according to rustfmt defaults (standard library imports first,
then external crates) and that the file is formatted; re-run checks to confirm
the lint passes.

In `@src/openhuman/memory/store/memory_trait.rs`:
- Around line 131-132: The CI fails due to formatting whitespace/blank-line
issues around the comment "Simple keyword-based filtering for episodic matches."
in memory_trait.rs; remove the extra blank lines/trailing whitespace near that
comment (the blank lines around the surrounding blocks at the same area
referenced in the review), then run cargo fmt and cargo check to ensure
formatting conforms to rustfmt; confirm changes touch the comment region in
memory_trait.rs (the function/impl block containing the episodic matching logic)
and re-run CI.

In `@src/openhuman/skills/bridge/net.rs`:
- Around line 3-6: Update the module-level documentation in net.rs to remove the
incorrect reference to "V8 isolates" and instead mention that the skills runtime
uses QuickJS (rquickjs); specifically edit the top doc comment that describes
the synchronous HTTP client bridge (module-level docs in net.rs) to say
QuickJS/rquickjs rather than V8/deno_core so future maintainers are not misled.
- Around line 8-9: The imports in src/openhuman/skills/bridge/net.rs are not
formatted/ordered per rustfmt which caused the cargo fmt check to fail; run
cargo fmt (or rustfmt) to reformat the file so the use statements (e.g., use
std::collections::HashMap; and use serde::Deserialize;) are ordered and
formatted correctly, then run cargo check to ensure no other issues remain
before merging.

In `@src/openhuman/skills/mod.rs`:
- Around line 1-9: The module-level rustdoc in the skills module is misformatted
causing `cargo fmt --all -- --check` to fail; run `cargo fmt` (or rustfmt) to
reformat `src/openhuman/skills/mod.rs` so the module doc comments for the
`skills` module and its list items are properly wrapped/indented, then re-run
`cargo fmt --all -- --check` (and `cargo check`) before merging; reference the
module-level doc for the `skills` module (top-of-file rustdoc in mod.rs) when
locating the text to be reformatted.

In `@src/openhuman/skills/ping_scheduler.rs`:
- Around line 3-7: The module docs incorrectly claim we ping "all `Running`
skills" but the tick() implementation filters to Running skills whose published
state shows connection_status == "connected"; update the documentation
(top-level comment and the similar doc blocks around the other occurrences) to
state that pinging is applied only to skills with status Running and
connection_status "connected" (or, alternatively, remove that additional filter
from tick() if you prefer to truly ping all Running skills) and ensure
references to tick(), the connection_status check, and the published state
semantics are consistent; adjust any related tests or comments to match the
chosen behavior.

In `@src/openhuman/skills/qjs_engine.rs`:
- Around line 45-47: Remove the stray blank lines inside the multi-line rustdoc
comments (the lines that are just "/// " without content) so the doc blocks are
contiguous (e.g., the block beginning "This struct orchestrates the lifecycle of
all JavaScript-based skills" and the other doc blocks noted in the review), then
run cargo fmt to normalize formatting and cargo check to ensure no
warnings/errors; ensure all rustdoc blocks have consistent "///" lines without
empty comment-only lines.

In `@src/openhuman/skills/quickjs_libs/mod.rs`:
- Around line 3-6: Update the module doc comment in the quickjs_libs module to
fix the typo: change "it includes" to "It includes" so the sentence starts with
a capital letter; locate the top-level doc comment in
src/openhuman/skills/quickjs_libs/mod.rs (module quickjs_libs) and correct that
capitalization in the comment block.

In `@src/openhuman/skills/registry_ops.rs`:
- Around line 40-43: Remove the extra blank lines around the call to
tag_categories(&mut registry) in registry_ops.rs so it matches rustfmt
formatting rules; ensure the call sits correctly within its surrounding block
(no stray blank lines before/after) and then run cargo fmt and cargo check to
validate formatting and compilation; reference the tag_categories function call
in your edit to locate and adjust the spacing.

In `@src/openhuman/skills/utils.rs`:
- Around line 2-6: Update the module-level documentation at the top of utils.rs
to correct the typo "bridgeing" to "bridging" in the doc comment describing safe
execution wrappers; locate the module doc comment (the leading //! block in
utils.rs) and replace the misspelled word so the sentence reads "...bridging
between synchronous and asynchronous code."
- Around line 48-56: Update the doc comment in src/openhuman/skills/utils.rs to
replace the incorrect "V8 isolate operations" / deno_core references with
QuickJS (rquickjs): change wording describing contexts that block the current
thread to mention QuickJS/rquickjs and synchronous bridge calls used by the
skills runtime, and ensure the summary for the function that "safely executes an
asynchronous future on a dedicated background thread" accurately states it
avoids deadlocks with rquickjs-based operations rather than V8.

---

Nitpick comments:
In `@src/openhuman/skills/qjs_engine.rs`:
- Around line 978-979: The inner function walk() is using a doc comment (///)
which won't appear in rustdoc because it's nested; replace the leading
triple-slash doc comment with a normal comment line(s) (//) above the inner fn
walk() declaration and adjust wording/formatting as needed so the intent remains
clear without trying to be a crate-level doc; keep the function name walk and
its signature unchanged.

In `@src/openhuman/skills/quickjs_libs/qjs_ops/ops_state.rs`:
- Around line 25-30: JsMemoryInsertInput currently defines created_at and
updated_at but they are never used or propagated to NamespaceDocumentInput
(timestamps are server-generated in upsert_document); either remove these fields
from JsMemoryInsertInput to avoid dead fields or mark them as intentionally
ignored for API compatibility by renaming to _created_at and _updated_at (or
adding a clear comment) to mirror the pattern used in the memory store client
(see the explicit ignored timestamp fields in client.rs around upsert handling);
update serde/deserialization accordingly and add a brief comment that timestamps
are server-generated so reviewers know this omission is intentional.

In `@src/openhuman/skills/skill_registry.rs`:
- Around line 145-166: The call_tool_scoped API allows an invalid combination
where ToolCallOrigin::SkillSelf includes a caller_skill_id but the separate
skill_id parameter can point to a different skill, which will always be
rejected; to fix this, change call_tool_scoped so that when origin matches
ToolCallOrigin::SkillSelf you derive the target skill id from the origin (use
caller_skill_id) instead of accepting the external skill_id parameter (or split
into two entrypoints: one for self-calls that takes only origin and one for
external calls that requires an explicit target), and update callers to stop
passing a separate skill_id for self-origin calls; refer to call_tool_scoped and
ToolCallOrigin::SkillSelf to locate the logic to adjust.

🪄 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: 6748b264-9032-4a23-82ef-21efb958560c

📥 Commits

Reviewing files that changed from the base of the PR and between 7bce9d2 and 0d198e4.

📒 Files selected for processing (56)

• src/core/cli.rs
• src/core/jsonrpc.rs
• src/lib.rs
• src/main.rs
• src/openhuman/config/mod.rs
• src/openhuman/memory/chunker.rs
• src/openhuman/memory/embeddings.rs
• src/openhuman/memory/ingestion.rs
• src/openhuman/memory/ingestion_queue.rs
• src/openhuman/memory/mod.rs
• src/openhuman/memory/ops.rs
• src/openhuman/memory/relex.rs
• src/openhuman/memory/response_cache.rs
• src/openhuman/memory/rpc_models.rs
• src/openhuman/memory/schemas.rs
• src/openhuman/memory/store/client.rs
• src/openhuman/memory/store/factories.rs
• src/openhuman/memory/store/memory_trait.rs
• src/openhuman/memory/store/mod.rs
• src/openhuman/memory/traits.rs
• src/openhuman/mod.rs
• src/openhuman/skills/bridge/mod.rs
• src/openhuman/skills/bridge/net.rs
• src/openhuman/skills/cron_scheduler.rs
• src/openhuman/skills/manifest.rs
• src/openhuman/skills/mod.rs
• src/openhuman/skills/ops.rs
• src/openhuman/skills/ping_scheduler.rs
• src/openhuman/skills/preferences.rs
• src/openhuman/skills/qjs_engine.rs
• src/openhuman/skills/qjs_skill_instance/event_loop/mod.rs
• src/openhuman/skills/qjs_skill_instance/event_loop/rpc_handlers.rs
• src/openhuman/skills/qjs_skill_instance/event_loop/webhook_handler.rs
• src/openhuman/skills/qjs_skill_instance/instance.rs
• src/openhuman/skills/qjs_skill_instance/js_handlers.rs
• src/openhuman/skills/qjs_skill_instance/js_helpers.rs
• src/openhuman/skills/qjs_skill_instance/mod.rs
• src/openhuman/skills/qjs_skill_instance/types.rs
• src/openhuman/skills/quickjs_libs/mod.rs
• src/openhuman/skills/quickjs_libs/qjs_ops/mod.rs
• src/openhuman/skills/quickjs_libs/qjs_ops/ops.rs
• src/openhuman/skills/quickjs_libs/qjs_ops/ops_core.rs
• src/openhuman/skills/quickjs_libs/qjs_ops/ops_net.rs
• src/openhuman/skills/quickjs_libs/qjs_ops/ops_state.rs
• src/openhuman/skills/quickjs_libs/qjs_ops/ops_storage.rs
• src/openhuman/skills/quickjs_libs/qjs_ops/ops_webhook.rs
• src/openhuman/skills/quickjs_libs/qjs_ops/types.rs
• src/openhuman/skills/quickjs_libs/storage.rs
• src/openhuman/skills/registry_cache.rs
• src/openhuman/skills/registry_ops.rs
• src/openhuman/skills/registry_types.rs
• src/openhuman/skills/schemas.rs
• src/openhuman/skills/skill_registry.rs
• src/openhuman/skills/socket_manager.rs
• src/openhuman/skills/types.rs
• src/openhuman/skills/utils.rs

[coderabbitai]

⚠️ Potential issue | 🟠 Major

These helpers are still rooted at the workspace, not the memory directory.

The new docs now promise memory-directory confinement, but resolve_existing_memory_path() and resolve_writable_memory_path() both join against workspace_dir. That means sibling paths like skills_data/... are still in scope. Either switch these helpers to resolve_memory_root() or tone the wording back to “workspace” so the docs match the actual boundary.

Also applies to: 375-405

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/openhuman/memory/ops.rs` around lines 354 - 355, The helper functions
resolve_existing_memory_path and resolve_writable_memory_path claim to confine
paths to the memory directory but currently join against workspace_dir, allowing
sibling paths; update these functions to use the actual memory root resolver
(e.g., call resolve_memory_root()) when joining and validating paths (and ensure
validate_memory_relative_path still checks against that memory root), or
alternatively change the docstrings/messages to say “workspace” instead of
“memory directory” so behavior and docs match—adjust any path joins and
validation logic in resolve_existing_memory_path, resolve_writable_memory_path
(and nearby code in the 375-405 range) to reference resolve_memory_root() and
enforce non-escaping of the memory directory.

[coderabbitai]

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

file="$(fd -p 'response_cache.rs' | head -n1)"
echo "Inspecting $file"
nl -ba "$file" | sed -n '228,290p'

python - <<'PY'
from pathlib import Path
import re

p = next(Path('.').rglob('response_cache.rs'))
text = p.read_text()

m = re.search(r'#\[cfg\(test\)\]\s*mod\s+tests\s*\{', text)
if not m:
    print("FAIL: cfg(test) mod tests block not found")
    raise SystemExit(1)

tail = text[m.start():]
opens = tail.count('{')
closes = tail.count('}')
print(f"opens_from_tests={opens}, closes_from_tests={closes}")

if closes < opens:
    print("FAIL: tests module appears unclosed")
    raise SystemExit(2)

print("PASS: brace count from tests block looks balanced")
PY

Repository: tinyhumansai/openhuman

Length of output: 175

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Find and inspect the file
file="src/openhuman/memory/response_cache.rs"

if [ ! -f "$file" ]; then
    echo "File not found: $file"
    exit 1
fi

echo "=== File inspection (lines 220-280) ==="
head -n 280 "$file" | tail -n 60 | cat -n

echo ""
echo "=== Python analysis ==="
python3 - "$file" <<'PY'
import sys
from pathlib import Path

filepath = sys.argv[1]
text = Path(filepath).read_text()
lines = text.split('\n')

# Find the cfg(test) mod tests block
for i, line in enumerate(lines, 1):
    if '#[cfg(test)]' in line and 'mod tests' in lines[i] if i < len(lines) else False:
        print(f"Found #[cfg(test)] mod tests block starting around line {i}")
        print(f"Lines {i} to {i+2}: {repr(lines[i-1:i+2])}")
        break

# Count braces from the tests block start
import re
m = re.search(r'#\[cfg\(test\)\]\s*mod\s+tests\s*\{', text)
if m:
    print(f"\nTests block starts at position {m.start()}")
    tail = text[m.start():]
    opens = tail.count('{')
    closes = tail.count('}')
    print(f"Brace balance from tests block: opens={opens}, closes={closes}")
    if closes >= opens:
        print("Status: Braces appear balanced")
    else:
        print("Status: Unclosed brace detected")
        print(f"\nLast 500 chars of file:\n{text[-500:]}")
else:
    print("cfg(test) mod tests block not found")

# Show total lines
print(f"\nTotal lines in file: {len(lines)}")
PY

Repository: tinyhumansai/openhuman

Length of output: 3268

---

🏁 Script executed:

# Check if the file compiles
cd /repo && cargo check --manifest-path src/openhuman/Cargo.toml 2>&1 | head -n 100

Repository: tinyhumansai/openhuman

Length of output: 121

---

🏁 Script executed:

#!/bin/bash
# Find the correct Cargo.toml and try to check the code
find . -name "Cargo.toml" -path "*/openhuman/*" | head -n 3

# Try cargo check from repo root on the specific file
cargo check 2>&1 | grep -A 20 "response_cache.rs" | head -n 30

Repository: tinyhumansai/openhuman

Length of output: 152

---

🏁 Script executed:

# Verify the exact state of the file ending
tail -n 10 src/openhuman/memory/response_cache.rs | cat -n

Repository: tinyhumansai/openhuman

Length of output: 336

---

Close mod tests block and restore actual test implementations.

The mod tests { block opens at line 235 but never closes—this is a compile error. The file ends at line 237 with only a placeholder comment. Replace the placeholder with concrete tests validating cache behavior (insert, retrieve, eviction, stats, clear operations), or move them to an integration test module.

Suggested fix

 #[cfg(test)]
 mod tests {
-    // ... (tests remain unchanged)
+    use super::*;
+
+    #[test]
+    fn test_cache_basic_operations() {
+        // Test insert, retrieve, etc.
+    }
+}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/openhuman/memory/response_cache.rs` around lines 234 - 236, The tests
module "mod tests" is left unclosed causing a compile error; close the module
and replace the placeholder with concrete unit tests that exercise the
ResponseCache API (e.g., create a ResponseCache, call insert, get/lookup to
verify retrieval, trigger eviction behavior and assert capacity eviction, verify
stats counters via stats() and test clear() to ensure emptiness), or
alternatively move those test cases into an integration test file; ensure the
"mod tests" block is properly terminated with a closing brace and include tests
named to reflect operations (insert/retrieve/evict/stats/clear) referencing the
ResponseCache type and its methods.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Rustfmt gate is failing in this file.

CI indicates formatting differences in the updated rustdoc/comment sections. Please run cargo fmt --all and re-push.

As per coding guidelines src/**/*.rs: Run cargo fmt and cargo check before merging changes to Rust code.

Also applies to: 10-13, 53-56, 75-79

🧰 Tools

🪛 GitHub Actions: Type Check

[error] 1-1: cargo fmt --all -- --check failed due to formatting differences in module-level rustdoc comment around line 1.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/openhuman/skills/ops.rs` around lines 1 - 5, Rustfmt failure: run cargo
fmt --all to reformat src/openhuman/skills/ops.rs (fixing rustdoc/comment
spacing changes around the module-level comments and the blocks mentioned) and
then run cargo check to ensure no other issues; update the commit and push.
Specifically, reformat the module docs in ops.rs and any nearby comment blocks
referenced (lines ~10-13, 53-56, 75-79) so they match rustfmt style, then re-run
cargo check before pushing the changes.

[coderabbitai]

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 (7)

src/openhuman/skills/registry_ops.rs (1)

230-243: ⚠️ Potential issue | 🟠 Major

Cleanup may delete pre-existing skill installation on checksum failure.

If the skill was already installed and the user attempts an update, a checksum mismatch will delete the existing working installation (line 237), leaving the user with no skill at all.

The issue: create_dir_all at line 214 succeeds silently if the directory exists, so there's no distinction between a newly-created directory and a pre-existing one.

🔧 Suggested fix: Track whether directory was newly created

     let skill_dir = workspace_dir.join("skills").join(skill_id);
+    let skill_dir_existed = skill_dir.exists();
     std::fs::create_dir_all(&skill_dir).map_err(|e| format!("failed to create skill dir: {e}"))?;
 
     // Fetch manifest (local or remote)
     ...
 
     // Verify checksum if present to ensure integrity of the downloaded bundle
     if let Some(expected) = &entry.checksum_sha256 {
         let mut hasher = Sha256::new();
         hasher.update(&js_bytes);
         let actual = format!("{:x}", hasher.finalize());
         if actual != *expected {
-            // Clean up the directory if verification fails
-            let _ = std::fs::remove_dir_all(&skill_dir);
+            // Clean up only if we created the directory (avoid deleting existing installation)
+            if !skill_dir_existed {
+                let _ = std::fs::remove_dir_all(&skill_dir);
+            }
             return Err(format!(
                 "checksum mismatch for '{skill_id}': expected {expected}, got {actual}"
             ));
         }

Alternatively, consider installing to a temporary directory first, verifying the checksum, then atomically moving to the final location.

🤖 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 230 - 243, The
checksum-failure cleanup currently unconditionally removes skill_dir (used in
the download/install flow where create_dir_all may have reused an existing
install), which can delete a pre-existing installation; modify the install logic
in registry_ops.rs to avoid removing an existing skill_dir on checksum mismatch
by either (a) tracking whether create_dir_all actually created the directory
(set a flag like dir_was_new when you call create_dir_all/use metadata) and only
remove the directory if it was newly created, or (b) download and write js_bytes
into a temporary directory/file (e.g., temp_dir_for_skill) and verify
entry.checksum_sha256 against the temp artifact, then atomically rename/move the
temp_dir to skill_dir only on success; update error handling around checksum
verification (the block referencing hasher.finalize(), actual, expected, and the
std::fs::remove_dir_all(&skill_dir) call) to implement one of these approaches.

src/openhuman/skills/qjs_skill_instance/event_loop/mod.rs (1)

516-518: ⚠️ Potential issue | 🔴 Critical

Missing import causes compile failure: handle_server_event not in scope.

The pipeline failures confirm that handle_server_event is called but not imported. The AI summary indicates the import line was removed while the call remained.

🐛 Fix: Add the missing import

 use super::js_handlers::{
-    call_lifecycle, handle_cron_trigger, handle_js_call, handle_js_void_call,
+    call_lifecycle, handle_cron_trigger, handle_js_call, handle_js_void_call, handle_server_event,
     read_pending_tool_result, start_async_tool_call,
 };

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/openhuman/skills/qjs_skill_instance/event_loop/mod.rs` around lines 516 -
518, The call to handle_server_event in the SkillMessage::ServerEvent arm is
failing because handle_server_event is not imported; re-add the missing
use/import for handle_server_event (the function that handles server events)
from its defining module so it is in scope for mod.rs, ensuring the import
matches the module where handle_server_event is declared and bringing the symbol
into scope for the async call in the event loop.

src/openhuman/memory/ingestion.rs (3)

392-402: ⚠️ Potential issue | 🟠 Major

Update entity_type when the higher-confidence classification wins.

When the same normalized name is seen again with a stronger score, this code only updates confidence. The stale entity_type is then reused for relation validation and returned in the final payload, so a later PERSON correction can stay stuck as TOPIC or WORK_ITEM.

🔧 Minimal fix

         if confidence > entry.confidence {
             entry.confidence = confidence;
+            entry.entity_type = entity_type.to_string();
         }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/openhuman/memory/ingestion.rs` around lines 392 - 402, The code inserts
or fetches a RawEntity into self.entities keyed by resolved_name but only
updates entry.confidence when a higher confidence is seen, leaving
entry.entity_type stale; change the logic in the block that checks if confidence
> entry.confidence (the entry variable created via
self.entities.entry(resolved_name.clone()).or_insert_with(...) and using the
local entity_type and confidence variables) to also assign entry.entity_type =
entity_type.to_string() when replacing the confidence so the entity_type is
synchronized with the higher-confidence classification.

---

1646-1653: ⚠️ Potential issue | 🟠 Major

extract_graph computes enriched metadata but never writes it back.

put_doc() stores the raw NamespaceDocumentInput first and the ingestion queue later calls this path. Here we recompute kind, extracted tags, and ingestion metadata via enrich_document_metadata(...), then discard the enriched document and only return tags. Documents inserted through put_doc() therefore diverge from ingest_document() and keep stale document metadata unless there is a separate metadata-update path.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/openhuman/memory/ingestion.rs` around lines 1646 - 1653, The code calls
enrich_document_metadata(document, &parsed, config) in extract_graph but throws
away the enriched result (only returning tags), so documents created via
put_doc() never get their recomputed kind/tags/ingestion metadata persisted and
diverge from ingest_document(). Fix by taking the enriched metadata returned
from enrich_document_metadata and writing it back to the document store
immediately after upsert_graph_relations (e.g., call the same persistence/update
routine used by put_doc()/put_document or add a new method like
update_document_metadata on self), ensuring the sanitized namespace and
document_id are used to persist the enriched kind, tags and ingestion fields
instead of discarding them.

---

289-293: ⚠️ Potential issue | 🟠 Major

Preserve REVIEWS / WORKS_ON instead of collapsing them to OWNS.

This branch rewrites "WORKS_ON", "RESPONSIBLE_FOR", and "REVIEWS" to the canonical predicate OWNS. That destroys relation semantics: for example, will_review_regex() later emits "reviews", but the stored edge becomes OWNS, so reviewer queries can never distinguish reviewers from owners.

🛠️ Suggested predicate split

-        "OWNS" | "WORKS_ON" | "RESPONSIBLE_FOR" | "REVIEWS" => RelationRule {
-            canonical: "OWNS",
+        "OWNS" => RelationRule {
+            canonical: "OWNS",
+            allowed_head: PERSON_TYPES,
+            allowed_tail: ORG_TYPES,
+        },
+        "WORKS_ON" => RelationRule {
+            canonical: "WORKS_ON",
+            allowed_head: PERSON_TYPES,
+            allowed_tail: ORG_TYPES,
+        },
+        "RESPONSIBLE_FOR" => RelationRule {
+            canonical: "RESPONSIBLE_FOR",
+            allowed_head: PERSON_TYPES,
+            allowed_tail: ORG_TYPES,
+        },
+        "REVIEWS" => RelationRule {
+            canonical: "REVIEWS",
             allowed_head: PERSON_TYPES,
             allowed_tail: ORG_TYPES,
         },

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/openhuman/memory/ingestion.rs` around lines 289 - 293, The match arm that
collapses "OWNS" | "WORKS_ON" | "RESPONSIBLE_FOR" | "REVIEWS" into a single
RelationRule with canonical "OWNS" loses semantics; instead add separate
branches for "WORKS_ON", "RESPONSIBLE_FOR", and "REVIEWS" (in addition to
"OWNS") each returning a RelationRule with an appropriate canonical value (e.g.,
"WORKS_ON", "RESPONSIBLE_FOR", "REVIEWS") and the same allowed_head/allowed_tail
(PERSON_TYPES and ORG_TYPES) so downstream logic like will_review_regex() can
distinguish reviewers from owners and queries retain correct predicates.

src/openhuman/skills/quickjs_libs/qjs_ops/ops_state.rs (2)

113-115: ⚠️ Potential issue | 🔴 Critical

Confine data_read / data_write to data_dir.

PathBuf::join is not a sandbox: absolute paths replace the base, and .. segments can escape it. As written, a skill can read or overwrite arbitrary files reachable by the process even though this bridge is documented as staying within the skill's data directory. Resolve the final path and verify it stays under sc.data_dir before performing I/O.

Also applies to: 127-129

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/openhuman/skills/quickjs_libs/qjs_ops/ops_state.rs` around lines 113 -
115, The data_read/data_write closures (the move |filename: String| closures
that use sc.data_dir.join(&filename)) currently allow absolute paths and “..” to
escape the skill data directory; fix by resolving the candidate path to an
absolute/canonical path (e.g., std::fs::canonicalize or equivalent) and the
canonicalized sc.data_dir, then verify the candidate path starts_with the
canonicalized sc.data_dir before doing any I/O, returning a JS error if the
check fails; apply the same canonicalize+starts_with guard to both the read
closure at the closure that uses sc.data_dir.join(&filename) and the similar
write closure around lines 127–129 so all file accesses are confined to
sc.data_dir.

---

184-210: ⚠️ Potential issue | 🟠 Major

memory_insert now reports success before the write is durable.

put_doc() already persists the document and only offloads graph extraction to the ingestion queue. Spawning it here turns storage failures into log-only events and makes immediate follow-up reads race the background task. If this API should stay fire-and-forget, it needs an explicit async status/ack surface instead of returning Ok(()).

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/openhuman/skills/quickjs_libs/qjs_ops/ops_state.rs` around lines 184 -
210, The current memory_insert spawns client.put_doc in a background task
(tokio::spawn) which logs failures only and reports success before the write is
durable; change memory_insert to await client.put_doc(...) directly (remove the
tokio::spawn) so errors are propagated to the caller and immediate reads are
consistent, or alternatively implement an explicit async ack/status surface on
memory_insert that returns the put_doc Result; locate the tokio::spawn block
around client.put_doc and NamespaceDocumentInput in the memory_insert code path
and either await the call or change the function signature to return a Result
acknowledging persistence.

🧹 Nitpick comments (4)

src/openhuman/skills/manifest.rs (3)

53-65: Validation implementation is correct; note silent failure in discovery path.

The custom deserializer is well-implemented and provides a clear error message. However, based on qjs_engine.rs:315-335, manifest deserialization errors during skill discovery are silently skipped (the match expression moves to the next skill without logging). This means skills with empty auth.modes will be silently excluded during discovery rather than surfacing an actionable error.

Consider adding a log::warn! in the error branch of qjs_engine.rs:315-335 to help skill authors diagnose configuration issues, though this is outside the scope of this file.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/openhuman/skills/manifest.rs` around lines 53 - 65, The deserializer
deserialize_non_empty_modes correctly rejects empty auth.modes, but
deserialization errors are silently ignored during skill discovery; update the
match/error handling in the manifest deserialization site (the code that calls
Manifest::deserialize in qjs_engine.rs around the manifest parsing/match block)
to log a warning when deserialization fails—use log::warn! (including the skill
identifier/path and the error message) in the Err arm so authors see why a skill
was skipped while keeping the existing flow that continues to the next skill.

---

110-112: Consider clarifying the runtime field documentation.

The default runtime is "v8" and is_javascript() accepts "v8", "javascript", and "quickjs" as equivalent. Given that the actual skills runtime uses QuickJS (per coding guidelines), the documentation could clarify that this field indicates JavaScript compatibility rather than the literal engine used.

📝 Suggested clarification

-    /// The runtime environment required by the skill. Defaults to "v8".
+    /// The runtime environment required by the skill. Defaults to "v8".
+    /// Accepted JavaScript runtimes: "v8", "javascript", "quickjs" (all use the QuickJS engine internally).

As per coding guidelines: "Skills runtime uses QuickJS (rquickjs) in src/openhuman/skills/, not V8/deno_core".

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/openhuman/skills/manifest.rs` around lines 110 - 112, Update the runtime
field documentation on the manifest struct to clarify that the runtime string
denotes JavaScript compatibility rather than the literal JS engine: mention that
the default is "v8", that is_javascript() treats "v8", "javascript", and
"quickjs" as equivalent, and that the actual skills runtime uses QuickJS
(rquickjs) per coding guidelines; adjust the doc comment above pub runtime:
String and/or default_runtime() description accordingly so consumers understand
it's a compatibility selector, not a direct engine choice.

---

20-21: Minor: Inconsistent doc comment capitalization.

Doc comments at lines 20, 75, and 126 start with lowercase "human-readable" while other doc comments in this file start with capital letters. Consider capitalizing for consistency.

📝 Suggested fix

-    /// human-readable label for the setup process.
+    /// Human-readable label for the setup process.

Similar changes at lines 75 and 126.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/openhuman/skills/manifest.rs` around lines 20 - 21, Change the
lowercase-leading doc comment for the field `label` (pub label: Option<String>)
to start with an uppercase letter, and make the same capitalization fix for the
two other doc comments in this file that currently start with lowercase (the
ones noted in the review); ensure all field/section doc comments in
src/openhuman/skills/manifest.rs consistently begin with a capitalized first
word.

src/openhuman/skills/ops.rs (1)

81-84: Consider logging when the skills directory cannot be read.

Returning an empty Vec on any read_dir error silently hides permission errors or other I/O issues. Adding a tracing::warn! or similar would aid debugging without changing the return type.

💡 Optional improvement

     let entries = match std::fs::read_dir(&skills_dir) {
         Ok(entries) => entries,
-        Err(_) => return Vec::new(),
+        Err(e) => {
+            tracing::debug!("Could not read skills directory {}: {e}", skills_dir.display());
+            return Vec::new();
+        }
     };

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/openhuman/skills/ops.rs` around lines 81 - 84, The read_dir error is
being silenced; update the match that assigns entries (the let entries = match
std::fs::read_dir(&skills_dir) { ... }) to log the error before returning an
empty Vec: capture the Err as e and call tracing::warn! (or similar) with a
message that includes the error and the skills_dir path, then keep returning
Vec::new() to preserve behavior; ensure you import tracing if not already used.

🤖 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/memory/ingestion.rs`:
- Around line 52-57: The MemoryIngestionConfig struct currently fails
deserialization when fields like model_name are omitted; add #[serde(default)]
to the MemoryIngestionConfig declaration and implement Default for
MemoryIngestionConfig (providing a sensible default for model_name and any other
fields) so serde can fill missing fields during partial config overrides; update
the struct annotated with #[serde(rename_all = "camelCase")] to also have
#[serde(default)] and implement Default for MemoryIngestionConfig to return safe
defaults.

In `@src/openhuman/memory/rpc_models.rs`:
- Around line 248-253: The DTO exposes min_retention and as_of but
memory_recall_memories currently ignores them (only forwarding namespace and
resolved_limit()), which can silently return unfiltered results; update the code
so either (A) the memory_recall_memories handler reads rpc_models::min_retention
and rpc_models::as_of and forwards them into the backend call/path that performs
retention/time filtering (ensure the backend API/signature is updated to accept
and apply these parameters), or (B) remove/deprecate these fields from the
public DTO by making min_retention and as_of non-serialized (e.g., serde(skip) /
remove from the struct) and return a validation error in memory_recall_memories
if callers supply them; reference the rpc_models struct fields min_retention and
as_of and the memory_recall_memories function and resolved_limit() helper when
making the change so callers cannot request silent no-ops.

In `@src/openhuman/skills/quickjs_libs/qjs_ops/ops_state.rs`:
- Around line 166-172: The rejection stems from hard-coded validation of
input.source_type in ops_state.rs: remove or relax the strict matches check
around source_type (the block using
input.source_type.unwrap_or_else(...).to_ascii_lowercase() and the matches!
call) so that JS skills can pass through other provenance values like "gmail",
"notion", "file", "web", etc.; either delete the matches! validation entirely or
replace it with a permissive whitelist/config hook if you must restrict values,
and keep the js_err call only for truly invalid/malformed inputs rather than
unknown provenance strings.
- Around line 25-30: The fields created_at/updated_at from JsMemoryInsertInput
are being deserialized but dropped because NamespaceDocumentInput lacks storage
and put_doc ignores them; update the write path so put_doc (and the conversion
into NamespaceDocumentInput) preserves timestamps by mapping created_at and
updated_at into the document metadata (e.g., add them into the metadata map or a
metadata struct) when constructing NamespaceDocumentInput from
JsMemoryInsertInput, ensuring JsMemoryInsertInput -> NamespaceDocumentInput
conversion and the put_doc call both forward those values; alternatively, if not
supported, remove/deprecate created_at/updated_at from JsMemoryInsertInput and
update associated docs and tests to avoid silently discarding them.

---

Outside diff comments:
In `@src/openhuman/memory/ingestion.rs`:
- Around line 392-402: The code inserts or fetches a RawEntity into
self.entities keyed by resolved_name but only updates entry.confidence when a
higher confidence is seen, leaving entry.entity_type stale; change the logic in
the block that checks if confidence > entry.confidence (the entry variable
created via self.entities.entry(resolved_name.clone()).or_insert_with(...) and
using the local entity_type and confidence variables) to also assign
entry.entity_type = entity_type.to_string() when replacing the confidence so the
entity_type is synchronized with the higher-confidence classification.
- Around line 1646-1653: The code calls enrich_document_metadata(document,
&parsed, config) in extract_graph but throws away the enriched result (only
returning tags), so documents created via put_doc() never get their recomputed
kind/tags/ingestion metadata persisted and diverge from ingest_document(). Fix
by taking the enriched metadata returned from enrich_document_metadata and
writing it back to the document store immediately after upsert_graph_relations
(e.g., call the same persistence/update routine used by put_doc()/put_document
or add a new method like update_document_metadata on self), ensuring the
sanitized namespace and document_id are used to persist the enriched kind, tags
and ingestion fields instead of discarding them.
- Around line 289-293: The match arm that collapses "OWNS" | "WORKS_ON" |
"RESPONSIBLE_FOR" | "REVIEWS" into a single RelationRule with canonical "OWNS"
loses semantics; instead add separate branches for "WORKS_ON",
"RESPONSIBLE_FOR", and "REVIEWS" (in addition to "OWNS") each returning a
RelationRule with an appropriate canonical value (e.g., "WORKS_ON",
"RESPONSIBLE_FOR", "REVIEWS") and the same allowed_head/allowed_tail
(PERSON_TYPES and ORG_TYPES) so downstream logic like will_review_regex() can
distinguish reviewers from owners and queries retain correct predicates.

In `@src/openhuman/skills/qjs_skill_instance/event_loop/mod.rs`:
- Around line 516-518: The call to handle_server_event in the
SkillMessage::ServerEvent arm is failing because handle_server_event is not
imported; re-add the missing use/import for handle_server_event (the function
that handles server events) from its defining module so it is in scope for
mod.rs, ensuring the import matches the module where handle_server_event is
declared and bringing the symbol into scope for the async call in the event
loop.

In `@src/openhuman/skills/quickjs_libs/qjs_ops/ops_state.rs`:
- Around line 113-115: The data_read/data_write closures (the move |filename:
String| closures that use sc.data_dir.join(&filename)) currently allow absolute
paths and “..” to escape the skill data directory; fix by resolving the
candidate path to an absolute/canonical path (e.g., std::fs::canonicalize or
equivalent) and the canonicalized sc.data_dir, then verify the candidate path
starts_with the canonicalized sc.data_dir before doing any I/O, returning a JS
error if the check fails; apply the same canonicalize+starts_with guard to both
the read closure at the closure that uses sc.data_dir.join(&filename) and the
similar write closure around lines 127–129 so all file accesses are confined to
sc.data_dir.
- Around line 184-210: The current memory_insert spawns client.put_doc in a
background task (tokio::spawn) which logs failures only and reports success
before the write is durable; change memory_insert to await client.put_doc(...)
directly (remove the tokio::spawn) so errors are propagated to the caller and
immediate reads are consistent, or alternatively implement an explicit async
ack/status surface on memory_insert that returns the put_doc Result; locate the
tokio::spawn block around client.put_doc and NamespaceDocumentInput in the
memory_insert code path and either await the call or change the function
signature to return a Result acknowledging persistence.

In `@src/openhuman/skills/registry_ops.rs`:
- Around line 230-243: The checksum-failure cleanup currently unconditionally
removes skill_dir (used in the download/install flow where create_dir_all may
have reused an existing install), which can delete a pre-existing installation;
modify the install logic in registry_ops.rs to avoid removing an existing
skill_dir on checksum mismatch by either (a) tracking whether create_dir_all
actually created the directory (set a flag like dir_was_new when you call
create_dir_all/use metadata) and only remove the directory if it was newly
created, or (b) download and write js_bytes into a temporary directory/file
(e.g., temp_dir_for_skill) and verify entry.checksum_sha256 against the temp
artifact, then atomically rename/move the temp_dir to skill_dir only on success;
update error handling around checksum verification (the block referencing
hasher.finalize(), actual, expected, and the std::fs::remove_dir_all(&skill_dir)
call) to implement one of these approaches.

---

Nitpick comments:
In `@src/openhuman/skills/manifest.rs`:
- Around line 53-65: The deserializer deserialize_non_empty_modes correctly
rejects empty auth.modes, but deserialization errors are silently ignored during
skill discovery; update the match/error handling in the manifest deserialization
site (the code that calls Manifest::deserialize in qjs_engine.rs around the
manifest parsing/match block) to log a warning when deserialization fails—use
log::warn! (including the skill identifier/path and the error message) in the
Err arm so authors see why a skill was skipped while keeping the existing flow
that continues to the next skill.
- Around line 110-112: Update the runtime field documentation on the manifest
struct to clarify that the runtime string denotes JavaScript compatibility
rather than the literal JS engine: mention that the default is "v8", that
is_javascript() treats "v8", "javascript", and "quickjs" as equivalent, and that
the actual skills runtime uses QuickJS (rquickjs) per coding guidelines; adjust
the doc comment above pub runtime: String and/or default_runtime() description
accordingly so consumers understand it's a compatibility selector, not a direct
engine choice.
- Around line 20-21: Change the lowercase-leading doc comment for the field
`label` (pub label: Option<String>) to start with an uppercase letter, and make
the same capitalization fix for the two other doc comments in this file that
currently start with lowercase (the ones noted in the review); ensure all
field/section doc comments in src/openhuman/skills/manifest.rs consistently
begin with a capitalized first word.

In `@src/openhuman/skills/ops.rs`:
- Around line 81-84: The read_dir error is being silenced; update the match that
assigns entries (the let entries = match std::fs::read_dir(&skills_dir) { ... })
to log the error before returning an empty Vec: capture the Err as e and call
tracing::warn! (or similar) with a message that includes the error and the
skills_dir path, then keep returning Vec::new() to preserve behavior; ensure you
import tracing if not already used.

🪄 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: 62ffe27a-3018-4beb-93b5-e83a28bb3e9f

📥 Commits

Reviewing files that changed from the base of the PR and between 0d198e4 and e064016.

📒 Files selected for processing (20)

• src/main.rs
• src/openhuman/memory/chunker.rs
• src/openhuman/memory/embeddings.rs
• src/openhuman/memory/ingestion.rs
• src/openhuman/memory/relex.rs
• src/openhuman/memory/rpc_models.rs
• src/openhuman/memory/store/memory_trait.rs
• src/openhuman/skills/bridge/net.rs
• src/openhuman/skills/manifest.rs
• src/openhuman/skills/mod.rs
• src/openhuman/skills/ops.rs
• src/openhuman/skills/qjs_engine.rs
• src/openhuman/skills/qjs_skill_instance/event_loop/mod.rs
• src/openhuman/skills/qjs_skill_instance/event_loop/webhook_handler.rs
• src/openhuman/skills/qjs_skill_instance/js_handlers.rs
• src/openhuman/skills/quickjs_libs/qjs_ops/ops_state.rs
• src/openhuman/skills/registry_ops.rs
• src/openhuman/skills/skill_registry.rs
• src/openhuman/skills/socket_manager.rs
• src/openhuman/skills/types.rs

✅ Files skipped from review due to trivial changes (9)

• src/openhuman/skills/mod.rs
• src/openhuman/skills/types.rs
• src/openhuman/skills/qjs_skill_instance/event_loop/webhook_handler.rs
• src/openhuman/skills/qjs_skill_instance/js_handlers.rs
• src/openhuman/memory/embeddings.rs
• src/openhuman/memory/relex.rs
• src/openhuman/memory/chunker.rs
• src/openhuman/skills/socket_manager.rs
• src/openhuman/skills/qjs_engine.rs

🚧 Files skipped from review as they are similar to previous changes (3)

• src/main.rs
• src/openhuman/skills/bridge/net.rs
• src/openhuman/memory/store/memory_trait.rs

[coderabbitai]

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

# First, locate and read the ingestion.rs file to see the current state
fd "ingestion.rs" src/openhuman/

Repository: tinyhumansai/openhuman

Length of output: 99

---

🏁 Script executed:

# Get the file size to determine how to read it
wc -l src/openhuman/memory/ingestion.rs

Repository: tinyhumansai/openhuman

Length of output: 104

---

🏁 Script executed:

# Read the struct definition and surrounding code
head -100 src/openhuman/memory/ingestion.rs | cat -n

Repository: tinyhumansai/openhuman

Length of output: 4281

---

🏁 Script executed:

# Search for doc_ingest function to understand config handling
rg "doc_ingest" src/openhuman/memory/

Repository: tinyhumansai/openhuman

Length of output: 655

---

🏁 Script executed:

# Search for how MemoryIngestionConfig is used in deserialization/unwrap_or_default
rg "MemoryIngestionConfig" src/openhuman/memory/ -A 3 -B 1

Repository: tinyhumansai/openhuman

Length of output: 7207

---

🏁 Script executed:

# Check for relation_rule function mentioned in scratchpad
rg "relation_rule" src/openhuman/memory/

Repository: tinyhumansai/openhuman

Length of output: 250

---

🏁 Script executed:

# Search for ExtractionAccumulator and add_entity
rg "ExtractionAccumulator" src/openhuman/memory/ -A 5

Repository: tinyhumansai/openhuman

Length of output: 2631

---

🏁 Script executed:

# Search for doc_ingest implementation in ops.rs
rg "pub async fn doc_ingest" src/openhuman/memory/ops.rs -A 15

Repository: tinyhumansai/openhuman

Length of output: 709

---

🏁 Script executed:

# Check MemoryIngestionRequest struct definition
rg "pub struct MemoryIngestionRequest" src/openhuman/memory/ingestion.rs -A 5

Repository: tinyhumansai/openhuman

Length of output: 276

---

🏁 Script executed:

# Search for any test cases that attempt partial config
rg "batchSize\|batch_size" src/openhuman/memory/ -B 3 -A 3

Repository: tinyhumansai/openhuman

Length of output: 48

---

🏁 Script executed:

# Check if there's any serde test or example
rg "json\|from_str\|from_value" src/openhuman/memory/ingestion.rs | head -20

Repository: tinyhumansai/openhuman

Length of output: 48

---

🏁 Script executed:

# Check the complete MemoryIngestionRequest definition
rg "pub struct MemoryIngestionRequest" src/openhuman/memory/ingestion.rs -A 10

Repository: tinyhumansai/openhuman

Length of output: 417

---

🏁 Script executed:

# Check how the config field is actually used - look at the Option type
rg "config: Option\|config: MemoryIngestionConfig" src/openhuman/memory/

Repository: tinyhumansai/openhuman

Length of output: 48

---

🏁 Script executed:

# Check the actual RPC handler to see if partial configs can be sent
rg "fn handle_doc_ingest" src/openhuman/memory/schemas.rs -A 20

Repository: tinyhumansai/openhuman

Length of output: 744

---

🏁 Script executed:

# Test serde deserialization behavior with a quick Python check
python3 << 'EOF'
# Simulate serde behavior: without struct-level default, partial object fails
# With field-level default on the option/field, missing field works but partial object still fails
test_scenarios = {
    "full_config": '{"modelName": "test", "batchSize": 32}',
    "partial_config": '{"batchSize": 32}',
    "empty_config": '{}',
    "missing_field": 'null'
}

print("Serde deserialization behavior analysis:")
print("=" * 60)
for scenario, json_str in test_scenarios.items():
    print(f"\nScenario: {scenario}")
    print(f"JSON input: {json_str}")
    if scenario == "full_config":
        print("Result: ✓ Success - all required fields present")
    elif scenario == "partial_config":
        print("Result: ✗ FAIL - 'modelName' required but missing in object")
    elif scenario == "empty_config":
        print("Result: ✗ FAIL - 'modelName' required but missing")
    elif scenario == "missing_field":
        print("Result: ✓ Success - #[serde(default)] uses Default impl")

print("\n" + "=" * 60)
print("KEY INSIGHT:")
print("- #[serde(default)] on field level allows MISSING field → uses Default")
print("- But if field is PRESENT but PARTIAL, struct deserialization fails")
print("- To allow partial objects, need #[serde(default)] at STRUCT level")
EOF

Repository: tinyhumansai/openhuman

Length of output: 873

---

🏁 Script executed:

# Check if there are RPC tests that send partial configs
fd "test" src/openhuman/memory/ | xargs grep -l "config\|Config" 2>/dev/null | head -5

Repository: tinyhumansai/openhuman

Length of output: 48

---

🏁 Script executed:

# Find IngestDocParams definition
rg "struct IngestDocParams" src/openhuman/memory/

Repository: tinyhumansai/openhuman

Length of output: 122

---

🏁 Script executed:

# If not found there, search more broadly
rg "IngestDocParams" src/openhuman/ -l

Repository: tinyhumansai/openhuman

Length of output: 125

---

🏁 Script executed:

# Check if config is part of the RPC params
rg "IngestDocParams\|struct.*Params" src/openhuman/memory/schemas.rs -B 2 -A 8

Repository: tinyhumansai/openhuman

Length of output: 48

---

🏁 Script executed:

# Verify the deserialization path - check what parse_params does
rg "parse_params" src/openhuman/memory/schemas.rs -B 2 -A 2

Repository: tinyhumansai/openhuman

Length of output: 4906

---

🏁 Script executed:

# Now read the IngestDocParams definition fully
rg "pub struct IngestDocParams" src/openhuman/memory/ops.rs -A 15

Repository: tinyhumansai/openhuman

Length of output: 533

---

🏁 Script executed:

# Get the full IngestDocParams definition including the config field
rg "pub struct IngestDocParams" src/openhuman/memory/ops.rs -A 20

Repository: tinyhumansai/openhuman

Length of output: 697

---

Add #[serde(default)] to allow partial ingestion config overrides.

MemoryIngestionConfig currently requires model_name during deserialization. When an RPC client sends a partial config (e.g., {"config": {"batchSize": 32}}), deserialization fails because model_name has no default. The RPC surface advertises optional config but rejects partial updates.

Fix: Add struct-level serde default

 #[derive(Debug, Clone, Serialize, Deserialize)]
-#[serde(rename_all = "camelCase")]
+#[serde(default, rename_all = "camelCase")]
 pub struct MemoryIngestionConfig {

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/openhuman/memory/ingestion.rs` around lines 52 - 57, The
MemoryIngestionConfig struct currently fails deserialization when fields like
model_name are omitted; add #[serde(default)] to the MemoryIngestionConfig
declaration and implement Default for MemoryIngestionConfig (providing a
sensible default for model_name and any other fields) so serde can fill missing
fields during partial config overrides; update the struct annotated with
#[serde(rename_all = "camelCase")] to also have #[serde(default)] and implement
Default for MemoryIngestionConfig to return safe defaults.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Don't expose min_retention / as_of as real filters if they're ignored.

memory_recall_memories only forwards namespace and resolved_limit(), and the tests below explicitly codify these fields as compatibility no-ops. Callers can therefore request retention/time filtering and silently get an unfiltered result. Either wire them through to the backend or reject/deprecate them on this DTO.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/openhuman/memory/rpc_models.rs` around lines 248 - 253, The DTO exposes
min_retention and as_of but memory_recall_memories currently ignores them (only
forwarding namespace and resolved_limit()), which can silently return unfiltered
results; update the code so either (A) the memory_recall_memories handler reads
rpc_models::min_retention and rpc_models::as_of and forwards them into the
backend call/path that performs retention/time filtering (ensure the backend
API/signature is updated to accept and apply these parameters), or (B)
remove/deprecate these fields from the public DTO by making min_retention and
as_of non-serialized (e.g., serde(skip) / remove from the struct) and return a
validation error in memory_recall_memories if callers supply them; reference the
rpc_models struct fields min_retention and as_of and the memory_recall_memories
function and resolved_limit() helper when making the change so callers cannot
request silent no-ops.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

createdAt / updatedAt are accepted but silently discarded.

These fields deserialize into JsMemoryInsertInput, but NamespaceDocumentInput has nowhere to store them and the put_doc call drops them entirely. That makes the public JS API lie about what is persisted. Either move them into metadata on write or remove/deprecate them until the store supports first-class timestamps.

Also applies to: 187-198

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/openhuman/skills/quickjs_libs/qjs_ops/ops_state.rs` around lines 25 - 30,
The fields created_at/updated_at from JsMemoryInsertInput are being deserialized
but dropped because NamespaceDocumentInput lacks storage and put_doc ignores
them; update the write path so put_doc (and the conversion into
NamespaceDocumentInput) preserves timestamps by mapping created_at and
updated_at into the document metadata (e.g., add them into the metadata map or a
metadata struct) when constructing NamespaceDocumentInput from
JsMemoryInsertInput, ensuring JsMemoryInsertInput -> NamespaceDocumentInput
conversion and the put_doc call both forward those values; alternatively, if not
supported, remove/deprecate created_at/updated_at from JsMemoryInsertInput and
update associated docs and tests to avoid silently discarding them.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Don't hard-code sourceType to doc|chat|email.

The memory subsystem already stores other source types in this PR (gmail, notion), and the RPC models also document values like file / web / note. This check prevents JS skills from preserving their real provenance and will reject existing integrations unnecessarily.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/openhuman/skills/quickjs_libs/qjs_ops/ops_state.rs` around lines 166 -
172, The rejection stems from hard-coded validation of input.source_type in
ops_state.rs: remove or relax the strict matches check around source_type (the
block using input.source_type.unwrap_or_else(...).to_ascii_lowercase() and the
matches! call) so that JS skills can pass through other provenance values like
"gmail", "notion", "file", "web", etc.; either delete the matches! validation
entirely or replace it with a permissive whitelist/config hook if you must
restrict values, and keep the js_err call only for truly invalid/malformed
inputs rather than unknown provenance strings.