feat(skills,node): integrate SKILL.md + managed Node runtime (#681)

[oxoxDev]

Summary

• Parse SKILL.md with YAML frontmatter aligned to the agentskills.io spec (name, description, license, compatibility, metadata, allowed-tools, plus unknown-field passthrough).
• Discover skills across three scopes: User (~/.openhuman/skills, ~/.agents/skills), Project (<workspace>/.openhuman/skills behind a trust marker), and Legacy (<workspace>/skills) — resources (scripts/, references/, assets/) inventoried per skill.
• Ship a managed Node.js runtime: detect compatible system node, otherwise download + SHASUMS256-verify + atomic-install into a user cache, under a bootstrap mutex.
• Expose two sandboxed tools — node_exec (run JS via the managed node) and npm_exec (run npm subcommands with a disallow-list for network-mutating actions) — behind a node.enabled config flag.
• Grant node_exec + npm_exec to the code_executor and tool_maker sub-agents.

Note: This is a draft PR / backend checkpoint. The user-facing UI for enabling a SKILL.md inside the app is not yet included — tracked as follow-up work.

Problem

Issue #681 asks for SKILL.md integration (spec: https://agentskills.io). Today openhuman can run QuickJS skills from a git submodule but cannot ingest a plain-markdown skill package that declares its tools + resources via YAML frontmatter. Separately, sub-agents lack a managed JavaScript runtime — without it, skills authored for node_exec/npm_exec cannot run portably across hosts.

Solution

Rust core — 14 micro-commits across 4 modules

Skills (src/openhuman/skills/)

• d121f32b build(skills): add serde_yaml for SKILL.md frontmatter
• 1e1f9657 feat(skills): parse SKILL.md frontmatter and add multi-path discovery
• 776aee34 fix(skills): align frontmatter with agentskills.io spec

Config (src/openhuman/config/schema/)

• 17cd8fac feat(config): add NodeConfig with env overrides (enable flag, version pin, cache dir, timeouts)

Node runtime (src/openhuman/node_runtime/)

• 0c4d62f2 build(runtime): add tar+xz2+zip for node runtime extraction
• b0775b49 feat(node_runtime): detect compatible system node on PATH
• da891639 feat(node_runtime): SHASUMS256-verified distribution downloader
• 57d5ffa2 feat(node_runtime): archive extraction + atomic install
• 5769e752 feat(node_runtime): bootstrap mutex + cache + bin path helper

Tools (src/openhuman/tools/impl/system/)

• ed4f4270 feat(tools): node_exec runs JS via managed node runtime
• 7892f7fe feat(tools): npm_exec runs npm via managed node runtime
• 16f80755 feat(tools): shell prepends managed node bin to PATH when cached
• 584a3bcb feat(tools): register node_exec + npm_exec behind node.enabled
• 793a2331 feat(agents): grant node_exec + npm_exec to code_executor + tool_maker

Security posture

• npm_exec disallow-list rejects publish, adduser, login, token, access, team, hook, profile, etc. — only local project operations are permitted.
• cwd override in both tools rejects .. / absolute paths escaping the workspace.
• Env is scrubbed to a SAFE_ENV_VARS allowlist before invoking node/npm.
• Downloader verifies SHA-256 against the upstream SHASUMS256.txt before atomic rename into cache.

Submission Checklist

• Unit tests — Resolver / downloader / exec tool coverage is planned as a follow-up (task C14). Current commits compile and pass cargo check --manifest-path app/src-tauri/Cargo.toml, but dedicated unit tests are not yet in this PR.
• E2E / integration — SKILL.md ingestion + node tool execution E2E against the mock backend is planned as a follow-up. Not yet in this PR.
• Doc comments — New modules (node_runtime/*, skills/ops.rs additions, node_exec, npm_exec) carry rustdoc on public items and module headers.
• Inline comments — Non-obvious paths (bootstrap mutex, SHASUMS lookup, disallow-list rationale, cwd escape check) are commented.
• N/A for UI tests — No frontend changes in this PR; UI wiring is the next checkpoint.

Impact

• Desktop-only surface: core additions. No Tauri shell changes.
• Opt-in: all new tools gated by node.enabled in config; off by default.
• Disk: managed node install is cached under the user cache dir (~60 MB per LTS pin).
• Network: first bootstrap fetches https://nodejs.org/dist/<ver>/ archive + SHASUMS256.txt. No runtime auto-update.
• Backwards compat: existing QuickJS skills flow untouched. skills/ legacy path still discovered.

Related

• Issue: Integrate skills.md #681
• Follow-up PR(s)/TODOs:
  • UI: Skills page section to enable/disable a discovered SKILL.md (separate PR).
  • Tests: C14 — unit coverage for resolver + downloader + exec tools.
  • Docs: C15 — user docs for node runtime + skills integration.
  • Safety: C13 — expand credential-handling guidance in docs/constitution.

Summary by CodeRabbit

• New Features

  • Execute JavaScript via a new node_exec tool and run npm commands via npm_exec.
  • Managed Node.js runtime with system detection, version handling, and optional automatic install; faster/robust runtime probing.

• Skills

  • SKILL.md with YAML frontmatter, multi-root discovery (project/user/legacy), deterministic resource inventory, and migration warnings.

• Configuration

  • New Node.js runtime settings: enable/disable, version pinning, cache directory, and prefer-system toggle.

[coderabbitai]

📝 Walkthrough

Walkthrough

Adds a managed Node.js runtime: config schema and env overrides, system detection with timeouts, verified download/extract/install, a memoized bootstrap resolver, two new tools (node_exec, npm_exec) with conditional registration and PATH injection, plus SKILL.md frontmatter parsing and multi-root skill discovery.

Changes

Cohort / File(s)	Summary
Dependencies Cargo.toml	Added serde_yaml, tar, xz2, zip, and wait-timeout for YAML frontmatter parsing, archive handling, zip support, and process timeouts.
Config schema & env src/openhuman/config/schema/node.rs, src/openhuman/config/schema/types.rs, src/openhuman/config/schema/mod.rs, src/openhuman/config/schema/load.rs	Introduced NodeConfig, added Config.node with serde defaults, and env overrides parsing for OPENHUMAN_NODE_* (including new parse_env_bool).
Node runtime core src/openhuman/mod.rs, src/openhuman/node_runtime/mod.rs, src/openhuman/node_runtime/resolver.rs, src/openhuman/node_runtime/downloader.rs, src/openhuman/node_runtime/extractor.rs, src/openhuman/node_runtime/bootstrap.rs	New node_runtime subsystem: system node discovery (with 5s probe timeout), version parsing, SHASUMS fetching, verified download, tar.xz & zip extraction, atomic install, and NodeBootstrap with memoized resolution and try_cached API.
Tools: node/npm exec src/openhuman/tools/impl/system/mod.rs, src/openhuman/tools/impl/system/node_exec.rs, src/openhuman/tools/impl/system/npm_exec.rs	Added NodeExecTool and NpmExecTool implementing execution schemas, security checks, workspace path validation, PATH injection via NodeBootstrap, timeouts, and output truncation.
Shell integration & registry src/openhuman/tools/impl/system/shell.rs, src/openhuman/tools/ops.rs	ShellTool can accept optional NodeBootstrap (uses non-blocking try_cached() to prepend managed bin to PATH). Tool registry conditionally constructs/registers Node tools when node.enabled is true; added unit tests for registry presence/absence.
Skills discovery src/openhuman/skills/ops.rs	Reworked skill loading to multi-root discovery (project/user/legacy), added SKILL.md YAML frontmatter parsing, resource inventorying, trust gating, name deduplication/precedence, and legacy skill.json fallback.
Agent configs & tests src/openhuman/agent/agents/.../agent.toml, src/openhuman/channels/tests/prompt.rs	Added node_exec and npm_exec to agents' named tool allowlists; updated test to use ..Default::default() in a Skill literal.

sequenceDiagram
    participant Client
    participant NodeBootstrap
    participant SystemResolver
    participant Downloader
    participant Extractor
    participant FileSystem

    Client->>NodeBootstrap: resolve()
    activate NodeBootstrap
    NodeBootstrap->>NodeBootstrap: check_cached()
    alt Cached exists
        NodeBootstrap-->>Client: ResolvedNode
    else
        alt prefer_system
            NodeBootstrap->>SystemResolver: detect_system_node(target_version)
            activate SystemResolver
            SystemResolver->>FileSystem: locate node, run `node --version` (5s timeout)
            SystemResolver-->>NodeBootstrap: Some(SystemNode) or None
            deactivate SystemResolver
        end
        alt SystemNode available
            NodeBootstrap-->>Client: ResolvedNode (System)
        else
            NodeBootstrap->>Downloader: fetch_shasums(version)
            activate Downloader
            Downloader->>FileSystem: GET SHASUMS256.txt
            Downloader-->>NodeBootstrap: shasums map
            deactivate Downloader

            NodeBootstrap->>Downloader: download_distribution(archive)
            activate Downloader
            Downloader->>FileSystem: stream download + SHA-256 verification
            Downloader-->>NodeBootstrap: verified archive
            deactivate Downloader

            NodeBootstrap->>Extractor: extract_distribution(archive)
            activate Extractor
            Extractor->>FileSystem: extract to staged dir (tar.xz or zip)
            Extractor-->>NodeBootstrap: staged top-level dir
            deactivate Extractor

            NodeBootstrap->>Extractor: atomic_install(staged, final_dest)
            activate Extractor
            Extractor->>FileSystem: rename staged→final (backup if exists)
            Extractor-->>NodeBootstrap: final_dest
            deactivate Extractor

            NodeBootstrap-->>Client: ResolvedNode (Managed)
        end
    end
    deactivate NodeBootstrap

Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~75 minutes

Possibly related issues

• Integrate skills.md #681: Implements Node.js runtime resolution/installer and node_exec/npm_exec tools, matching the issue objectives.

Possibly related PRs

• feat(agent): simplify harness + trim bundled prompts #500: Overlaps in adding node_exec/npm_exec and Node runtime integration; closely related to agent/tool allowlist changes.
• feat(agent): sub-agents, reasoning→agentic routing, layered context pipeline #474: Related changes to tool registry and runtime wiring; touches similar registration pathways.
• feat(agent): pure orchestrator pattern with per-skill delegation tools #496: Modifies tool registration/visibility and agent tool allowlists; intersects with this PR's registry updates.

Suggested reviewers

• senamakel

🐰
I hop through PATH and fetch the tar,
I curl the SHASUMS, check each char,
I unzip, atomically plant a tree—
Node and npm now run safe and free,
Hooray for runtimes, happy builds for me! 🥕✨

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately summarizes the main changes: SKILL.md integration and managed Node runtime are the primary features introduced in this comprehensive changeset.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.

_{✏️ 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.}

[senamakel]

banger PR. waiting to merge

[coderabbitai]

Actionable comments posted: 5

🧹 Nitpick comments (2)

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

502-514: Dead code: SKILL.md checks are unreachable in legacy loader.

load_from_legacy_manifest is only called when SKILL.md doesn't exist (see load_skill_dir lines 383-393). The skill_md.exists() checks here will always be false:

• Line 504: read_skill_md_description(&skill_md) will always return None (file read fails), falling back to "No description provided".
• Line 510: The conditional will always take the else branch.

Consider simplifying:

♻️ Proposed simplification

-    let skill_md = dir.join(SKILL_MD);
     let description = if manifest.description.is_empty() {
-        read_skill_md_description(&skill_md)
-            .unwrap_or_else(|| "No description provided".to_string())
+        "No description provided".to_string()
     } else {
         manifest.description
     };

-    let location = if skill_md.exists() {
-        Some(skill_md)
-    } else {
-        Some(manifest_path.to_path_buf())
-    };
+    let location = Some(manifest_path.to_path_buf());

🤖 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 502 - 514, The SKILL.md existence
checks in load_from_legacy_manifest are dead code because load_skill_dir only
calls load_from_legacy_manifest when SKILL.md is absent; remove the redundant
skill_md/SKILL_MD existence logic and the call to read_skill_md_description, and
instead directly use manifest.description (or the manifest_path fallback) for
description and location; update load_from_legacy_manifest to compute
description = if manifest.description.is_empty() { "No description
provided".to_string() } else { manifest.description } and set location =
Some(manifest_path.to_path_buf()), removing references to skill_md, SKILL_MD,
and read_skill_md_description.

src/openhuman/config/schema/load.rs (1)

796-824: Log invalid Node boolean env values and de-duplicate parsing.

At Line 797 and Line 817, invalid values are silently ignored. Please emit a warning (with env var name) and reuse one parser to keep behavior consistent.

♻️ Suggested refactor

+        let parse_bool_env = |name: &str, raw: &str| -> Option<bool> {
+            match raw.trim().to_ascii_lowercase().as_str() {
+                "1" | "true" | "yes" | "on" => Some(true),
+                "0" | "false" | "no" | "off" => Some(false),
+                _ => {
+                    tracing::warn!(env = %name, value = %raw, "invalid boolean env override ignored");
+                    None
+                }
+            }
+        };
+
         // Node runtime overrides
         if let Ok(flag) = std::env::var("OPENHUMAN_NODE_ENABLED") {
-            let normalized = flag.trim().to_ascii_lowercase();
-            match normalized.as_str() {
-                "1" | "true" | "yes" | "on" => self.node.enabled = true,
-                "0" | "false" | "no" | "off" => self.node.enabled = false,
-                _ => {}
+            if let Some(enabled) = parse_bool_env("OPENHUMAN_NODE_ENABLED", &flag) {
+                self.node.enabled = enabled;
             }
         }
@@
         if let Ok(flag) = std::env::var("OPENHUMAN_NODE_PREFER_SYSTEM") {
-            let normalized = flag.trim().to_ascii_lowercase();
-            match normalized.as_str() {
-                "1" | "true" | "yes" | "on" => self.node.prefer_system = true,
-                "0" | "false" | "no" | "off" => self.node.prefer_system = false,
-                _ => {}
+            if let Some(prefer_system) = parse_bool_env("OPENHUMAN_NODE_PREFER_SYSTEM", &flag) {
+                self.node.prefer_system = prefer_system;
             }
         }

As per coding guidelines: "Add substantial, development-oriented logs while implementing features or fixes so issues are easy to trace end-to-end; log critical checkpoints including ... branch decisions ... and error handling paths."

🤖 Prompt for AI Agents

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

In `@src/openhuman/config/schema/load.rs` around lines 796 - 824, The env-boolean
parsing for OPENHUMAN_NODE_ENABLED and OPENHUMAN_NODE_PREFER_SYSTEM silently
ignores invalid values and the parsing logic is duplicated; add a single helper
function (e.g., parse_env_bool or parse_bool_env) in load.rs that takes the env
var name, reads and normalizes the value, returns Result<bool, String> or
Option<bool>, and use it for both self.node.enabled and self.node.prefer_system;
when the helper encounters an unrecognized value, emit a warning log including
the env var name and the invalid value so callers see why the fallback occurred,
and replace the duplicated match blocks with calls to this helper to keep
behavior consistent.

🤖 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/node_runtime/bootstrap.rs`:
- Around line 245-253: resolve_from_system currently forwards SystemNode.version
(which includes a leading "v") into build_resolved, violating the
ResolvedNode::version contract; update resolve_from_system to normalize the
version by removing a leading 'v' or 'V' (e.g. via strip_prefix or
trim_start_matches) before calling build_resolved so ResolvedNode::version is
always stored without the leading "v" (refer to resolve_from_system, SystemNode,
build_resolved, and ResolvedNode::version).

In `@src/openhuman/node_runtime/downloader.rs`:
- Around line 181-217: The download loop can leave a truncated file at
target_path on failures from response.chunk(), file.write_all(), or a suppressed
file.flush(); modify the code in the download routine (the loop that calls
response.chunk(), file.write_all(), and the subsequent file.flush()) to ensure
the partial file is removed on any error path and to propagate flush errors
instead of ignoring them. Implement this by adding a cleanup step that calls
tokio::fs::remove_file(target_path).await in the error branch (or use a
scope/guard that removes the file on Err) and replace file.flush().await.ok()
with a propagated result (using ? or map_err) so flush failures trigger cleanup
and return an error, preserving the existing SHA mismatch removal behavior for
consistency. Ensure you reference target_path, response.chunk(), write_all(),
and file.flush() when making the changes.

In `@src/openhuman/node_runtime/extractor.rs`:
- Around line 181-205: The current sequence renames final_dest to a backup and
then performs fs::rename(&staged, &final_dest) but does not restore the backup
if that second rename fails; modify the code around the fs::rename(&staged,
&final_dest) call in extractor.rs so that on error you attempt to move the
backup path (the Option<PathBuf> named backup) back to final_dest before
returning the error: capture the rename error, if backup.is_some() attempt
fs::rename(&backup_path, &final_dest) (or fs::remove_dir_all then fs::rename as
appropriate) and log any restore failure as secondary, then return the original
error (with context) so the working runtime is restored to the previous install
when staged->final_dest fails.

In `@src/openhuman/node_runtime/resolver.rs`:
- Around line 128-145: The probe_node_version function currently calls
Command::output() which blocks indefinitely; replace that with spawning the
child and enforcing a real timeout: use
Command::new(path).arg("--version")...spawn() to get a Child, then use a timeout
API (e.g. wait_timeout::ChildExt::wait_timeout or an async tokio::time::timeout
alternative) with Duration::from_secs(5) to wait for the child; if the child
exits within the timeout, call child.wait_with_output() to capture stdout/stderr
and return the version string, otherwise kill the child (child.kill() and
child.wait()) and return None. Update references in probe_node_version to use
spawn, wait_timeout, kill, and wait_with_output accordingly.

In `@src/openhuman/tools/impl/system/node_exec.rs`:
- Around line 162-175: The code builds a shell command using script_path and
extra_args without validating they stay inside workspace_dir, allowing path
traversal like "../etc/passwd"; implement a resolver similar to
npm_exec.rs::resolve_cwd(): add a new resolve_script_path(workspace_dir,
script_path) that rejects absolute paths and any ".." components, canonicalizes
the joined path against workspace_dir, returns the safe relative/resolved path,
and use that resolved path (instead of the raw script_path) when calling
shell_quote in node_exec.rs (also apply the same validation to any extra_args
that represent file paths); reference inline_code, script_path, extra_args,
shell_quote, resolved.node_bin and replace usages accordingly.

---

Nitpick comments:
In `@src/openhuman/config/schema/load.rs`:
- Around line 796-824: The env-boolean parsing for OPENHUMAN_NODE_ENABLED and
OPENHUMAN_NODE_PREFER_SYSTEM silently ignores invalid values and the parsing
logic is duplicated; add a single helper function (e.g., parse_env_bool or
parse_bool_env) in load.rs that takes the env var name, reads and normalizes the
value, returns Result<bool, String> or Option<bool>, and use it for both
self.node.enabled and self.node.prefer_system; when the helper encounters an
unrecognized value, emit a warning log including the env var name and the
invalid value so callers see why the fallback occurred, and replace the
duplicated match blocks with calls to this helper to keep behavior consistent.

In `@src/openhuman/skills/ops.rs`:
- Around line 502-514: The SKILL.md existence checks in
load_from_legacy_manifest are dead code because load_skill_dir only calls
load_from_legacy_manifest when SKILL.md is absent; remove the redundant
skill_md/SKILL_MD existence logic and the call to read_skill_md_description, and
instead directly use manifest.description (or the manifest_path fallback) for
description and location; update load_from_legacy_manifest to compute
description = if manifest.description.is_empty() { "No description
provided".to_string() } else { manifest.description } and set location =
Some(manifest_path.to_path_buf()), removing references to skill_md, SKILL_MD,
and read_skill_md_description.

🪄 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: 7a5b9a6c-1f56-4999-a1a3-43d017fb7e40

📥 Commits

Reviewing files that changed from the base of the PR and between 6dc0078 and c8296ab.

⛔ Files ignored due to path filters (2)

• Cargo.lock is excluded by !**/*.lock
• app/src-tauri/Cargo.lock is excluded by !**/*.lock

📒 Files selected for processing (20)

• Cargo.toml
• src/openhuman/agent/agents/code_executor/agent.toml
• src/openhuman/agent/agents/tool_maker/agent.toml
• src/openhuman/channels/tests/prompt.rs
• src/openhuman/config/schema/load.rs
• src/openhuman/config/schema/mod.rs
• src/openhuman/config/schema/node.rs
• src/openhuman/config/schema/types.rs
• src/openhuman/mod.rs
• src/openhuman/node_runtime/bootstrap.rs
• src/openhuman/node_runtime/downloader.rs
• src/openhuman/node_runtime/extractor.rs
• src/openhuman/node_runtime/mod.rs
• src/openhuman/node_runtime/resolver.rs
• src/openhuman/skills/ops.rs
• src/openhuman/tools/impl/system/mod.rs
• src/openhuman/tools/impl/system/node_exec.rs
• src/openhuman/tools/impl/system/npm_exec.rs
• src/openhuman/tools/impl/system/shell.rs
• src/openhuman/tools/ops.rs

[coderabbitai]

Actionable comments posted: 5

🤖 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/node_runtime/bootstrap.rs`:
- Around line 230-235: probe_managed_install() is currently accepting whatever
build_resolved() returns even when npm is missing, causing a corrupted managed
cache to be reused; change probe_managed_install() (and the same logic around
the npm check used at the other site) to validate that npm_bin.exists() (and any
other required launchers) before accepting a managed install and return an
error/None when npm is absent so build_resolved() will trigger a reinstall;
specifically, update the code paths that inspect npm_bin (the npm launcher check
near the existing tracing::warn!) to fail the probe instead of only warning,
ensuring npm_exec can recover by forcing a fresh managed install.

In `@src/openhuman/node_runtime/resolver.rs`:
- Around line 116-125: The which_node() function currently returns the first
regular file named node but doesn't verify it is executable; change which_node
to skip non-executable matches by checking candidate metadata for execute
permission before returning. Implement an is_executable check used in
which_node: on Unix inspect metadata().permissions().mode() & 0o111 != 0, and on
Windows treat the existence of the .exe suffix (std::env::consts::EXE_SUFFIX) as
sufficient (or use metadata().is_file()); only return Some(candidate) when
is_executable(candidate) is true to avoid masking later valid installs.

In `@src/openhuman/skills/ops.rs`:
- Around line 563-569: The YAML parse failure is only logged and replaced with
SkillFrontmatter::default(), so the real parse error never appears in the
Skill.warnings shown in the catalog; update the
serde_yaml::from_str::<SkillFrontmatter>(&yaml) error branch to append a
diagnostic containing the parse error (err) to the Skill.warnings collection for
this skill (while still returning SkillFrontmatter::default()), so the concrete
parse message is surfaced in Skill.warnings instead of only a log line. Ensure
you reference the local frontmatter variable and push the formatted error into
the skill's warnings slice/vec where other warnings are collected.
- Around line 592-605: The walk_files function currently uses
path.is_dir()/is_file() which follow symlinks; change it to detect and skip
symlinks so you neither recurse into directory symlinks nor treat symlinked
files as bundled. Inside walk_files (and when iterating entries) call
entry.file_type() or std::fs::symlink_metadata(&path) and if
file_type.is_symlink() { continue; } then only recurse when file_type.is_dir()
and push only when file_type.is_file(); preserve existing error handling for
metadata/file_type calls.
- Around line 356-376: scan_root currently iterates std::fs::read_dir() which
yields entries in unspecified order causing nondeterministic winners when two
sibling folders declare the same frontmatter.name; change scan_root to collect
the directory entries into a Vec, filter to directories and non-dot names, sort
that Vec by a stable key (e.g. entry.file_name() as a string, optionally
case-insensitive) and then iterate the sorted list calling load_skill_dir(&path,
&dir_name, scope) so same-scope name collisions are resolved deterministically;
keep existing error handling where read_dir fails.

🪄 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: 42f0cec6-de2c-4514-bb92-5692d490462c

📥 Commits

Reviewing files that changed from the base of the PR and between c8296ab and 9f551c9.

⛔ Files ignored due to path filters (1)

• Cargo.lock is excluded by !**/*.lock

📒 Files selected for processing (8)

• Cargo.toml
• src/openhuman/config/schema/load.rs
• src/openhuman/node_runtime/bootstrap.rs
• src/openhuman/node_runtime/downloader.rs
• src/openhuman/node_runtime/extractor.rs
• src/openhuman/node_runtime/resolver.rs
• src/openhuman/skills/ops.rs
• src/openhuman/tools/impl/system/node_exec.rs

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

• src/openhuman/node_runtime/extractor.rs
• src/openhuman/node_runtime/downloader.rs
• src/openhuman/tools/impl/system/node_exec.rs

[coderabbitai]

Actionable comments posted: 5

🤖 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/node_runtime/bootstrap.rs`:
- Around line 123-130: The default cache_root should not be workspace-local:
change cache_root() so when config.cache_dir is empty it uses a user-owned OS
cache (e.g. dirs::cache_dir() or dirs_next::cache_dir() and join a stable
subpath like "openhuman/node-runtime") instead of
workspace_dir.join("node-runtime"); keep honor of an explicit config.cache_dir
when set. Add a concise rustdoc on cache_root() describing this policy and
ensure probe_managed_install() and any code that reuses workspace-local runtimes
only do so when an explicit, trusted override/flag is present (e.g. a new
allow_workspace_cache or explicit config.cache_dir value), updating
probe_managed_install() to check that flag before accepting a workspace-local
node/npm tree.

In `@src/openhuman/node_runtime/resolver.rs`:
- Around line 90-110: The current resolver accepts a system Node when majors
match but doesn't verify npm, causing cached System Node to break npm_exec on
distros that package npm separately; update the resolver logic (the code path
that returns Some(SystemNode) and constructs SystemNode) to verify npm is
present and usable before returning: perform a lightweight check (e.g., probe
for npm via the same mechanism used by npm_exec or run "npm --version") and if
the check fails, log a clear message and return None so the managed runtime will
be downloaded instead of caching NodeSource::System.

In `@src/openhuman/skills/ops.rs`:
- Around line 371-380: The directory discovery loop currently uses path.is_dir()
which follows symlinks; modify the loop that iterates entries (the code calling
entry.path(), path.is_dir(), and load_skill_dir) to explicitly skip symlinked
entries before treating them as directories: obtain the DirEntry's file type
(via entry.file_type() or path.symlink_metadata()) and if file_type.is_symlink()
is true then continue, otherwise proceed with the existing is_dir check and call
to load_skill_dir; this ensures symlinked skill directories are ignored.
- Around line 601-608: The function inventory_resources currently descends into
each resource root (RESOURCE_DIRS) after testing root.is_dir(), which follows
symlinks and can recurse into external trees; before calling walk_files(root,
...), call std::fs::symlink_metadata on the root Path and skip the directory if
its FileType reports is_symlink() (i.e., treat symlinked roots as non-existent),
so inventory_resources rejects symlinked top-level resource directories rather
than dereferencing them; update the logic around root, root.is_dir(), and the
walk_files invocation accordingly.
- Around line 221-234: The current shim load_skills(workspace_dir) omits
user-scoped directories and thus drops user-installed skills for existing
callers; update the callers in src/openhuman/agent/harness/session/builder.rs
and src/openhuman/channels/runtime/startup.rs to call discover_skills (or
discover_skills_inner with the appropriate user None/Some args) instead of
load_skills so user home paths (~/.openhuman/skills and ~/.agents/skills) are
included, or alternatively modify load_skills to delegate to discover_skills
(preserving workspace precedence) so its public behavior remains
backward-compatible while retaining user-scope discovery.

🪄 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: 9b23deb7-d072-47a9-a61c-48b15fa3bb1b

📥 Commits

Reviewing files that changed from the base of the PR and between 9f551c9 and 52a0609.

📒 Files selected for processing (3)

• src/openhuman/node_runtime/bootstrap.rs
• src/openhuman/node_runtime/resolver.rs
• src/openhuman/skills/ops.rs

[coderabbitai]

Actionable comments posted: 4

🧹 Nitpick comments (2)

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

264-297: Add debug/trace checkpoints in skill discovery flow.

The new multi-scope flow has key branch decisions (trusted gating, roots scanned, collision outcomes) but no debug/trace instrumentation, which makes “why didn’t my skill load?” hard to diagnose.

As per coding guidelines: "Use log / tracing at debug or trace level for development-oriented diagnostics on new or changed flows — log entry/exit points, branch decisions, external calls, retries, state transitions, and error paths".

🤖 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 264 - 297, Add debug/trace logging
to the skill discovery flow in discover_skills_inner: log entry/exit of
discover_skills_inner, the home_dir and workspace_dir presence, whether trusted
gating allowed project roots to be scanned, each root returned by user_roots()
and project_roots(), each call to scan_root() with the SkillScope, and collision
outcomes when absorb() overrides an existing entry in by_name (include the skill
name and scopes). Use the project logging/tracing facility at debug/trace level
and add logs around the final sort/return of out so callers can see the
resulting ordered skill list.

---

674-1028: Split this module to keep ops.rs focused and under size target.

src/openhuman/skills/ops.rs now combines discovery, parsing, resource walking, and a large test suite in one file (>1000 lines). Please split into sibling modules (e.g., discovery.rs, parser.rs, resources.rs, and test modules) to keep responsibilities isolated.

As per coding guidelines: "Keep source files ≤ ~500 lines; split modules when growing to maintain single responsibility".

🤖 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 674 - 1028, The ops.rs file is too
large and mixes discovery, parsing, and resource-walking code plus tests; split
responsibilities into sibling modules: create discovery.rs for discover_skills,
discover_skills_inner, load_skills_ws (test shim) and init_skills_dir; parser.rs
for parse_skill_md and frontmatter parsing helpers; resources.rs for
inventory_resources and related FS-walking helpers; move the corresponding unit
tests into each module (or into a tests submodule per module), keep test helpers
like write adjacent to tests, and update ops.rs (or src/openhuman/skills/mod.rs)
to pub use the moved functions (e.g., pub use discovery::{discover_skills,
init_skills_dir}; pub use parser::parse_skill_md; pub use
resources::inventory_resources) so external callers are unchanged; ensure
visibility (pub(crate)/pub) and imports (use crate::openhuman::skills::...) are
adjusted and run cargo test to fix any path or privacy errors.

🤖 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/node_runtime/bootstrap.rs`:
- Around line 220-223: After calling build_resolved(bin_dir, version,
NodeSource::Managed) for a fresh install, re-run the managed install validation
(the same checks probe_managed_install performs) against the extracted install
before accepting/caching it; if validation fails (e.g., npm launcher missing or
not a file), reject/cleanup the install and return an error instead of returning
the build result so resolve()/npm_exec cannot remain broken. Apply the same
re-validation to the other fresh-install path around the code at the second
affected block (the other build_resolved return).
- Around line 202-218: The bootstrap currently races across processes because
archive_path and scratch (created from process id) are shared; protect the
cache/version lifecycle by acquiring a filesystem lock scoped to the cache_root
+ dist.archive_name (or version) before downloading/extracting, and release it
after atomic_install completes; replace the PID-only scratch path with a truly
unique temp directory (e.g., create a uniquified temp dir via tempfile::Builder
or mkdtemp semantics) and use that for extract_distribution so concurrent
bootstraps do not remove each other’s staging; ensure download_distribution,
extract_distribution and atomic_install are executed while holding the file lock
and still clean up the unique scratch and archive_path after releasing the lock
as needed.

In `@src/openhuman/skills/ops.rs`:
- Around line 574-585: The loop that splits frontmatter vs body drops any
subsequent lines equal to '---' because the delimiter check runs
unconditionally; update the logic in the for loop (the block using variables
lines, terminated, yaml, body) so the trim-and-check for '---' only occurs when
!terminated — e.g., move the if line.trim() == "---" branch inside the
!terminated branch so that after terminated==true any '---' lines are appended
to body (body.push_str(line); body.push('\n')) instead of being skipped.
- Around line 358-362: Reject symlinked scan roots in scan_root by calling
std::fs::symlink_metadata(root) before attempting to read_dir and returning an
empty Vec if symlink_metadata fails or if metadata.file_type().is_symlink() is
true; update the top of fn scan_root to perform this check (use
std::fs::symlink_metadata and metadata.file_type().is_symlink()) and only call
std::fs::read_dir(root) afterwards so discovery will not traverse outside the
workspace when the provided root is a symlink.

---

Nitpick comments:
In `@src/openhuman/skills/ops.rs`:
- Around line 264-297: Add debug/trace logging to the skill discovery flow in
discover_skills_inner: log entry/exit of discover_skills_inner, the home_dir and
workspace_dir presence, whether trusted gating allowed project roots to be
scanned, each root returned by user_roots() and project_roots(), each call to
scan_root() with the SkillScope, and collision outcomes when absorb() overrides
an existing entry in by_name (include the skill name and scopes). Use the
project logging/tracing facility at debug/trace level and add logs around the
final sort/return of out so callers can see the resulting ordered skill list.
- Around line 674-1028: The ops.rs file is too large and mixes discovery,
parsing, and resource-walking code plus tests; split responsibilities into
sibling modules: create discovery.rs for discover_skills, discover_skills_inner,
load_skills_ws (test shim) and init_skills_dir; parser.rs for parse_skill_md and
frontmatter parsing helpers; resources.rs for inventory_resources and related
FS-walking helpers; move the corresponding unit tests into each module (or into
a tests submodule per module), keep test helpers like write adjacent to tests,
and update ops.rs (or src/openhuman/skills/mod.rs) to pub use the moved
functions (e.g., pub use discovery::{discover_skills, init_skills_dir}; pub use
parser::parse_skill_md; pub use resources::inventory_resources) so external
callers are unchanged; ensure visibility (pub(crate)/pub) and imports (use
crate::openhuman::skills::...) are adjusted and run cargo test to fix any path
or privacy errors.

🪄 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: 01688e20-2301-4515-b03e-53bfd93d1f69

📥 Commits

Reviewing files that changed from the base of the PR and between 52a0609 and 58be66c.

📒 Files selected for processing (3)

• src/openhuman/node_runtime/bootstrap.rs
• src/openhuman/node_runtime/resolver.rs
• src/openhuman/skills/ops.rs

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

• src/openhuman/node_runtime/resolver.rs

[coderabbitai]

⚠️ Potential issue | 🟠 Major

The shared cache still races across bootstraps/processes.

The tokio::sync::Mutex only protects callers sharing this NodeBootstrap. archive_path is global inside cache_root, and the PID-only scratch directory is not unique across bootstrap instances in the same process. Two concurrent installs can overwrite/remove the same archive or staging tree and turn a cold start into a corrupt cache or a spurious bootstrap failure. Please add a filesystem lock scoped to the cache/version, and switch the scratch path to a truly unique temp dir.

🤖 Prompt for AI Agents

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

In `@src/openhuman/node_runtime/bootstrap.rs` around lines 202 - 218, The
bootstrap currently races across processes because archive_path and scratch
(created from process id) are shared; protect the cache/version lifecycle by
acquiring a filesystem lock scoped to the cache_root + dist.archive_name (or
version) before downloading/extracting, and release it after atomic_install
completes; replace the PID-only scratch path with a truly unique temp directory
(e.g., create a uniquified temp dir via tempfile::Builder or mkdtemp semantics)
and use that for extract_distribution so concurrent bootstraps do not remove
each other’s staging; ensure download_distribution, extract_distribution and
atomic_install are executed while holding the file lock and still clean up the
unique scratch and archive_path after releasing the lock as needed.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Re-validate the just-installed tree before caching it.

probe_managed_install() already rejects installs where npm is missing or not a file, but the fresh-install path bypasses that guard and caches whatever build_resolved() returns. If extraction leaves a broken npm launcher or symlink, the first resolve() succeeds and npm_exec stays broken for the rest of the process.

Suggested fix

-        let bin_dir = managed_bin_dir(&install_dir);
-        let version = dist.version.trim_start_matches('v').to_string();
-        build_resolved(bin_dir, version, NodeSource::Managed)
+        probe_managed_install(&install_dir, &cache_root, &self.config.version)
+            .context("managed install validation failed after extraction")

Also applies to: 255-260

🤖 Prompt for AI Agents

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

In `@src/openhuman/node_runtime/bootstrap.rs` around lines 220 - 223, After
calling build_resolved(bin_dir, version, NodeSource::Managed) for a fresh
install, re-run the managed install validation (the same checks
probe_managed_install performs) against the extracted install before
accepting/caching it; if validation fails (e.g., npm launcher missing or not a
file), reject/cleanup the install and return an error instead of returning the
build result so resolve()/npm_exec cannot remain broken. Apply the same
re-validation to the other fresh-install path around the code at the second
affected block (the other build_resolved return).

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Reject symlinked scan roots before calling read_dir.

Line 359 dereferences root directly. If <workspace>/skills or <workspace>/.openhuman/skills is itself a symlink, discovery can traverse outside the workspace tree.

Proposed fix

 fn scan_root(root: &Path, scope: SkillScope) -> Vec<Skill> {
+    let meta = match std::fs::symlink_metadata(root) {
+        Ok(m) => m,
+        Err(_) => return Vec::new(),
+    };
+    if meta.file_type().is_symlink() || !meta.is_dir() {
+        return Vec::new();
+    }
+
     let entries = match std::fs::read_dir(root) {
         Ok(entries) => entries,
         Err(_) => 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 358 - 362, Reject symlinked scan
roots in scan_root by calling std::fs::symlink_metadata(root) before attempting
to read_dir and returning an empty Vec if symlink_metadata fails or if
metadata.file_type().is_symlink() is true; update the top of fn scan_root to
perform this check (use std::fs::symlink_metadata and
metadata.file_type().is_symlink()) and only call std::fs::read_dir(root)
afterwards so discovery will not traverse outside the workspace when the
provided root is a symlink.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Do not drop --- lines from Markdown body after frontmatter ends.

At Line 575, the delimiter check runs even after termination, so body lines equal to --- are silently removed. That mutates valid SKILL.md content.

Proposed fix

-    for line in lines {
-        if line.trim() == "---" {
-            terminated = true;
-            continue;
-        }
-        if !terminated {
+    for line in lines {
+        if !terminated && line.trim() == "---" {
+            terminated = true;
+            continue;
+        }
+        if !terminated {
             yaml.push_str(line);
             yaml.push('\n');
         } else {
             body.push_str(line);
             body.push('\n');
         }
     }

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	for line in lines {
	if line.trim() == "---" {
	terminated = true;
	continue;
	}
	if !terminated {
	yaml.push_str(line);
	yaml.push('\n');
	} else {
	body.push_str(line);
	body.push('\n');
	}
	for line in lines {
	if !terminated && line.trim() == "---" {
	terminated = true;
	continue;
	}
	if !terminated {
	yaml.push_str(line);
	yaml.push('\n');
	} else {
	body.push_str(line);
	body.push('\n');
	}
	}

🤖 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 574 - 585, The loop that splits
frontmatter vs body drops any subsequent lines equal to '---' because the
delimiter check runs unconditionally; update the logic in the for loop (the
block using variables lines, terminated, yaml, body) so the trim-and-check for
'---' only occurs when !terminated — e.g., move the if line.trim() == "---"
branch inside the !terminated branch so that after terminated==true any '---'
lines are appended to body (body.push_str(line); body.push('\n')) instead of
being skipped.