From 3da80d852eaad27c8380a95dcbedfda77b4e4884 Mon Sep 17 00:00:00 2001 From: oxoxDev <164490987+oxoxDev@users.noreply.github.com> Date: Tue, 21 Apr 2026 23:29:21 +0530 Subject: [PATCH] feat(skills,node): integrate SKILL.md + managed Node runtime (#681) (#723) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * build(skills): add serde_yaml for SKILL.md frontmatter (#681) Co-Authored-By: Claude Opus 4.7 * feat(skills): parse SKILL.md frontmatter and add multi-path discovery (#681) Extends the skills module with agentskills.io-style discovery: * Parses YAML frontmatter in SKILL.md (name, description, version, author, tags, allowed-tools, license) via serde_yaml, with a catch-all extras map for forward-compatible keys. * Adds SkillScope (User / Project / Legacy) and discover_skills(), which scans ~/.openhuman/skills, ~/.agents/skills, /.openhuman/ skills, /.agents/skills and the legacy /skills directory. * Gates project-scope loading behind an explicit /.openhuman/trust marker (is_workspace_trusted helper). * Resolves name collisions with project > user > legacy precedence and surfaces shadowing as per-skill warnings. * Inventories bundled resources under scripts/, references/, assets/. * Keeps the existing load_skills(workspace_dir) API as a backwards-compatible wrapper for existing callers. * Preserves legacy skill.json fallback (marked legacy = true). * Lenient validation: missing / mismatched / oversized name or description produce warnings instead of errors. * Adds 12 unit tests covering frontmatter parsing, trust gating, collision shadowing, lenient fallbacks, legacy JSON and resource inventory. One existing prompt test updated to use ..Default::default() for the expanded struct. Co-Authored-By: Claude Opus 4.7 * fix(skills): align frontmatter with agentskills.io spec (#681) Per the agentskills.io SKILL.md spec, only name, description, license, compatibility, metadata, and allowed-tools are valid top-level keys. Our SkillFrontmatter had version, author, and tags as top-level fields, which drifted from the spec and would silently swallow mis-shaped data for skills authored against the canonical schema. Demote version/author/tags into the metadata map. Non-spec top-level keys still parse via #[serde(flatten)] into extra, and when present there we emit a migration warning and still populate the derived Skill fields so existing skills keep working. Add compatibility as an optional string alongside license. New tests cover the spec-compliant metadata shape, the deprecated top-level fallback path, and verify that the full spec frontmatter parses without leaking into extras. Co-Authored-By: Claude Opus 4.7 * build(runtime): add tar+xz2+zip for node runtime extraction (#681) Node.js distributions ship as .tar.xz on Unix and .zip on Windows. The upcoming Node runtime bootstrap extracts these in pure Rust; xz2 is built with the `static` feature so liblzma is bundled and not a system dependency. zip is pinned to default-features = false + deflate to avoid pulling in bzip2/zstd/aes which we don't need for Node archives. Deps only — no behavior change. Co-Authored-By: Claude Opus 4.7 * feat(config): add NodeConfig with env overrides (#681) Introduce managed Node.js runtime configuration so skills that require `node`/`npm` (agentskills.io packages with build steps) can be wired in behind a single config switch. Fields: - `node.enabled` — master switch (default true) - `node.version` — pinned release (default `v22.11.0` LTS) - `node.cache_dir` — absolute path for managed distributions; empty = use workspace default - `node.prefer_system` — reuse matching system `node` when found (default true); set false for reproducible CI / airgapped deploys Env overrides land in load.rs alongside the existing LOCAL_AI_TIER block: - OPENHUMAN_NODE_ENABLED - OPENHUMAN_NODE_VERSION - OPENHUMAN_NODE_CACHE_DIR - OPENHUMAN_NODE_PREFER_SYSTEM No resolver / downloader yet — subsequent commits in the #681 series consume this config to bootstrap the runtime. Co-Authored-By: Claude Opus 4.6 * feat(node_runtime): detect compatible system node on PATH (#681) Introduce the `openhuman::node_runtime` module and its first piece: a synchronous resolver that walks `PATH`, probes `node --version`, and returns a `SystemNode` when the host toolchain major-version matches the configured target. Why resolve first: a successful probe lets the bootstrap skip a ~60 MB download per managed install. Matching is intentionally loose on the patch level (Node LTS lines are ABI-stable and skills pin their own deps via package-lock.json); operators needing strict pinning can set `node.prefer_system = false`. Tracing is verbose by design — resolver decisions gate the download path, so operators need a clear breadcrumb trail at `debug`/`info`. Includes unit tests for `parse_node_version` covering `v` prefix, whitespace, major-only, and malformed inputs. Co-Authored-By: Claude Opus 4.6 * feat(node_runtime): SHASUMS256-verified distribution downloader (#681) Add the second piece of the managed Node.js runtime: streaming download of OS/arch-specific prebuilt archives from nodejs.org, gated by a SHA-256 match against the release's signed `SHASUMS256.txt`. Key contracts: - `NodeDistribution::for_host(version)` picks the right archive for the current OS/arch tuple. Supported matrix: * darwin-{arm64,x64}.tar.xz * linux-{arm64,x64,armv7l}.tar.xz * win-{arm64,x64}.zip Unsupported hosts surface a clear error so the caller can flip `node.enabled = false` or point at a pre-installed toolchain. - `fetch_shasums(client, version)` parses `SHASUMS256.txt` into a filename -> hex digest map. Tolerant of trailing / signature blocks. - `download_distribution(...)` streams chunks into the target path, computes SHA-256 on the fly, and wipes the partial file if the digest does not match. Integrity check is mandatory — skills will execute untrusted code inside the resolved runtime. Verbose tracing at `info` and `debug` follows the repo debug-logging rule; operators can grep `[node_runtime::downloader]` to trace every GET, chunk boundary (via total-bytes log), and hash decision. Co-Authored-By: Claude Opus 4.6 * feat(node_runtime): archive extraction + atomic install (#681) Extract downloaded Node.js distributions and move them into the final cache path without leaving the reader observing a half-populated directory. - `extract_distribution(archive, extract_root, is_zip)` — wraps the synchronous `tar`/`xz2`/`zip` crates in `spawn_blocking` and returns the single top-level folder produced by the archive (`node-vX.Y.Z- -/`). `set_preserve_permissions(true)` + `set_overwrite( true)` on the tar side keeps the `node` binary's `+x` bit. The zip path restores Unix mode bits via `unix_mode()` so cross-OS builds still land correct permissions. - `atomic_install(staged, final_dest)` — renames a staged directory into place via a single `rename(2)`, moving any pre-existing install to a `.old-` sibling first and cleaning it up on success. This gives concurrent readers a "before" or "after" view, never a partial one. - Unsafe zip paths (`enclosed_name()` rejection) are logged and skipped rather than traversed — defence against malicious archives, even though the digest check in the downloader already rules out tampered official releases. No new tests here: the logic leans on upstream tar/zip crates, and meaningful end-to-end coverage requires a real archive on disk — that comes in the integration-test commit later in this series. Co-Authored-By: Claude Opus 4.6 * feat(node_runtime): bootstrap mutex + cache + bin path helper (#681) Stitch resolver, downloader, and extractor into a single idempotent entry point callers use at startup (or lazily before the first `node_exec`/`npm_exec` call). `NodeBootstrap::resolve()` contract: 1. Return cached `ResolvedNode` if a previous call already succeeded. 2. Bail when `node.enabled = false`. 3. If `node.prefer_system`, probe the host PATH. Matching major version wins — return a `System`-sourced `ResolvedNode`. 4. Otherwise compute the install path (`{cache_dir}/node-v{version}-{os}-{arch}/`). If it already contains valid bins, reuse it. This makes the bootstrap ~free across restarts once a managed install lands. 5. Else fetch `SHASUMS256.txt`, locate the expected digest for our archive, stream the download, extract into a `.stage-` scratch dir, `atomic_install` the top-level folder into the final path, and remove scratch + archive to reclaim disk. Concurrency is handled by a `tokio::sync::Mutex>` — parallel callers queue behind the first one; the winner memoises, the losers pick up the cached result. No race can produce two concurrent downloads of the same archive. Platform-specific bin layout lives in `managed_bin_dir` / `build_ resolved`: - Unix: `/bin/{node,npm}` - Windows: `/{node.exe,npm.cmd}` This closes the resolver/downloader/cache layer promised in the #681 checkpoint. Tools (`node_exec`, `npm_exec`) and shell-PATH injection layer on top in the next batch of commits. Co-Authored-By: Claude Opus 4.6 * feat(tools): node_exec runs JS via managed node runtime (#681) Executes `inline_code` (via `node -e`) or a workspace-relative `script_path` through the NodeBootstrap-resolved `node` binary. POSIX single-quote quoting keeps user input inert; `env_clear` + allow-list mirrors the shell tool so secrets never leak. 300s default timeout (capped at 1800s), 1MB stdout/stderr caps. PATH is prepended with the resolved bin dir so child processes see managed `node`/`npm`. Co-Authored-By: Claude Opus 4.7 * feat(tools): npm_exec runs npm via managed node runtime (#681) Thin npm CLI wrapper paired with node_exec. Subcommands go through `is_sane_subcommand` (alphanumerics + `._-:` only) and a deny-list (publish/adduser/login/token/…) blocks registry-mutation and auth flows. `cwd` is resolved under the workspace — absolute paths and `..` components are rejected. 600s default timeout (1800s ceiling), 1MB stdout/stderr caps. PATH prepended with managed bin dir so npm's own node/corepack lookups hit the managed toolchain. Co-Authored-By: Claude Opus 4.7 * feat(tools): shell prepends managed node bin to PATH when cached (#681) Adds a non-blocking `NodeBootstrap::try_cached()` primitive that peeks the memoised `ResolvedNode` without holding the async lock or triggering a download. ShellTool gains an optional `node_bootstrap` field wired through a new `with_node_bootstrap` constructor; when set, each shell invocation consults `try_cached()` and, on a hit, prepends the managed bin dir to the child PATH using the platform separator. Unrelated shell commands stay byte-identical — no download is ever forced from the shell path. Consequence: once `node_exec`/`npm_exec` have resolved the toolchain once, skills that shell out to `node`/`npm`/`npx`/`corepack` transparently pick up the managed install without any per-command coordination. Co-Authored-By: Claude Opus 4.7 * feat(tools): register node_exec + npm_exec behind node.enabled (#681) Wires the skills-oriented Node.js toolchain into the tool registry: * Constructs one session-scoped `NodeBootstrap` in `all_tools_with_runtime` when `root_config.node.enabled` is true — all three consumers (ShellTool, NodeExecTool, NpmExecTool) share the same `Arc` so the download/extract/install pipeline runs at most once per session and the memoised `ResolvedNode` is reused across every shelling-out call. * Swaps ShellTool to `with_node_bootstrap(...)` when the bootstrap exists so shell PATH injection fires automatically once any node/npm tool has resolved the runtime. * Registers `node_exec` and `npm_exec` only when the flag is on — flipping `node.enabled = false` cleanly removes both tools and falls back to the legacy ShellTool construction. * Adds two regression tests (`all_tools_registers_node_exec_when_node_enabled` and `all_tools_excludes_node_exec_when_node_disabled`) so future refactors can't silently drop the wiring. Co-Authored-By: Claude Opus 4.7 * feat(agents): grant node_exec + npm_exec to code_executor + tool_maker (#681) code_executor is the sandboxed developer sub-agent — it writes, runs, and debugs code — and tool_maker is the narrow self-healing agent that polyfills missing commands. Both now see the managed Node.js tools so skills and polyfills can call into the resolved runtime directly rather than relying on whatever `node`/`npm` happens to be on the host PATH. Co-Authored-By: Claude Opus 4.6 * style(core): apply cargo fmt auto-fixes (#681) Co-Authored-By: Claude Opus 4.7 * chore(cargo): sync Cargo.lock to OpenHuman v0.52.26 after rebase (#681) Co-Authored-By: Claude Opus 4.7 * fix(node_runtime): strip leading v/V defensively in resolve_from_system (#681) CodeRabbit R1 flagged that build_resolved receives a raw version string from SystemNode. detect_system_node already trims the prefix, but trim defensively at the resolve_from_system boundary too so any future code path constructing SystemNode with an un-normalised version won't emit "vvX.Y.Z". Co-Authored-By: Claude Opus 4.6 * fix(node_runtime): propagate flush errors and clean up partial archives (#681) CodeRabbit R2 flagged that download_distribution silently swallowed flush errors via `.ok()` and left partial files on disk when any chunk/write/flush step errored. Wrap the streaming loop in an async block returning Result<()>, propagate flush errors, and on any failure delete the partial archive so a retry starts clean and callers never see a half-written file. Co-Authored-By: Claude Opus 4.6 * fix(node_runtime): restore previous install on staged rename failure (#681) CodeRabbit R3 flagged that atomic_install left the user with no Node runtime on disk if the staged->final rename failed after a backup had been taken. Wrap the rename in `if let Err`, restore the backup if present, log restore failures separately (warning), and always return the original error so the caller sees the true failure cause. Co-Authored-By: Claude Opus 4.6 * fix(node_runtime): enforce real 5s timeout on node --version probe (#681) CodeRabbit R4 flagged that probe_node_version used Command::output() with no real timeout — a broken shim or FUSE-backed binary could hang the bootstrap forever. Add wait-timeout crate dep and rewrite the probe to spawn with piped stdio, wait_timeout(5s), kill on timeout, and read stdout/stderr from the piped handles after exit. Logs a warning when a probe times out so the install can be diagnosed. Co-Authored-By: Claude Opus 4.6 * fix(tools): reject script_path escapes in node_exec (#681) CodeRabbit R5 flagged that node_exec joined user-supplied script_path onto the workspace without rejecting `..` segments or absolute paths, letting a prompt-injected `../../../etc/passwd` read arbitrary files via node. Add a resolve_script_path helper mirroring npm_exec::resolve_cwd — reject empty, absolute, parent-dir, and Windows-prefix components. Extra positional args stay opaque (shell-quoted, not path-checked) and get an explanatory comment at the call site. Unit tests cover each rejection case plus the relative- subdir happy path. Co-Authored-By: Claude Opus 4.6 * refactor(skills): drop dead SKILL.md branch in legacy manifest loader (#681) CodeRabbit N1 flagged dead code: load_from_legacy_manifest only runs when load_skill_dir already determined SKILL.md is absent, so the fallback that re-checked for SKILL.md and its helper read_skill_md_description could never execute. Simplify to a direct description/location decision and delete the dead helper. Co-Authored-By: Claude Opus 4.6 * refactor(config): extract parse_env_bool helper with warn on unknown values (#681) CodeRabbit N2 flagged two near-identical boolean env-override match blocks for OPENHUMAN_NODE_ENABLED and OPENHUMAN_NODE_PREFER_SYSTEM. Extract a module-level parse_env_bool helper that accepts 1/true/yes/on and 0/false/no/off (case-insensitive) and emits a tracing::warn when the value is unrecognised so silent mis-spellings don't invisibly leave the config unchanged. Co-Authored-By: Claude Opus 4.6 * fix(node_runtime): detect corrupted cache missing npm (#723) `probe_managed_install` now verifies the npm launcher exists before reusing an extracted install. A download interrupted after `node` was extracted but before `npm` would otherwise be cached forever and `npm_exec` could never self-heal — now the corrupted cache forces a fresh download via the normal resolve path. Addresses CodeRabbit review feedback on PR #723. Co-Authored-By: Claude Opus 4.7 * fix(node_runtime): skip non-executable PATH shims when probing node (#723) `which_node` previously returned the first matching filename on `PATH` regardless of its execute bit. A non-executable `node` placeholder earlier in `PATH` (e.g. an unprivileged shim left by a failed install) would mask a valid later install and force the managed runtime download. Now checks `file && (mode & 0o111 != 0)` on Unix to mirror shell `which` behaviour. Windows remains unchanged — `.exe` suffix already encodes executability for the loader. Addresses CodeRabbit review feedback on PR #723. Co-Authored-By: Claude Opus 4.7 * fix(skills): deterministic entry order in scan_root (#723) `read_dir` order is unspecified by the OS. When two sibling skill directories declare the same logical `frontmatter.name` (which can differ from the folder name), cross-scope/same-scope deduplication downstream would pick a non-deterministic winner across runs. Sort entries by on-disk directory name for a stable, reproducible order so collision resolution is deterministic. Addresses CodeRabbit review feedback on PR #723. Co-Authored-By: Claude Opus 4.7 * fix(skills): surface YAML parse errors as skill warnings (#723) `parse_skill_md` previously swallowed the `serde_yaml` error via `log::warn!` and fell back to an empty `SkillFrontmatter`, then the catalog reported a generic "could not parse — exposing directory as placeholder". Skill authors had no way to see the real cause without scraping logs. Return parse-level diagnostics as a third tuple element. `load_from_skill_md` merges them into the skill's user-visible `warnings`, so the catalog now surfaces the actual YAML error (e.g. "frontmatter parse error: mapping values are not allowed here at line 3 column 12"). Addresses CodeRabbit review feedback on PR #723. Co-Authored-By: Claude Opus 4.7 * fix(skills): skip symlinks when walking skill resources (#723) `walk_files` used `is_dir()` / `is_file()` which transparently follow symlinks. Two failure modes: 1. **Unbounded recursion** — a skill resource symlink that points back at an ancestor (e.g. `resources/self -> resources/`) would cause infinite traversal, eventually blowing the stack. 2. **Silent out-of-tree leakage** — a symlink pointing at `/`, `/etc`, or another skill's directory would enumerate its contents into the current skill's resource listing. Switch to `entry.file_type()` and skip symlinks before descending. Directories and regular files behave as before. Addresses CodeRabbit review feedback on PR #723. Co-Authored-By: Claude Opus 4.7 * fix(node_runtime): default managed cache to user-owned OS cache (#723) Default `cache_root()` to `dirs::cache_dir()/openhuman/node-runtime/` so a repo cannot ship a checked-in `./node-runtime/` and have the bootstrap reuse it as a trusted managed install. Explicit `config.cache_dir` still wins; workspace-local falls back only when `dirs::cache_dir()` is unavailable, and we emit a warning on that fallback. As a second line of defence, `probe_managed_install()` now canonicalises both the install dir and the cache root and refuses any install that escapes the cache tree. Co-Authored-By: Claude Opus 4.6 * fix(node_runtime): require npm when accepting system node (#723) On distros that package `node` and `npm` separately (Debian/Ubuntu, Alpine `nodejs-current`, some NixOS setups) the host `node` can be present without `npm`. `npm_exec` then fails every call because the resolved `SystemNode` has no usable npm launcher. Before returning `Some(SystemNode)`, locate `npm` on `PATH` and probe `npm --version` through the same `wait_timeout` path as the node probe. Either missing gate falls back to the managed download flow. Co-Authored-By: Claude Opus 4.6 * fix(skills): surface user-scope skills via load_skills (#723) Existing production callers (`agent::harness::session::builder`, `channels::runtime::startup`) reach the skill catalog via `load_skills(workspace_dir)`. Previously this shim passed `None` for the home directory, so skills installed under `~/.openhuman/skills/` and `~/.agents/skills/` were silently dropped even after the multi-scope discovery landed in `discover_skills`. Delegate to `discover_skills_inner` with `dirs::home_dir()` so user-scope skills reach the runtime; project-scope still wins on name collision. Tests stay hermetic via a `load_skills_ws` helper that preserves the old workspace-only semantics. A new `load_skills_surfaces_user_scope` test drives a tempdir-as-home through `discover_skills` and asserts the user-scope skill is returned with `SkillScope::User`. Addresses CodeRabbit review comment 3116332244. Co-Authored-By: Claude Opus 4.6 * fix(skills): skip symlinked skill dirs during scan (#723) `scan_root` previously accepted any entry where `path.is_dir()` returned true. `is_dir()` dereferences symlinks, so a link from `/foo -> /some/external/tree` would load as a legitimate skill even though `walk_files` already rejects symlinks deeper in the resource walker. Attacker-authored symlinks in a skills root would therefore have one remaining escape hatch. Switch to `entry.file_type()` (non-dereferencing) and reject both symlinked and non-directory entries at the top level. Treat a failed `file_type()` probe as "not safe to traverse" and skip. A new `symlinked_skill_dirs_are_skipped` test (Unix-only, since symlinks are the platform guarantee being tested) creates an external skill tempdir, links it into the workspace skills root, and asserts `load_skills_ws` returns empty. Addresses CodeRabbit review comment 3116332252. Co-Authored-By: Claude Opus 4.6 * fix(skills): reject symlinked resource roots (#723) `inventory_resources` gated each resource sub-root (`scripts/`, `references/`, `assets/`) on `root.is_dir()`. `is_dir()` follows symlinks, so a `scripts -> /etc` link inside a skill directory would pass the check and `walk_files` would inventory the external tree. Deeper symlinks inside the walk were already rejected by `walk_files`, but the root-level check was the missing layer. Use `std::fs::symlink_metadata` for a non-dereferencing probe and reject roots whose own `file_type().is_symlink()` returns true. Treat any `symlink_metadata` error as a non-existent root and skip. A new `symlinked_resource_roots_are_rejected` test (Unix-only) links a skill's `assets/` sub-root to an external tempdir with a file inside and asserts `inventory_resources` returns empty. Addresses CodeRabbit review comment 3116332260. Co-Authored-By: Claude Opus 4.6 --------- Co-authored-by: Claude Opus 4.7 --- Cargo.lock | 154 ++- Cargo.toml | 10 + .../agent/agents/code_executor/agent.toml | 2 +- .../agent/agents/tool_maker/agent.toml | 2 +- src/openhuman/channels/tests/prompt.rs | 1 + src/openhuman/config/schema/load.rs | 43 + src/openhuman/config/schema/mod.rs | 2 + src/openhuman/config/schema/node.rs | 52 + src/openhuman/config/schema/types.rs | 5 + src/openhuman/mod.rs | 1 + src/openhuman/node_runtime/bootstrap.rs | 361 ++++++ src/openhuman/node_runtime/downloader.rs | 280 +++++ src/openhuman/node_runtime/extractor.rs | 235 ++++ src/openhuman/node_runtime/mod.rs | 21 + src/openhuman/node_runtime/resolver.rs | 290 +++++ src/openhuman/skills/ops.rs | 1059 ++++++++++++++--- src/openhuman/tools/impl/system/mod.rs | 4 + src/openhuman/tools/impl/system/node_exec.rs | 345 ++++++ src/openhuman/tools/impl/system/npm_exec.rs | 363 ++++++ src/openhuman/tools/impl/system/shell.rs | 51 +- src/openhuman/tools/ops.rs | 135 ++- 21 files changed, 3265 insertions(+), 151 deletions(-) create mode 100644 src/openhuman/config/schema/node.rs create mode 100644 src/openhuman/node_runtime/bootstrap.rs create mode 100644 src/openhuman/node_runtime/downloader.rs create mode 100644 src/openhuman/node_runtime/extractor.rs create mode 100644 src/openhuman/node_runtime/mod.rs create mode 100644 src/openhuman/node_runtime/resolver.rs create mode 100644 src/openhuman/tools/impl/system/node_exec.rs create mode 100644 src/openhuman/tools/impl/system/npm_exec.rs diff --git a/Cargo.lock b/Cargo.lock index b17c67fb4..3590980a2 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -189,6 +189,15 @@ dependencies = [ "syn 2.0.117", ] +[[package]] +name = "arbitrary" +version = "1.4.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "c3d036a3c4ab069c7b410a2ce876bd74808d2d0888a82667669f8e783a898bf1" +dependencies = [ + "derive_arbitrary", +] + [[package]] name = "arboard" version = "3.6.1" @@ -1407,6 +1416,17 @@ dependencies = [ "syn 1.0.109", ] +[[package]] +name = "derive_arbitrary" +version = "1.4.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "1e567bd82dcff979e4b03460c307b3cdc9e96fde3d73bed1496d2bc75d9dd62a" +dependencies = [ + "proc-macro2", + "quote", + "syn 2.0.117", +] + [[package]] name = "derive_more" version = "1.0.0" @@ -1922,6 +1942,17 @@ version = "0.2.9" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "28dea519a9695b9977216879a3ebfddf92f1c08c05d984f8996aecd6ecdc811d" +[[package]] +name = "filetime" +version = "0.2.27" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "f98844151eee8917efc50bd9e8318cb963ae8b297431495d3f758616ea5c57db" +dependencies = [ + "cfg-if", + "libc", + "libredox", +] + [[package]] name = "find-msvc-tools" version = "0.1.9" @@ -3186,7 +3217,10 @@ version = "0.1.16" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "e02f3bb43d335493c96bf3fd3a321600bf6bd07ed34bc64118e9293bdffea46c" dependencies = [ + "bitflags 2.11.1", "libc", + "plain", + "redox_syscall 0.7.4", ] [[package]] @@ -3273,6 +3307,17 @@ version = "0.1.2" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "112b39cec0b298b6c1999fee3e31427f74f676e4cb9879ed1a121b43661a4154" +[[package]] +name = "lzma-sys" +version = "0.1.20" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "5fda04ab3764e6cde78b9974eec4f779acaba7c4e84b36eca3cf77c581b85d27" +dependencies = [ + "cc", + "libc", + "pkg-config", +] + [[package]] name = "mac" version = "0.1.1" @@ -4438,10 +4483,12 @@ dependencies = [ "serde", "serde-big-array", "serde_json", + "serde_yaml", "sha2 0.10.9", "shellexpand", "socketioxide", "sysinfo", + "tar", "tempfile", "thiserror 2.0.18", "tokio", @@ -4465,8 +4512,11 @@ dependencies = [ "wa-rs-proto", "wa-rs-tokio-transport", "wa-rs-ureq-http", + "wait-timeout", "webpki-roots 1.0.6", "whisper-rs", + "xz2", + "zip", ] [[package]] @@ -4635,7 +4685,7 @@ checksum = "2621685985a2ebf1c516881c026032ac7deafcda1a2c9b7850dc81e3dfcb64c1" dependencies = [ "cfg-if", "libc", - "redox_syscall", + "redox_syscall 0.5.18", "smallvec", "windows-link", ] @@ -4838,6 +4888,12 @@ version = "0.3.33" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "19f132c84eca552bf34cab8ec81f1c1dcc229b811638f9d283dceabe58c5569e" +[[package]] +name = "plain" +version = "0.2.3" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "b4596b6d070b27117e987119b4dac604f3c58cfb0b191112e24771b2faeac1a6" + [[package]] name = "png" version = "0.18.1" @@ -5374,6 +5430,15 @@ dependencies = [ "bitflags 2.11.1", ] +[[package]] +name = "redox_syscall" +version = "0.7.4" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "f450ad9c3b1da563fb6948a8e0fb0fb9269711c9c73d9ea1de5058c79c8d643a" +dependencies = [ + "bitflags 2.11.1", +] + [[package]] name = "redox_users" version = "0.4.6" @@ -6208,6 +6273,19 @@ dependencies = [ "serde", ] +[[package]] +name = "serde_yaml" +version = "0.9.34+deprecated" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "6a8b1a1a2ebf674015cc02edccce75287f1a0130d394307b36743c2f5d504b47" +dependencies = [ + "indexmap", + "itoa", + "ryu", + "serde", + "unsafe-libyaml", +] + [[package]] name = "sha1" version = "0.10.6" @@ -6520,6 +6598,17 @@ version = "0.2.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "7b2093cf4c8eb1e67749a6762251bc9cd836b6fc171623bd0a9d324d37af2417" +[[package]] +name = "tar" +version = "0.4.45" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "22692a6476a21fa75fdfc11d452fda482af402c008cdbaf3476414e122040973" +dependencies = [ + "filetime", + "libc", + "xattr", +] + [[package]] name = "tempfile" version = "3.27.0" @@ -7215,6 +7304,12 @@ dependencies = [ "subtle", ] +[[package]] +name = "unsafe-libyaml" +version = "0.2.11" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "673aac59facbab8a9007c7f6108d11f63b603f7cabff99fabf650fea5c32b861" + [[package]] name = "untrusted" version = "0.9.0" @@ -7572,6 +7667,15 @@ dependencies = [ "wa-rs-core", ] +[[package]] +name = "wait-timeout" +version = "0.2.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "09ac3b126d3914f9849036f826e054cbabdc8519970b8998ddaf3b5bd3c65f11" +dependencies = [ + "libc", +] + [[package]] name = "walkdir" version = "2.5.0" @@ -8548,6 +8652,16 @@ dependencies = [ "zeroize", ] +[[package]] +name = "xattr" +version = "1.6.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "32e45ad4206f6d2479085147f02bc2ef834ac85886624a23575ae137c8aa8156" +dependencies = [ + "libc", + "rustix", +] + [[package]] name = "xkbcommon" version = "0.8.0" @@ -8582,6 +8696,15 @@ version = "0.8.15" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "fdd20c5420375476fbd4394763288da7eb0cc0b8c11deed431a91562af7335d3" +[[package]] +name = "xz2" +version = "0.1.7" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "388c44dc09d76f1536602ead6d325eb532f5c122f17782bd57fb47baeeb767e2" +dependencies = [ + "lzma-sys", +] + [[package]] name = "yoke" version = "0.7.5" @@ -8745,6 +8868,23 @@ dependencies = [ "syn 2.0.117", ] +[[package]] +name = "zip" +version = "2.4.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "fabe6324e908f85a1c52063ce7aa26b68dcb7eb6dbc83a2d148403c9bc3eba50" +dependencies = [ + "arbitrary", + "crc32fast", + "crossbeam-utils", + "displaydoc", + "flate2", + "indexmap", + "memchr", + "thiserror 2.0.18", + "zopfli", +] + [[package]] name = "zlib-rs" version = "0.6.3" @@ -8757,6 +8897,18 @@ version = "1.0.21" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "b8848ee67ecc8aedbaf3e4122217aff892639231befc6a1b58d29fff4c2cabaa" +[[package]] +name = "zopfli" +version = "0.8.3" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "f05cd8797d63865425ff89b5c4a48804f35ba0ce8d125800027ad6017d2b5249" +dependencies = [ + "bumpalo", + "crc32fast", + "log", + "simd-adler32", +] + [[package]] name = "zune-core" version = "0.5.1" diff --git a/Cargo.toml b/Cargo.toml index a7589b574..1ed3883da 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -16,6 +16,7 @@ crate-type = ["rlib"] [dependencies] serde = { version = "1", features = ["derive"] } serde_json = "1" +serde_yaml = "0.9" # Used by composio post-processing to convert Gmail HTML bodies to # markdown. `html2md` is the canonical pure-Rust crate (no system deps); # `htmd` (>0.5) is more actively maintained but our usage is one @@ -37,6 +38,15 @@ rand = "0.9" dirs = "5" sha2 = "0.10" hmac = "0.12" +# Archive extraction for the Node.js runtime bootstrap. Unix Node +# distributions ship as .tar.xz, Windows as .zip. `xz2` with `static` +# bundles liblzma so we don't need it as a system dependency. +tar = "0.4" +xz2 = { version = "0.1", features = ["static"] } +zip = { version = "2", default-features = false, features = ["deflate"] } +# Real timeout for `node --version` probes in the runtime resolver. Guards +# against a broken shim on PATH hanging the bootstrap forever. +wait-timeout = "0.2" uuid = { version = "1", features = ["v4"] } anyhow = "1.0" async-trait = "0.1" diff --git a/src/openhuman/agent/agents/code_executor/agent.toml b/src/openhuman/agent/agents/code_executor/agent.toml index 75d173d83..69405214b 100644 --- a/src/openhuman/agent/agents/code_executor/agent.toml +++ b/src/openhuman/agent/agents/code_executor/agent.toml @@ -14,4 +14,4 @@ omit_skills_catalog = true hint = "coding" [tools] -named = ["shell", "file_read", "file_write", "git_operations"] +named = ["shell", "file_read", "file_write", "git_operations", "node_exec", "npm_exec"] diff --git a/src/openhuman/agent/agents/tool_maker/agent.toml b/src/openhuman/agent/agents/tool_maker/agent.toml index 7cdacc4f8..f69587611 100644 --- a/src/openhuman/agent/agents/tool_maker/agent.toml +++ b/src/openhuman/agent/agents/tool_maker/agent.toml @@ -13,4 +13,4 @@ omit_skills_catalog = true hint = "coding" [tools] -named = ["file_write", "shell"] +named = ["file_write", "shell", "node_exec", "npm_exec"] diff --git a/src/openhuman/channels/tests/prompt.rs b/src/openhuman/channels/tests/prompt.rs index c1ece7a85..aa58164ef 100644 --- a/src/openhuman/channels/tests/prompt.rs +++ b/src/openhuman/channels/tests/prompt.rs @@ -173,6 +173,7 @@ fn prompt_skills_compact_list() { tools: vec![], prompts: vec!["Long prompt content that should NOT appear in system prompt".into()], location: None, + ..Default::default() }]; let prompt = build_system_prompt(ws.path(), "model", &[], &skills, None, Some("Discord")); diff --git a/src/openhuman/config/schema/load.rs b/src/openhuman/config/schema/load.rs index 87505ae96..5ab8273b0 100644 --- a/src/openhuman/config/schema/load.rs +++ b/src/openhuman/config/schema/load.rs @@ -21,6 +21,25 @@ fn default_config_and_workspace_dirs() -> Result<(PathBuf, PathBuf)> { Ok((config_dir.clone(), config_dir.join("workspace"))) } +/// Parse a boolean env-var value. Accepts the usual truthy/falsy tokens +/// (`1/true/yes/on` and `0/false/no/off`, case-insensitive). Returns `None` +/// on unrecognised values and logs a warning so silent mis-spellings don't +/// invisibly leave the config unchanged. +fn parse_env_bool(name: &str, raw: &str) -> Option { + 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; expected 1/true/yes/on or 0/false/no/off" + ); + None + } + } +} + const ACTIVE_WORKSPACE_STATE_FILE: &str = "active_workspace.toml"; static WARNED_WORLD_READABLE_CONFIGS: OnceLock>> = OnceLock::new(); @@ -793,6 +812,30 @@ impl Config { } } + // Node runtime overrides + if let Ok(flag) = std::env::var("OPENHUMAN_NODE_ENABLED") { + if let Some(enabled) = parse_env_bool("OPENHUMAN_NODE_ENABLED", &flag) { + self.node.enabled = enabled; + } + } + if let Ok(version) = std::env::var("OPENHUMAN_NODE_VERSION") { + let trimmed = version.trim(); + if !trimmed.is_empty() { + self.node.version = trimmed.to_string(); + } + } + if let Ok(dir) = std::env::var("OPENHUMAN_NODE_CACHE_DIR") { + let trimmed = dir.trim(); + if !trimmed.is_empty() { + self.node.cache_dir = trimmed.to_string(); + } + } + if let Ok(flag) = std::env::var("OPENHUMAN_NODE_PREFER_SYSTEM") { + if let Some(prefer_system) = parse_env_bool("OPENHUMAN_NODE_PREFER_SYSTEM", &flag) { + self.node.prefer_system = prefer_system; + } + } + let dsn_value = std::env::var("OPENHUMAN_SENTRY_DSN") .ok() .or_else(|| option_env!("OPENHUMAN_SENTRY_DSN").map(|s| s.to_string())); diff --git a/src/openhuman/config/schema/mod.rs b/src/openhuman/config/schema/mod.rs index 66610f133..6377f04d9 100644 --- a/src/openhuman/config/schema/mod.rs +++ b/src/openhuman/config/schema/mod.rs @@ -19,6 +19,7 @@ pub use load::{ user_openhuman_dir, write_active_user_id, PRE_LOGIN_USER_ID, }; mod local_ai; +mod node; mod observability; mod orchestrator; mod proxy; @@ -44,6 +45,7 @@ pub use heartbeat_cron::{CronConfig, HeartbeatConfig}; pub use identity_cost::{CostConfig, ModelPricing, PeripheralBoardConfig, PeripheralsConfig}; pub use learning::{LearningConfig, ReflectionSource}; pub use local_ai::LocalAiConfig; +pub use node::NodeConfig; pub use observability::ObservabilityConfig; pub use orchestrator::OrchestratorConfig; pub use proxy::{ diff --git a/src/openhuman/config/schema/node.rs b/src/openhuman/config/schema/node.rs new file mode 100644 index 000000000..9e1a2d52a --- /dev/null +++ b/src/openhuman/config/schema/node.rs @@ -0,0 +1,52 @@ +//! Node.js managed runtime configuration. +//! +//! Controls whether the core bootstraps a Node.js toolchain for skills that +//! require `node`/`npm` (e.g. agentskills.io packages with build steps). + +use schemars::JsonSchema; +use serde::{Deserialize, Serialize}; + +#[derive(Debug, Clone, Serialize, Deserialize, JsonSchema)] +pub struct NodeConfig { + /// Master switch. When `false`, the Node runtime is not resolved and + /// `node_exec` / `npm_exec` tools are not registered. + #[serde(default = "default_enabled")] + pub enabled: bool, + /// Target Node.js release line (used to build download URLs and bin cache + /// directory name, e.g. `v22.11.0`). Pin to a known LTS for reproducibility. + #[serde(default = "default_version")] + pub version: String, + /// Absolute path to a directory where managed Node distributions are + /// extracted. Empty string means "use the default workspace cache dir" + /// (resolved by the runtime bootstrap). + #[serde(default)] + pub cache_dir: String, + /// When `true` and a system `node` binary is found on `PATH` whose major + /// version matches `version`, reuse it instead of downloading. Disable for + /// reproducible CI / airgapped deployments. + #[serde(default = "default_prefer_system")] + pub prefer_system: bool, +} + +fn default_enabled() -> bool { + true +} + +fn default_version() -> String { + "v22.11.0".to_string() +} + +fn default_prefer_system() -> bool { + true +} + +impl Default for NodeConfig { + fn default() -> Self { + Self { + enabled: default_enabled(), + version: default_version(), + cache_dir: String::new(), + prefer_system: default_prefer_system(), + } + } +} diff --git a/src/openhuman/config/schema/types.rs b/src/openhuman/config/schema/types.rs index 4a078ca6c..ec466e1a1 100644 --- a/src/openhuman/config/schema/types.rs +++ b/src/openhuman/config/schema/types.rs @@ -124,6 +124,10 @@ pub struct Config { #[serde(default)] pub local_ai: LocalAiConfig, + /// Node.js managed runtime configuration (skills that need `node`/`npm`). + #[serde(default)] + pub node: NodeConfig, + #[serde(default)] pub voice_server: VoiceServerConfig, @@ -247,6 +251,7 @@ impl Default for Config { peripherals: PeripheralsConfig::default(), agents: HashMap::new(), local_ai: LocalAiConfig::default(), + node: NodeConfig::default(), voice_server: VoiceServerConfig::default(), query_classification: QueryClassificationConfig::default(), integrations: IntegrationsConfig::default(), diff --git a/src/openhuman/mod.rs b/src/openhuman/mod.rs index bd99a4407..77ee3d093 100644 --- a/src/openhuman/mod.rs +++ b/src/openhuman/mod.rs @@ -39,6 +39,7 @@ pub mod learning; pub mod local_ai; pub mod memory; pub mod migration; +pub mod node_runtime; pub mod overlay; pub mod providers; pub mod referral; diff --git a/src/openhuman/node_runtime/bootstrap.rs b/src/openhuman/node_runtime/bootstrap.rs new file mode 100644 index 000000000..df509e8dd --- /dev/null +++ b/src/openhuman/node_runtime/bootstrap.rs @@ -0,0 +1,361 @@ +//! Node.js bootstrap orchestrator. +//! +//! Ties the [`resolver`](super::resolver), [`downloader`](super::downloader), +//! and [`extractor`](super::extractor) modules into a single idempotent +//! entry point that callers use at startup (or lazily before the first +//! `node_exec` / `npm_exec` call): +//! +//! ```text +//! NodeBootstrap::new(config) -> resolve() -> ResolvedNode { node_bin, npm_bin, .. } +//! ``` +//! +//! The bootstrap is **serialised** through a `tokio::sync::Mutex` so that +//! concurrent callers never race on the download/extract/install pipeline. +//! Once a resolution succeeds the result is memoised — subsequent calls +//! return the cached `ResolvedNode` in O(1). + +use anyhow::{bail, Context, Result}; +use reqwest::Client; +use std::path::{Path, PathBuf}; +use std::sync::Arc; +use tokio::sync::Mutex; + +use super::downloader::{download_distribution, fetch_shasums, NodeDistribution}; +use super::extractor::{atomic_install, extract_distribution}; +use super::resolver::{detect_system_node, SystemNode}; +use crate::openhuman::config::schema::NodeConfig; + +/// Origin of the resolved toolchain — feeds into logging and lets the +/// caller decide whether to expose a "Node was downloaded to …" message in +/// the UI. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum NodeSource { + /// Reused a compatible `node` already on the host `PATH`. + System, + /// Downloaded + extracted a managed distribution. + Managed, +} + +/// Fully-resolved Node.js toolchain. Callers should only cache this via the +/// [`NodeBootstrap`] — constructing one by hand bypasses version pinning. +#[derive(Debug, Clone)] +pub struct ResolvedNode { + /// Directory that should be prepended to `PATH` for child processes so + /// `node`, `npm`, `npx`, `corepack` resolve to the managed binaries. + pub bin_dir: PathBuf, + /// Absolute path to the `node` binary. + pub node_bin: PathBuf, + /// Absolute path to the `npm` launcher (shell script on Unix, `.cmd` + /// shim on Windows). Symlinks on Unix distributions point at a JS file + /// in `lib/` — invoking through the launcher is the supported contract. + pub npm_bin: PathBuf, + /// Version string without the leading `v` (e.g. `"22.11.0"`). + pub version: String, + /// Where the toolchain came from. + pub source: NodeSource, +} + +/// Serialised bootstrap entrypoint. Hold one per process (e.g. behind a +/// `OnceCell`) — the internal mutex is what makes concurrent `resolve()` +/// calls safe. +pub struct NodeBootstrap { + config: NodeConfig, + workspace_dir: PathBuf, + client: Client, + cached: Arc>>, +} + +impl NodeBootstrap { + /// Build a new bootstrap. `workspace_dir` is used to derive the default + /// cache location when `config.cache_dir` is empty. + pub fn new(config: NodeConfig, workspace_dir: PathBuf, client: Client) -> Self { + Self { + config, + workspace_dir, + client, + cached: Arc::new(Mutex::new(None)), + } + } + + /// Peek at the memoised [`ResolvedNode`] without triggering a download. + /// + /// Returns `Some(..)` only when a previous `resolve()` call succeeded + /// and the cache lock is currently free. Returns `None` otherwise — + /// e.g. no resolution has happened yet, or another task holds the + /// lock doing the initial install. Callers use this for transparent + /// PATH injection (shell tool) where a blocking wait or a forced + /// download would change the semantics of unrelated commands. + pub fn try_cached(&self) -> Option { + self.cached.try_lock().ok().and_then(|g| g.clone()) + } + + /// Resolve the Node.js toolchain, downloading + extracting a managed + /// distribution if necessary. Idempotent: the first successful call + /// memoises the result; later calls return it without further I/O. + pub async fn resolve(&self) -> Result { + let mut guard = self.cached.lock().await; + if let Some(existing) = guard.as_ref() { + tracing::debug!( + version = %existing.version, + source = ?existing.source, + "[node_runtime::bootstrap] returning cached ResolvedNode" + ); + return Ok(existing.clone()); + } + + if !self.config.enabled { + bail!("node runtime is disabled (set node.enabled = true to use skills that require node/npm)"); + } + + if self.config.prefer_system { + if let Some(system) = detect_system_node(&self.config.version) { + let resolved = resolve_from_system(system)?; + *guard = Some(resolved.clone()); + return Ok(resolved); + } + } + + let managed = self.install_managed().await?; + *guard = Some(managed.clone()); + Ok(managed) + } + + /// Compute the cache root for managed Node.js installs. + /// + /// Resolution order (first hit wins): + /// 1. Explicit `config.cache_dir` — an operator/user opted into a specific + /// location and we honour it verbatim (including workspace-local paths + /// if they set one). + /// 2. OS user cache (`dirs::cache_dir()/openhuman/node-runtime`) — the + /// default. Lives in the user's home and cannot be spoofed by a + /// repository checked-in `./node-runtime/` tree. + /// 3. Last-resort `{workspace}/node-runtime/` fallback, emitted with a + /// warning for platforms where `dirs::cache_dir()` returns `None`. + /// + /// Note: returning a workspace-local path by default would let a malicious + /// repository vendor a fake `node-v*/` tree into the workspace and have + /// [`probe_managed_install`] reuse it as a trusted managed runtime (see + /// CodeRabbit finding on PR #723). Guarding that path in the probe is the + /// second defence; picking a user-owned default here is the first. + fn cache_root(&self) -> PathBuf { + let configured = self.config.cache_dir.trim(); + if !configured.is_empty() { + return PathBuf::from(configured); + } + if let Some(user_cache) = dirs::cache_dir() { + return user_cache.join("openhuman").join("node-runtime"); + } + tracing::warn!( + workspace = %self.workspace_dir.display(), + "[node_runtime::bootstrap] dirs::cache_dir() unavailable; falling back to workspace-local node-runtime (less secure — set config.cache_dir to a user-owned path)" + ); + self.workspace_dir.join("node-runtime") + } + + /// Full install path for the managed distribution. Matches the + /// archive's top-level folder name so `find_single_top_level` picks the + /// same directory when re-validating an existing install. + fn install_dir(&self, dist: &NodeDistribution) -> PathBuf { + // `archive_name` is e.g. `node-v22.11.0-darwin-arm64.tar.xz`. + // Strip the extension(s) to get the install folder name. + let stem = dist + .archive_name + .trim_end_matches(".zip") + .trim_end_matches(".tar.xz") + .trim_end_matches(".tar") + .to_string(); + self.cache_root().join(stem) + } + + /// Full managed-install flow: + /// 1. Shortcut if an extracted install already exists and has valid + /// `node`/`npm` binaries. + /// 2. Otherwise fetch `SHASUMS256.txt`, pick the matching digest, + /// download the archive, extract it, and atomically install. + async fn install_managed(&self) -> Result { + let dist = NodeDistribution::for_host(&self.config.version)?; + let install_dir = self.install_dir(&dist); + + let cache_root = self.cache_root(); + if let Some(resolved) = + probe_managed_install(&install_dir, &cache_root, &self.config.version) + { + tracing::info!( + install_dir = %install_dir.display(), + "[node_runtime::bootstrap] reusing existing managed install" + ); + return Ok(resolved); + } + + tracing::info!( + version = %dist.version, + install_dir = %install_dir.display(), + "[node_runtime::bootstrap] installing managed node" + ); + + let shasums = fetch_shasums(&self.client, &self.config.version).await?; + let expected = shasums + .get(&dist.archive_name) + .cloned() + .with_context(|| format!("SHASUMS256.txt missing entry for {}", dist.archive_name))?; + + let cache_root = self.cache_root(); + tokio::fs::create_dir_all(&cache_root) + .await + .with_context(|| format!("creating cache root {}", cache_root.display()))?; + let archive_path = cache_root.join(&dist.archive_name); + download_distribution(&self.client, &dist, &archive_path, &expected).await?; + + // Extract into a scratch folder so a partial extraction never + // contaminates the cache root; `atomic_install` promotes the + // inner top-level folder into the final install path. + let scratch = cache_root.join(format!(".stage-{}", std::process::id())); + // Wipe any leftover from a previous crashed run. + let _ = tokio::fs::remove_dir_all(&scratch).await; + let top_level = extract_distribution(&archive_path, &scratch, dist.is_zip).await?; + atomic_install(&top_level, &install_dir).await?; + let _ = tokio::fs::remove_dir_all(&scratch).await; + let _ = tokio::fs::remove_file(&archive_path).await; + + 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) + } +} + +/// Host-specific bin layout. +/// +/// * macOS/Linux: `/bin/{node,npm}` +/// * Windows: `/{node.exe,npm.cmd}` (no `bin/` subdir in the +/// official zip distributions) +fn managed_bin_dir(install_dir: &Path) -> PathBuf { + if cfg!(windows) { + install_dir.to_path_buf() + } else { + install_dir.join("bin") + } +} + +/// Build a [`ResolvedNode`] from a bin directory by filling in the +/// platform-specific executable names. +fn build_resolved(bin_dir: PathBuf, version: String, source: NodeSource) -> Result { + let (node_name, npm_name) = if cfg!(windows) { + ("node.exe", "npm.cmd") + } else { + ("node", "npm") + }; + let node_bin = bin_dir.join(node_name); + let npm_bin = bin_dir.join(npm_name); + if !node_bin.is_file() { + bail!( + "resolved node bin missing: {} — install appears corrupted", + node_bin.display() + ); + } + if !npm_bin.exists() { + tracing::warn!( + npm_bin = %npm_bin.display(), + "[node_runtime::bootstrap] npm launcher missing; npm_exec tool will fail until reinstall" + ); + } + Ok(ResolvedNode { + bin_dir, + node_bin, + npm_bin, + version, + source, + }) +} + +/// Wrap a detected system node in a [`ResolvedNode`]. +/// +/// `detect_system_node` already strips the leading `v` from the probed +/// version, but we re-normalise here so the `ResolvedNode::version` +/// contract (no leading `v`) cannot be violated by any future code path +/// that constructs a `SystemNode` differently. +fn resolve_from_system(system: SystemNode) -> Result { + let bin_dir = system + .path + .parent() + .map(Path::to_path_buf) + .unwrap_or_default(); + let version = system + .version + .trim_start_matches(|c: char| c == 'v' || c == 'V') + .trim() + .to_string(); + build_resolved(bin_dir, version, NodeSource::System) +} + +/// Check whether `install_dir` already contains a usable managed install +/// for `target_version`. Cheap enough to run on every `resolve()` because +/// it never touches the network — just a few `stat()` calls. +/// +/// Also guards against **cache-root escape**: callers derive `install_dir` +/// from `cache_root` via [`NodeBootstrap::install_dir`], but a symlinked or +/// out-of-tree `install_dir` (e.g. a committed workspace `./node-runtime/` +/// tree when `cache_root` resolves to the user cache) must not be treated +/// as a trusted install. We canonicalise both paths and require the install +/// to live under the cache root; mismatches force a fresh, verified +/// download via `install_managed()`. +/// +/// A managed install is only "usable" when both `node` and `npm` launchers +/// are present. `build_resolved` only hard-fails on missing `node`, so we +/// re-check `npm_bin` here and return `None` on absence — forcing a fresh +/// download via the normal resolve path. Without this, a corrupted cache +/// (e.g. download interrupted after node was extracted but before npm) +/// would be reused forever and `npm_exec` could never self-heal. +fn probe_managed_install( + install_dir: &Path, + cache_root: &Path, + target_version: &str, +) -> Option { + if !install_dir.is_dir() { + return None; + } + // Canonicalise both sides so a symlink inside the install can't smuggle + // a repo-controlled tree past the `starts_with` check. `cache_root` must + // exist because the caller created `install_dir` under it, but be + // defensive: treat a failed canonicalize as "not trustworthy". + let canon_install = match std::fs::canonicalize(install_dir) { + Ok(p) => p, + Err(err) => { + tracing::warn!( + install_dir = %install_dir.display(), + error = %err, + "[node_runtime::bootstrap] canonicalize(install_dir) failed; treating as unusable" + ); + return None; + } + }; + let canon_cache = match std::fs::canonicalize(cache_root) { + Ok(p) => p, + Err(err) => { + tracing::warn!( + cache_root = %cache_root.display(), + error = %err, + "[node_runtime::bootstrap] canonicalize(cache_root) failed; treating managed install as unusable" + ); + return None; + } + }; + if !canon_install.starts_with(&canon_cache) { + tracing::warn!( + install_dir = %canon_install.display(), + cache_root = %canon_cache.display(), + "[node_runtime::bootstrap] refusing to reuse managed install outside the resolved cache root (possible spoof)" + ); + return None; + } + let bin_dir = managed_bin_dir(install_dir); + let version = target_version.trim_start_matches('v').to_string(); + let resolved = build_resolved(bin_dir, version, NodeSource::Managed).ok()?; + if !resolved.npm_bin.is_file() { + tracing::warn!( + npm_bin = %resolved.npm_bin.display(), + "[node_runtime::bootstrap] managed install missing npm; forcing reinstall" + ); + return None; + } + Some(resolved) +} diff --git a/src/openhuman/node_runtime/downloader.rs b/src/openhuman/node_runtime/downloader.rs new file mode 100644 index 000000000..9ccd24ad7 --- /dev/null +++ b/src/openhuman/node_runtime/downloader.rs @@ -0,0 +1,280 @@ +//! Node.js distribution downloader with SHASUMS256 verification. +//! +//! Resolves the right archive for the current OS/arch off nodejs.org, +//! streams it to a caller-supplied temp path, and validates the SHA-256 +//! against the official `SHASUMS256.txt` for the release. Keeps everything +//! in one place so the bootstrap caller only needs to know "download this +//! version, give me the bytes on disk". +//! +//! ## Security +//! +//! We **require** a SHA-256 match before returning success — a corrupted or +//! tampered archive is treated the same as a failed download and the file +//! is deleted. There is no opt-out; skills will run untrusted code inside +//! the resolved Node runtime, so the integrity check is load-bearing. + +use anyhow::{anyhow, bail, Context, Result}; +use reqwest::Client; +use sha2::{Digest, Sha256}; +use std::collections::HashMap; +use std::path::Path; +use tokio::fs::File; +use tokio::io::AsyncWriteExt; + +/// Base URL for official Node.js release artifacts. +const NODEJS_DIST_BASE: &str = "https://nodejs.org/dist"; + +/// Describes a single downloadable Node.js distribution for the host triple. +#[derive(Debug, Clone)] +pub struct NodeDistribution { + /// Version string including the leading `v` (e.g. `v22.11.0`). + pub version: String, + /// Archive filename as it appears in `SHASUMS256.txt` + /// (e.g. `node-v22.11.0-darwin-arm64.tar.xz`). + pub archive_name: String, + /// Full download URL. + pub url: String, + /// Whether the archive is a zip (Windows) or tar.xz (everything else). + /// Drives which extraction path the caller invokes. + pub is_zip: bool, +} + +impl NodeDistribution { + /// Build the distribution descriptor for the current host OS/arch. + /// + /// Supported triples mirror the officially-prebuilt Node.js binaries: + /// + /// | OS | Arch | Archive suffix | + /// |----------|--------------------------------------|-----------------------------| + /// | macOS | aarch64, x86_64 | `-darwin-{arm64,x64}.tar.xz`| + /// | Linux | aarch64, x86_64, arm, armv7 | `-linux-{arm64,x64,armv7l}.tar.xz` | + /// | Windows | aarch64, x86_64 | `-win-{arm64,x64}.zip` | + /// + /// Everything else yields an error — the caller should surface it as a + /// "Node runtime unavailable on this host" message. + pub fn for_host(version: &str) -> Result { + let version = normalize_version(version); + let (suffix, is_zip) = host_archive_suffix()?; + let archive_name = format!("node-{version}-{suffix}"); + let url = format!("{NODEJS_DIST_BASE}/{version}/{archive_name}"); + tracing::debug!( + version = %version, + url = %url, + "[node_runtime::downloader] resolved distribution for host" + ); + Ok(Self { + version, + archive_name, + url, + is_zip, + }) + } +} + +/// Normalise a version string to the canonical `vX.Y.Z` form used by +/// nodejs.org. Config allows `22.11.0` or `v22.11.0`; we always emit the +/// `v`-prefixed variant because it is what appears in the URL path. +fn normalize_version(raw: &str) -> String { + let trimmed = raw.trim(); + if trimmed.starts_with('v') { + trimmed.to_string() + } else { + format!("v{trimmed}") + } +} + +/// Return `(archive_suffix, is_zip)` for the current host. The suffix omits +/// the `node-vX.Y.Z-` prefix because callers always interpolate the version. +fn host_archive_suffix() -> Result<(&'static str, bool)> { + let os = std::env::consts::OS; + let arch = std::env::consts::ARCH; + match (os, arch) { + ("macos", "aarch64") => Ok(("darwin-arm64.tar.xz", false)), + ("macos", "x86_64") => Ok(("darwin-x64.tar.xz", false)), + ("linux", "aarch64") => Ok(("linux-arm64.tar.xz", false)), + ("linux", "x86_64") => Ok(("linux-x64.tar.xz", false)), + ("linux", "arm") | ("linux", "armv7") => Ok(("linux-armv7l.tar.xz", false)), + ("windows", "aarch64") => Ok(("win-arm64.zip", true)), + ("windows", "x86_64") => Ok(("win-x64.zip", true)), + _ => Err(anyhow!( + "no prebuilt Node.js distribution for host {os}/{arch} — set node.enabled=false or install node manually" + )), + } +} + +/// Fetch `SHASUMS256.txt` for the release and return a +/// `archive_name -> sha256_hex` map. The hex digest is lowercase. +pub async fn fetch_shasums(client: &Client, version: &str) -> Result> { + let version = normalize_version(version); + let url = format!("{NODEJS_DIST_BASE}/{version}/SHASUMS256.txt"); + tracing::debug!(url = %url, "[node_runtime::downloader] fetching SHASUMS256.txt"); + + let body = client + .get(&url) + .send() + .await + .with_context(|| format!("GET {url}"))? + .error_for_status() + .with_context(|| format!("non-success status on {url}"))? + .text() + .await + .with_context(|| format!("reading body of {url}"))?; + + let map = parse_shasums(&body); + tracing::debug!( + entries = map.len(), + "[node_runtime::downloader] parsed SHASUMS256.txt" + ); + Ok(map) +} + +/// Parse the `SHASUMS256.txt` body into a lookup table. The format is one +/// entry per line: ` ` (two spaces). Unknown / blank +/// lines are skipped to be robust against trailing newlines or signature +/// blocks that may appear in future releases. +fn parse_shasums(body: &str) -> HashMap { + let mut out = HashMap::new(); + for line in body.lines() { + let mut parts = line.split_whitespace(); + let (Some(hash), Some(name)) = (parts.next(), parts.next()) else { + continue; + }; + if hash.len() == 64 && hash.chars().all(|c| c.is_ascii_hexdigit()) { + out.insert(name.to_string(), hash.to_ascii_lowercase()); + } + } + out +} + +/// Stream `dist.url` to `target_path`, computing the SHA-256 on the fly and +/// comparing against the digest supplied in `expected_sha256`. +/// +/// On mismatch or any I/O error the partial file at `target_path` is +/// removed — we never leave half-written / tampered archives on disk. +pub async fn download_distribution( + client: &Client, + dist: &NodeDistribution, + target_path: &Path, + expected_sha256: &str, +) -> Result<()> { + tracing::info!( + url = %dist.url, + target = %target_path.display(), + "[node_runtime::downloader] starting download" + ); + + if let Some(parent) = target_path.parent() { + tokio::fs::create_dir_all(parent) + .await + .with_context(|| format!("creating cache dir {}", parent.display()))?; + } + + let mut response = client + .get(&dist.url) + .send() + .await + .with_context(|| format!("GET {}", dist.url))? + .error_for_status() + .with_context(|| format!("non-success status on {}", dist.url))?; + + let total_bytes = response.content_length(); + let mut file = File::create(target_path) + .await + .with_context(|| format!("creating {}", target_path.display()))?; + let mut hasher = Sha256::new(); + let mut written: u64 = 0; + + // Stream into `file`. On any chunk / write / flush failure we remove + // the partial file on disk so a retry starts clean and callers never + // see a half-written archive. + let stream_result: Result<()> = async { + while let Some(chunk) = response + .chunk() + .await + .with_context(|| format!("streaming {}", dist.url))? + { + hasher.update(&chunk); + file.write_all(&chunk) + .await + .with_context(|| format!("writing chunk to {}", target_path.display()))?; + written = written.saturating_add(chunk.len() as u64); + } + file.flush() + .await + .with_context(|| format!("flushing {}", target_path.display()))?; + Ok(()) + } + .await; + + drop(file); + + if let Err(err) = stream_result { + tracing::warn!( + target = %target_path.display(), + error = %err, + "[node_runtime::downloader] streaming failed — removing partial archive" + ); + let _ = tokio::fs::remove_file(target_path).await; + return Err(err); + } + + let actual_hex = hex::encode(hasher.finalize()); + let expected = expected_sha256.trim().to_ascii_lowercase(); + + if actual_hex != expected { + tracing::error!( + expected = %expected, + actual = %actual_hex, + target = %target_path.display(), + "[node_runtime::downloader] SHA-256 mismatch — deleting partial archive" + ); + let _ = tokio::fs::remove_file(target_path).await; + bail!( + "SHA-256 mismatch for {} (expected {expected}, got {actual_hex})", + dist.archive_name + ); + } + + tracing::info!( + target = %target_path.display(), + bytes = written, + total = ?total_bytes, + "[node_runtime::downloader] download complete, hash verified" + ); + Ok(()) +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn normalizes_version_with_and_without_prefix() { + assert_eq!(normalize_version("22.11.0"), "v22.11.0"); + assert_eq!(normalize_version("v22.11.0"), "v22.11.0"); + assert_eq!(normalize_version(" v22.11.0\n"), "v22.11.0"); + } + + #[test] + fn parses_shasums_text() { + let body = "\ +abc123def4567890abc123def4567890abc123def4567890abc123def4567890 node-v22.11.0-darwin-arm64.tar.xz +1111222233334444555566667777888899990000111122223333444455556666 node-v22.11.0-linux-x64.tar.xz +garbage line +BADHASHNOTHEX node-v22.11.0-win-x64.zip +"; + let map = parse_shasums(body); + assert_eq!(map.len(), 2); + assert_eq!( + map.get("node-v22.11.0-darwin-arm64.tar.xz").unwrap(), + "abc123def4567890abc123def4567890abc123def4567890abc123def4567890" + ); + } + + #[test] + fn distribution_for_host_returns_sensible_url() { + let dist = NodeDistribution::for_host("v22.11.0").expect("host supported in CI"); + assert!(dist.url.starts_with("https://nodejs.org/dist/v22.11.0/")); + assert!(dist.archive_name.starts_with("node-v22.11.0-")); + } +} diff --git a/src/openhuman/node_runtime/extractor.rs b/src/openhuman/node_runtime/extractor.rs new file mode 100644 index 000000000..2fc46a3c7 --- /dev/null +++ b/src/openhuman/node_runtime/extractor.rs @@ -0,0 +1,235 @@ +//! Archive extraction for downloaded Node.js distributions. +//! +//! Handles both shapes that nodejs.org ships: +//! +//! * `.tar.xz` on macOS and Linux — decoded via `xz2` then unpacked through +//! the `tar` crate. +//! * `.zip` on Windows — unpacked through the `zip` crate. +//! +//! All archives are "single-rooted": they expand into one top-level folder +//! like `node-v22.11.0-darwin-arm64/`. We extract into a caller-supplied +//! staging directory, then return the absolute path of that inner folder so +//! the bootstrap layer can rename/move it into the cache atomically. +//! +//! Extraction is CPU/IO-bound and the underlying crates are synchronous, so +//! we wrap the real work in `tokio::task::spawn_blocking` to keep the +//! runtime responsive. + +use anyhow::{anyhow, Context, Result}; +use std::fs::{self, File}; +use std::io; +use std::path::{Path, PathBuf}; + +/// Extract `archive` into `extract_root` and return the absolute path of the +/// single top-level folder produced by the archive. +/// +/// `is_zip = true` selects the zip path, otherwise the tar.xz path runs. +/// On any error the caller should treat `extract_root` as contaminated and +/// remove it before retrying — we do not auto-clean because the caller +/// typically owns a fresh temp dir. +pub async fn extract_distribution( + archive: &Path, + extract_root: &Path, + is_zip: bool, +) -> Result { + let archive = archive.to_path_buf(); + let extract_root = extract_root.to_path_buf(); + + tracing::info!( + archive = %archive.display(), + extract_root = %extract_root.display(), + is_zip, + "[node_runtime::extractor] starting extraction" + ); + + tokio::task::spawn_blocking(move || -> Result { + fs::create_dir_all(&extract_root) + .with_context(|| format!("creating extract root {}", extract_root.display()))?; + + if is_zip { + extract_zip(&archive, &extract_root)?; + } else { + extract_tar_xz(&archive, &extract_root)?; + } + + let top_level = find_single_top_level(&extract_root)?; + tracing::info!( + top_level = %top_level.display(), + "[node_runtime::extractor] extraction complete" + ); + Ok(top_level) + }) + .await + .context("spawn_blocking join failure during extraction")? +} + +/// Extract a `.tar.xz` archive into `extract_root`. +fn extract_tar_xz(archive: &Path, extract_root: &Path) -> Result<()> { + let file = + File::open(archive).with_context(|| format!("opening archive {}", archive.display()))?; + let decoder = xz2::read::XzDecoder::new(file); + let mut tar = tar::Archive::new(decoder); + // `set_preserve_permissions(true)` is the default on Unix; we restate + // it so the `node` binary keeps its `+x` bit after extraction. + tar.set_preserve_permissions(true); + tar.set_overwrite(true); + tar.unpack(extract_root) + .with_context(|| format!("unpacking tar.xz into {}", extract_root.display()))?; + Ok(()) +} + +/// Extract a `.zip` archive into `extract_root`. Handles directory entries, +/// file entries, and restores Unix mode bits where present (no-op on +/// Windows hosts, which is where `.zip` actually matters). +fn extract_zip(archive: &Path, extract_root: &Path) -> Result<()> { + let file = + File::open(archive).with_context(|| format!("opening archive {}", archive.display()))?; + let mut zip = zip::ZipArchive::new(file) + .with_context(|| format!("opening zip archive {}", archive.display()))?; + + for i in 0..zip.len() { + let mut entry = zip + .by_index(i) + .with_context(|| format!("reading zip entry {i}"))?; + let Some(relative) = entry.enclosed_name() else { + tracing::warn!( + name = entry.name(), + "[node_runtime::extractor] skipping zip entry with unsafe path" + ); + continue; + }; + let out_path = extract_root.join(relative); + + if entry.is_dir() { + fs::create_dir_all(&out_path) + .with_context(|| format!("creating {}", out_path.display()))?; + } else { + if let Some(parent) = out_path.parent() { + fs::create_dir_all(parent) + .with_context(|| format!("creating {}", parent.display()))?; + } + let mut out = File::create(&out_path) + .with_context(|| format!("creating {}", out_path.display()))?; + io::copy(&mut entry, &mut out) + .with_context(|| format!("writing {}", out_path.display()))?; + } + + #[cfg(unix)] + if let Some(mode) = entry.unix_mode() { + use std::os::unix::fs::PermissionsExt; + fs::set_permissions(&out_path, fs::Permissions::from_mode(mode)) + .with_context(|| format!("chmod {}", out_path.display()))?; + } + } + + Ok(()) +} + +/// Locate the single top-level directory inside `extract_root`. Node.js +/// archives always produce one root folder; anything else (multiple +/// entries, only files) is a contract violation from our side and we +/// surface it as an error rather than guessing. +fn find_single_top_level(extract_root: &Path) -> Result { + let mut entries = fs::read_dir(extract_root) + .with_context(|| format!("listing {}", extract_root.display()))? + .collect::, _>>() + .with_context(|| format!("reading entries of {}", extract_root.display()))?; + + // Stable order for deterministic logging. + entries.sort_by_key(|e| e.file_name()); + + let mut dirs: Vec = entries + .into_iter() + .filter(|e| e.file_type().map(|t| t.is_dir()).unwrap_or(false)) + .map(|e| e.path()) + .collect(); + + match dirs.len() { + 1 => Ok(dirs.pop().unwrap()), + 0 => Err(anyhow!( + "expected one top-level folder under {}, found none", + extract_root.display() + )), + n => Err(anyhow!( + "expected one top-level folder under {}, found {n}: {:?}", + extract_root.display(), + dirs + )), + } +} + +/// Atomically move `staged` into place at `final_dest`. +/// +/// Strategy: +/// 1. If `final_dest` already exists, move it to a sibling `.old-` +/// path so we never lose a working install even if a later step fails. +/// 2. Rename `staged` -> `final_dest`. On the same filesystem this is a +/// single `rename(2)` and is atomic from the reader's perspective. +/// 3. Best-effort cleanup of the `.old-*` directory. +/// +/// Returns the `final_dest` path on success. +pub async fn atomic_install(staged: &Path, final_dest: &Path) -> Result { + let staged = staged.to_path_buf(); + let final_dest = final_dest.to_path_buf(); + + tokio::task::spawn_blocking(move || -> Result { + if let Some(parent) = final_dest.parent() { + fs::create_dir_all(parent) + .with_context(|| format!("creating parent {}", parent.display()))?; + } + + let mut backup: Option = None; + if final_dest.exists() { + let ts = std::process::id(); + let candidate = final_dest.with_extension(format!("old-{ts}")); + fs::rename(&final_dest, &candidate).with_context(|| { + format!( + "moving existing install {} aside to {}", + final_dest.display(), + candidate.display() + ) + })?; + backup = Some(candidate); + } + + if let Err(err) = fs::rename(&staged, &final_dest).with_context(|| { + format!( + "renaming staged {} -> {}", + staged.display(), + final_dest.display() + ) + }) { + // Stage->final rename failed; restore the previous install from + // backup so the working runtime stays in place. Surface any + // restore failure separately (as a warning) but always return + // the original error. + if let Some(backup_path) = backup.as_ref() { + if let Err(restore_err) = fs::rename(backup_path, &final_dest) { + tracing::warn!( + backup = %backup_path.display(), + final_dest = %final_dest.display(), + error = %restore_err, + "[node_runtime::extractor] failed to restore backup after staged rename failure" + ); + } else { + tracing::info!( + final_dest = %final_dest.display(), + "[node_runtime::extractor] restored previous install after staged rename failure" + ); + } + } + return Err(err); + } + + if let Some(path) = backup { + let _ = fs::remove_dir_all(&path); + } + tracing::info!( + final_dest = %final_dest.display(), + "[node_runtime::extractor] atomic install complete" + ); + Ok(final_dest) + }) + .await + .context("spawn_blocking join failure during atomic install")? +} diff --git a/src/openhuman/node_runtime/mod.rs b/src/openhuman/node_runtime/mod.rs new file mode 100644 index 000000000..365937dcc --- /dev/null +++ b/src/openhuman/node_runtime/mod.rs @@ -0,0 +1,21 @@ +//! Managed Node.js runtime for skills that require `node` / `npm`. +//! +//! Responsibilities are split across submodules: +//! +//! * [`resolver`] — detect a compatible system `node` on `PATH`. Cheap, +//! synchronous, called first so we can skip the download path when a +//! matching toolchain already exists on the host. +//! +//! Later commits layer on a downloader, archive extractor, cache manager, +//! and a bootstrap entry point that returns the resolved `node`/`npm` +//! binary paths for `node_exec` / `npm_exec` tools. + +pub mod bootstrap; +pub mod downloader; +pub mod extractor; +pub mod resolver; + +pub use bootstrap::{NodeBootstrap, NodeSource, ResolvedNode}; +pub use downloader::{download_distribution, fetch_shasums, NodeDistribution}; +pub use extractor::{atomic_install, extract_distribution}; +pub use resolver::{detect_system_node, parse_node_version, SystemNode}; diff --git a/src/openhuman/node_runtime/resolver.rs b/src/openhuman/node_runtime/resolver.rs new file mode 100644 index 000000000..19fe1bff4 --- /dev/null +++ b/src/openhuman/node_runtime/resolver.rs @@ -0,0 +1,290 @@ +//! System-node resolver. +//! +//! Walks `PATH`, probes `node --version`, and returns a [`SystemNode`] when +//! the host-installed binary matches the configured target major version. +//! Runs synchronously because it blocks on one short-lived subprocess and is +//! called exactly once per bootstrap — pushing it onto the Tokio runtime +//! would add noise without benefit. +//! +//! Target-version matching is intentionally loose: we only compare **major** +//! versions. Point releases of Node.js are ABI-stable, and skills pin their +//! own dependency versions via `package.json` / `package-lock.json`, so a +//! host `v22.8.0` is accepted when `node.version = "v22.11.0"`. If a user +//! needs strict pinning they can set `node.prefer_system = false`. + +use std::path::PathBuf; +use std::process::{Command, Stdio}; +use std::time::Duration; + +/// A usable Node.js toolchain discovered on the host `PATH`. +#[derive(Debug, Clone)] +pub struct SystemNode { + /// Absolute path to the `node` executable. + pub path: PathBuf, + /// Parsed major version (e.g. `22`). + pub major: u32, + /// Raw version string reported by `node --version`, trimmed of the + /// leading `v` and trailing whitespace (e.g. `"22.11.0"`). + pub version: String, +} + +/// Parse a version string like `v22.11.0` / `22.11.0` / `v22` and return the +/// numeric major component. +/// +/// Returns `None` when the input is malformed. Tolerant of surrounding +/// whitespace and an optional leading `v` prefix so it can accept both the +/// config value (`node.version = "v22.11.0"`) and the raw `node --version` +/// output (`v22.11.0\n`). +pub fn parse_node_version(raw: &str) -> Option { + let trimmed = raw.trim(); + let stripped = trimmed.strip_prefix('v').unwrap_or(trimmed); + let major = stripped.split('.').next()?; + major.parse::().ok() +} + +/// Probe the host for a `node` binary on `PATH` whose major version matches +/// `target_version`. Returns `Some(SystemNode)` on success, `None` when no +/// compatible toolchain is found. +/// +/// Heavy tracing is intentional — resolver decisions drive whether we skip a +/// multi-hundred-MB download, so operators need a clear breadcrumb trail. +pub fn detect_system_node(target_version: &str) -> Option { + let Some(target_major) = parse_node_version(target_version) else { + tracing::warn!( + target_version, + "[node_runtime::resolver] invalid target_version, skipping system-node probe" + ); + return None; + }; + + let Some(path) = which_node() else { + tracing::debug!( + "[node_runtime::resolver] no `node` found on PATH — will fall back to download" + ); + return None; + }; + + tracing::debug!( + path = %path.display(), + target_major, + "[node_runtime::resolver] probing system node" + ); + + let Some(version) = probe_node_version(&path) else { + tracing::warn!( + path = %path.display(), + "[node_runtime::resolver] `node --version` failed; treating as unavailable" + ); + return None; + }; + + let Some(host_major) = parse_node_version(&version) else { + tracing::warn!( + path = %path.display(), + version = %version, + "[node_runtime::resolver] could not parse `node --version` output" + ); + return None; + }; + + if host_major != target_major { + tracing::info!( + path = %path.display(), + host_major, + target_major, + "[node_runtime::resolver] host node major mismatch — will download managed runtime" + ); + return None; + } + + // `npm_exec` rides on the same resolved toolchain. On distros that + // package `nodejs` and `npm` separately (Debian/Ubuntu default, + // Alpine's `nodejs-current`, some NixOS setups) the `node` binary can + // be present without `npm`. If we cached `NodeSource::System` here + // every `npm_exec` call would break with an obscure error. Require a + // usable `npm --version` probe before accepting the system toolchain; + // on failure, return `None` so the managed download path takes over. + let Some(npm_path) = which_npm() else { + tracing::info!( + node_path = %path.display(), + "[node_runtime::resolver] compatible system node found but `npm` is missing on PATH — falling back to managed runtime" + ); + return None; + }; + + if probe_subcommand_version(&npm_path, "npm").is_none() { + tracing::warn!( + npm_path = %npm_path.display(), + "[node_runtime::resolver] `npm --version` failed; falling back to managed runtime" + ); + return None; + } + + let normalized = version.trim_start_matches('v').trim().to_string(); + tracing::info!( + path = %path.display(), + npm_path = %npm_path.display(), + version = %normalized, + "[node_runtime::resolver] reusing compatible system node (npm verified)" + ); + Some(SystemNode { + path, + major: host_major, + version: normalized, + }) +} + +/// Locate a `node` binary on `PATH`. Cross-platform: appends the host +/// executable suffix (`.exe` on Windows) so callers receive a path that can +/// be invoked directly. +/// +/// Unix command lookup skips non-executable entries. A non-executable +/// placeholder earlier in `PATH` (e.g. an unprivileged `node` shim left by +/// a failed install) would otherwise mask a valid later install and force +/// the managed runtime download. We mirror the shell behaviour by checking +/// the execute bit before returning. +fn which_node() -> Option { + let exe_name = format!("node{}", std::env::consts::EXE_SUFFIX); + which_exe(&exe_name) +} + +/// Locate an `npm` binary on `PATH`. Applies the same execute-bit filter +/// as [`which_node`]. On Windows we look for `npm.cmd` first (the official +/// installer ships a batch shim; there is no `npm.exe`) and fall back to +/// `npm` for unusual setups that expose a bare binary. +fn which_npm() -> Option { + #[cfg(windows)] + { + if let Some(p) = which_exe("npm.cmd") { + return Some(p); + } + which_exe("npm") + } + #[cfg(not(windows))] + { + which_exe("npm") + } +} + +/// `PATH` search helper shared by `which_node` / `which_npm`. Applies the +/// platform-specific executability check so a non-executable placeholder +/// earlier in `PATH` doesn't shadow a valid later entry. +fn which_exe(exe_name: &str) -> Option { + let path_var = std::env::var_os("PATH")?; + for dir in std::env::split_paths(&path_var) { + let candidate = dir.join(exe_name); + if is_executable_candidate(&candidate) { + return Some(candidate); + } + } + None +} + +#[cfg(unix)] +fn is_executable_candidate(path: &std::path::Path) -> bool { + use std::os::unix::fs::PermissionsExt; + std::fs::metadata(path) + .map(|meta| meta.is_file() && (meta.permissions().mode() & 0o111 != 0)) + .unwrap_or(false) +} + +#[cfg(not(unix))] +fn is_executable_candidate(path: &std::path::Path) -> bool { + // On Windows, the `.exe` suffix already encodes executability for the + // loader; any regular file matching `node.exe` is a valid candidate. + path.is_file() +} + +/// Invoke ` --version` with a real 5-second timeout and return the raw +/// version string on success. The timeout guards against a broken shim on +/// `PATH` hanging the bootstrap indefinitely. +fn probe_node_version(path: &std::path::Path) -> Option { + probe_subcommand_version(path, "node") +} + +/// Same semantics as [`probe_node_version`], but usable for arbitrary +/// toolchain binaries. `label` is only used for log attribution. +fn probe_subcommand_version(path: &std::path::Path, label: &str) -> Option { + use std::io::Read; + use wait_timeout::ChildExt; + + let mut child = Command::new(path) + .arg("--version") + .stdin(Stdio::null()) + .stdout(Stdio::piped()) + .stderr(Stdio::piped()) + .spawn() + .ok()?; + + let timeout = Duration::from_secs(5); + let status = match child.wait_timeout(timeout).ok()? { + Some(s) => s, + None => { + tracing::warn!( + path = %path.display(), + label, + timeout_secs = 5, + "[node_runtime::resolver] ` --version` timed out; killing process" + ); + let _ = child.kill(); + let _ = child.wait(); + return None; + } + }; + + if !status.success() { + let mut stderr_buf = String::new(); + if let Some(mut s) = child.stderr.take() { + let _ = s.read_to_string(&mut stderr_buf); + } + tracing::debug!( + status = ?status, + label, + stderr = %stderr_buf, + "[node_runtime::resolver] ` --version` exited non-zero" + ); + return None; + } + + let mut stdout_buf = String::new(); + if let Some(mut s) = child.stdout.take() { + let _ = s.read_to_string(&mut stdout_buf); + } + let trimmed = stdout_buf.trim().to_string(); + if trimmed.is_empty() { + return None; + } + Some(trimmed) +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn parses_version_with_v_prefix() { + assert_eq!(parse_node_version("v22.11.0"), Some(22)); + } + + #[test] + fn parses_version_without_v_prefix() { + assert_eq!(parse_node_version("22.11.0"), Some(22)); + } + + #[test] + fn parses_major_only() { + assert_eq!(parse_node_version("v22"), Some(22)); + } + + #[test] + fn tolerates_surrounding_whitespace() { + assert_eq!(parse_node_version(" v22.11.0\n"), Some(22)); + } + + #[test] + fn rejects_garbage() { + assert_eq!(parse_node_version("not-a-version"), None); + assert_eq!(parse_node_version(""), None); + assert_eq!(parse_node_version("v"), None); + } +} diff --git a/src/openhuman/skills/ops.rs b/src/openhuman/skills/ops.rs index 989dcb404..c069ee185 100644 --- a/src/openhuman/skills/ops.rs +++ b/src/openhuman/skills/ops.rs @@ -1,34 +1,177 @@ -//! High-level operations for managing skills. +//! Discovery and parsing of agentskills.io-style skills. //! -//! This module provides functions for initializing the skills directory, -//! loading skills from disk, and parsing legacy skill manifests (`skill.json`) -//! and documentation (`SKILL.md`). +//! A skill is a directory containing a `SKILL.md` file with YAML frontmatter +//! (`name`, `description`, …) followed by Markdown instructions. Optional +//! bundled resources live in sibling subdirectories (`scripts/`, `references/`, +//! `assets/`). +//! +//! Skills can be installed at two scopes: +//! - **User**: `~/.openhuman/skills//` or `~/.agents/skills//` +//! - **Project**: `/.openhuman/skills//` or +//! `/.agents/skills//` +//! +//! Project-scope skills are only loaded when a trust marker +//! (`/.openhuman/trust`) is present. When a skill name collides +//! across scopes, the project-scope copy wins. +//! +//! Legacy `skill.json` manifests and the flat `/skills//` +//! layout are still supported for backward compatibility. use serde::{Deserialize, Serialize}; +use std::collections::HashMap; use std::path::{Path, PathBuf}; -/// Represents a skill in the system. +const TRUST_MARKER: &str = "trust"; +const SKILL_MD: &str = "SKILL.md"; +const SKILL_JSON: &str = "skill.json"; +const MAX_NAME_LEN: usize = 64; +const MAX_DESCRIPTION_LEN: usize = 1024; +const RESOURCE_DIRS: &[&str] = &["scripts", "references", "assets"]; + +/// Where the skill was discovered. Determines precedence on name collision. +#[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize)] +#[serde(rename_all = "lowercase")] +pub enum SkillScope { + /// Skill shipped with the user's global config (`~/.openhuman/skills/...`). + User, + /// Skill shipped with the current workspace (`/.openhuman/skills/...`). + /// Requires the trust marker to be loaded. + Project, + /// Skill discovered under the legacy `/skills/` layout. + Legacy, +} + +impl Default for SkillScope { + fn default() -> Self { + Self::User + } +} + +/// Parsed frontmatter of a `SKILL.md` file. /// -/// This structure holds metadata about a skill, including its name, -/// description, version, and location on disk. +/// Matches the agentskills.io SKILL.md spec: `name` and `description` are +/// required; `license`, `compatibility`, `metadata`, and `allowed-tools` are +/// optional. Spec additions land in [`Self::extra`] via `#[serde(flatten)]`. +/// +/// Version, author, tags, and other non-required fields belong under +/// [`Self::metadata`]. Writers that still put them at the top level are +/// accepted with a migration warning. +#[derive(Debug, Clone, Default, Serialize, Deserialize)] +pub struct SkillFrontmatter { + #[serde(default)] + pub name: String, + #[serde(default)] + pub description: String, + #[serde(default)] + pub license: Option, + #[serde(default)] + pub compatibility: Option, + /// Spec-compliant metadata map. Version, author, tags, and other + /// non-required fields live here. + #[serde(default)] + pub metadata: HashMap, + /// Tools the skill author asserts their instructions rely on + /// (non-binding hint; the host decides what to expose). + #[serde(default, rename = "allowed-tools", alias = "allowed_tools")] + pub allowed_tools: Vec, + /// Forward-compat hatch for spec additions. Non-spec top-level keys + /// (including legacy `version`, `author`, `tags`) land here and trigger + /// a migration warning when read. + #[serde(flatten)] + pub extra: HashMap, +} + +fn metadata_string(fm: &SkillFrontmatter, key: &str) -> Option { + fm.metadata + .get(key) + .and_then(|v| v.as_str()) + .map(|s| s.to_string()) +} + +fn metadata_string_seq(value: &serde_yaml::Value) -> Vec { + value + .as_sequence() + .map(|seq| { + seq.iter() + .filter_map(|t| t.as_str().map(|s| s.to_string())) + .collect() + }) + .unwrap_or_default() +} + +fn extract_version(fm: &SkillFrontmatter, warnings: &mut Vec) -> String { + if let Some(v) = metadata_string(fm, "version") { + return v; + } + if let Some(v) = fm.extra.get("version").and_then(|v| v.as_str()) { + log::warn!("[skills] top-level 'version' is deprecated; move under 'metadata.version'"); + warnings + .push("top-level 'version' is deprecated; move under 'metadata.version'".to_string()); + return v.to_string(); + } + String::new() +} + +fn extract_author(fm: &SkillFrontmatter, warnings: &mut Vec) -> Option { + if let Some(v) = metadata_string(fm, "author") { + return Some(v); + } + if let Some(v) = fm.extra.get("author").and_then(|v| v.as_str()) { + log::warn!("[skills] top-level 'author' is deprecated; move under 'metadata.author'"); + warnings.push("top-level 'author' is deprecated; move under 'metadata.author'".to_string()); + return Some(v.to_string()); + } + None +} + +fn extract_tags(fm: &SkillFrontmatter, warnings: &mut Vec) -> Vec { + if let Some(v) = fm.metadata.get("tags") { + return metadata_string_seq(v); + } + if let Some(v) = fm.extra.get("tags") { + log::warn!("[skills] top-level 'tags' is deprecated; move under 'metadata.tags'"); + warnings.push("top-level 'tags' is deprecated; move under 'metadata.tags'".to_string()); + return metadata_string_seq(v); + } + Vec::new() +} + +/// A discovered skill. #[derive(Debug, Clone, Serialize, Deserialize, Default)] pub struct Skill { - /// Human-readable name of the skill. + /// Display name (from frontmatter, falls back to directory name). pub name: String, - /// Detailed description of what the skill does. + /// Short description used in the catalog summary. pub description: String, - /// Version string of the skill. + /// Version string, if declared. pub version: String, - /// Optional author of the skill. + /// Author string, if declared. pub author: Option, - /// List of tags associated with the skill for categorization. + /// Tags declared in frontmatter. pub tags: Vec, - /// List of tools provided by the skill. + /// Tool hint declared in frontmatter (`allowed-tools`). + #[serde(default)] pub tools: Vec, - /// List of prompt templates associated with the skill. + /// Prompt files declared in legacy `skill.json`. Unused for SKILL.md skills. + #[serde(default)] pub prompts: Vec, - /// Optional filesystem path to the skill's primary file (e.g., `SKILL.md`). + /// Path to the `SKILL.md` (or `skill.json`) file. pub location: Option, + /// Full parsed frontmatter when sourced from `SKILL.md`. + #[serde(default)] + pub frontmatter: SkillFrontmatter, + /// Bundled resource files (relative to the skill directory). + #[serde(default)] + pub resources: Vec, + /// Where the skill came from. + #[serde(default)] + pub scope: SkillScope, + /// True when loaded from the legacy `skill.json` / `/skills/` layout. + #[serde(default)] + pub legacy: bool, + /// Non-fatal parse warnings, surfaced in the catalog for user debugging. + #[serde(default)] + pub warnings: Vec, } /// Internal structure for parsing legacy `skill.json` manifests. @@ -50,9 +193,12 @@ struct LegacySkillManifest { prompts: Vec, } -/// Initialize the skills directory in the specified workspace. +/// Initialize the legacy skills directory in the specified workspace. /// -/// It creates the `skills` folder and a default `README.md` if they don't exist. +/// Creates `/skills/` and a placeholder `README.md` so the folder +/// is visible to the user. New-style skills should live under +/// `/.openhuman/skills/` instead, but this directory is kept for +/// backward compatibility. pub fn init_skills_dir(workspace_dir: &Path) -> Result<(), String> { let skills_dir = workspace_dir.join("skills"); std::fs::create_dir_all(&skills_dir).map_err(|e| { @@ -72,96 +218,450 @@ pub fn init_skills_dir(workspace_dir: &Path) -> Result<(), String> { Ok(()) } -/// Discover and load all skills from the `skills` directory in the workspace. +/// Backwards-compatible shim for callers that only have a workspace path. /// -/// It scans subdirectories, parses manifests (`skill.json`), and reads -/// descriptions from `SKILL.md` if necessary. +/// Delegates to [`discover_skills`] with the current user's home directory +/// so user-scope skills (`~/.openhuman/skills/`, `~/.agents/skills/`) are +/// surfaced for existing production callers (`agent::harness::session::builder`, +/// `channels::runtime::startup`). Previously this shim passed `None` for the +/// home directory, which silently dropped user-installed skills from the +/// main runtime path. +/// +/// Project-scope (workspace) skills still take precedence over user-scope +/// on name collisions. pub fn load_skills(workspace_dir: &Path) -> Vec { - let skills_dir = workspace_dir.join("skills"); - let entries = match std::fs::read_dir(&skills_dir) { + let trusted = is_workspace_trusted(workspace_dir); + let home = dirs::home_dir(); + discover_skills_inner(home.as_deref(), Some(workspace_dir), trusted) +} + +/// Discover skills from every supported location. +/// +/// * `home_dir` — user home (typically `dirs::home_dir()`), scanned for +/// `~/.openhuman/skills/` and `~/.agents/skills/`. +/// * `workspace_dir` — current workspace, scanned for project-scope paths. +/// * `trusted` — whether the caller has verified the project trust marker. +/// Project-scope skills are silently skipped when `false`. +/// +/// On name collisions, project-scope wins over user-scope and a warning is +/// attached to the retained skill. +pub fn discover_skills( + home_dir: Option<&Path>, + workspace_dir: Option<&Path>, + trusted: bool, +) -> Vec { + discover_skills_inner(home_dir, workspace_dir, trusted) +} + +/// Whether the workspace has opted into loading project-scope skills. +/// +/// Looks for `/.openhuman/trust`. The marker file's contents are +/// ignored — presence is sufficient. +pub fn is_workspace_trusted(workspace_dir: &Path) -> bool { + workspace_dir.join(".openhuman").join(TRUST_MARKER).exists() +} + +fn discover_skills_inner( + home_dir: Option<&Path>, + workspace_dir: Option<&Path>, + trusted: bool, +) -> Vec { + // Scan order matters for collision resolution: the last scope to register + // a name wins, so we scan user first, then project, then legacy. + let mut by_name: HashMap = HashMap::new(); + + if let Some(home) = home_dir { + for root in user_roots(home) { + absorb(&mut by_name, scan_root(&root, SkillScope::User)); + } + } + + if let Some(ws) = workspace_dir { + if trusted { + for root in project_roots(ws) { + absorb(&mut by_name, scan_root(&root, SkillScope::Project)); + } + } + // Legacy `/skills/` is always scanned so existing setups + // keep working without requiring users to move files or add the trust + // marker. Flagged with `legacy = true` so the UI can nudge migration. + absorb( + &mut by_name, + scan_root(&ws.join("skills"), SkillScope::Legacy), + ); + } + + let mut out: Vec = by_name.into_values().collect(); + out.sort_by(|a, b| a.name.cmp(&b.name)); + out +} + +fn user_roots(home: &Path) -> Vec { + vec![ + home.join(".openhuman").join("skills"), + home.join(".agents").join("skills"), + ] +} + +fn project_roots(workspace: &Path) -> Vec { + vec![ + workspace.join(".openhuman").join("skills"), + workspace.join(".agents").join("skills"), + ] +} + +fn absorb(by_name: &mut HashMap, incoming: Vec) { + for mut skill in incoming { + let key = skill.name.clone(); + if let Some(existing) = by_name.remove(&key) { + // Higher-precedence scope wins; lower loses and is dropped. + let (winner, loser) = if precedence(skill.scope) >= precedence(existing.scope) { + (&mut skill, existing) + } else { + // Put existing back; discard incoming. + let mut kept = existing; + kept.warnings.push(format!( + "name '{}' also declared in {:?} scope at {} (ignored)", + kept.name, + skill.scope, + skill + .location + .as_deref() + .map(|p| p.display().to_string()) + .unwrap_or_else(|| "".to_string()) + )); + by_name.insert(key, kept); + continue; + }; + winner.warnings.push(format!( + "shadowed {:?}-scope skill at {} with same name", + loser.scope, + loser + .location + .as_deref() + .map(|p| p.display().to_string()) + .unwrap_or_else(|| "".to_string()) + )); + } + by_name.insert(key, skill); + } +} + +fn precedence(scope: SkillScope) -> u8 { + match scope { + SkillScope::Legacy => 0, + SkillScope::User => 1, + SkillScope::Project => 2, + } +} + +fn scan_root(root: &Path, scope: SkillScope) -> Vec { + let entries = match std::fs::read_dir(root) { Ok(entries) => entries, Err(_) => return Vec::new(), }; - entries - .filter_map(Result::ok) - .filter_map(|entry| { - let path = entry.path(); - if !path.is_dir() { - return None; - } + // `read_dir` order is unspecified. When two sibling directories declare + // the same logical `frontmatter.name` (which can differ from the folder + // name), cross-scope/same-scope deduplication downstream would otherwise + // pick a non-deterministic winner across runs. Sort by on-disk directory + // name for a stable, reproducible order. + let mut entries: Vec<_> = entries.flatten().collect(); + entries.sort_by_key(|entry| entry.file_name()); - // Skip hidden directories (starting with '.') - let dir_name = entry.file_name().to_string_lossy().to_string(); - if dir_name.starts_with('.') { - return None; - } - - let manifest_path = path.join("skill.json"); - let skill_md_path = path.join("SKILL.md"); - - // Attempt to parse manifest if it exists, otherwise use directory name as fallback - let mut skill = if manifest_path.exists() { - parse_skill_manifest(&manifest_path, &dir_name) - } else { - Skill { - name: dir_name.clone(), - ..Skill::default() - } - }; - - // Fallback to SKILL.md for description if missing in manifest - if skill.description.is_empty() { - skill.description = read_skill_md_description(&skill_md_path) - .unwrap_or_else(|| "No description provided".to_string()); - } - - if skill.name.is_empty() { - skill.name = dir_name; - } - - // Link to the SKILL.md location if it exists - if skill.location.is_none() && skill_md_path.exists() { - skill.location = Some(skill_md_path); - } - - Some(skill) - }) - .collect() + let mut out = Vec::new(); + for entry in entries { + // Use `file_type()` rather than `path.is_dir()` so a symlinked + // child cannot be loaded as a skill. `is_dir()` dereferences + // symlinks, which would re-open out-of-tree loading even though + // `walk_files` already rejects symlinks deeper in the resource + // walker. Skip both symlinks and non-directory entries here; if + // the `file_type()` call itself fails (rare — transient I/O), + // treat it as "not safe to traverse" and skip. + let Ok(file_type) = entry.file_type() else { + continue; + }; + if file_type.is_symlink() || !file_type.is_dir() { + continue; + } + let path = entry.path(); + let dir_name = entry.file_name().to_string_lossy().to_string(); + if dir_name.starts_with('.') { + continue; + } + if let Some(skill) = load_skill_dir(&path, &dir_name, scope) { + out.push(skill); + } + } + out } -/// Parse a legacy `skill.json` manifest from the given path. -fn parse_skill_manifest(path: &Path, fallback_name: &str) -> Skill { - let manifest = std::fs::read_to_string(path) - .ok() - .and_then(|content| serde_json::from_str::(&content).ok()); +fn load_skill_dir(dir: &Path, dir_name: &str, scope: SkillScope) -> Option { + let skill_md = dir.join(SKILL_MD); + let legacy_manifest = dir.join(SKILL_JSON); - match manifest { - Some(manifest) => Skill { - name: if manifest.name.trim().is_empty() { - fallback_name.to_string() - } else { - manifest.name - }, - description: manifest.description, - version: manifest.version, - author: manifest.author, - tags: manifest.tags, - tools: manifest.tools, - prompts: manifest.prompts, - location: None, - }, - None => Skill { - name: fallback_name.to_string(), - ..Skill::default() - }, + if skill_md.exists() { + return Some(load_from_skill_md(&skill_md, dir, dir_name, scope)); + } + if legacy_manifest.exists() { + return Some(load_from_legacy_manifest( + &legacy_manifest, + dir, + dir_name, + scope, + )); + } + None +} + +fn load_from_skill_md(skill_md: &Path, dir: &Path, dir_name: &str, scope: SkillScope) -> Skill { + let mut warnings = Vec::new(); + let (frontmatter, body) = match parse_skill_md(skill_md) { + Some((fm, body, parse_warnings)) => { + warnings.extend(parse_warnings); + (fm, body) + } + None => { + warnings.push(format!( + "could not parse {} — exposing directory as placeholder", + skill_md.display() + )); + (SkillFrontmatter::default(), String::new()) + } + }; + + let name = if frontmatter.name.trim().is_empty() { + warnings.push("frontmatter missing 'name'; using directory name".to_string()); + dir_name.to_string() + } else { + if frontmatter.name != dir_name { + warnings.push(format!( + "frontmatter name '{}' does not match directory '{}'", + frontmatter.name, dir_name + )); + } + if frontmatter.name.len() > MAX_NAME_LEN { + warnings.push(format!( + "frontmatter name is {} chars (max recommended: {})", + frontmatter.name.len(), + MAX_NAME_LEN + )); + } + frontmatter.name.clone() + }; + + let description = if frontmatter.description.trim().is_empty() { + warnings + .push("frontmatter missing 'description'; falling back to first body line".to_string()); + first_body_line(&body).unwrap_or_else(|| "No description provided".to_string()) + } else { + if frontmatter.description.len() > MAX_DESCRIPTION_LEN { + warnings.push(format!( + "description is {} chars (max recommended: {})", + frontmatter.description.len(), + MAX_DESCRIPTION_LEN + )); + } + frontmatter.description.clone() + }; + + let version = extract_version(&frontmatter, &mut warnings); + let author = extract_author(&frontmatter, &mut warnings); + let tags = extract_tags(&frontmatter, &mut warnings); + let tools = frontmatter.allowed_tools.clone(); + + Skill { + name, + description, + version, + author, + tags, + tools, + prompts: Vec::new(), + location: Some(skill_md.to_path_buf()), + frontmatter, + resources: inventory_resources(dir), + scope, + legacy: false, + warnings, } } -/// Extract the first non-empty, non-header line from a `SKILL.md` file as the description. -fn read_skill_md_description(path: &Path) -> Option { +fn load_from_legacy_manifest( + manifest_path: &Path, + dir: &Path, + dir_name: &str, + scope: SkillScope, +) -> Skill { + let mut warnings = vec![format!( + "skill uses legacy skill.json; migrate to SKILL.md frontmatter" + )]; + let parsed = std::fs::read_to_string(manifest_path) + .ok() + .and_then(|content| serde_json::from_str::(&content).ok()); + + let manifest = parsed.unwrap_or_else(|| { + warnings.push(format!( + "could not parse {} as JSON; using directory name", + manifest_path.display() + )); + LegacySkillManifest { + name: dir_name.to_string(), + description: String::new(), + version: String::new(), + author: None, + tags: Vec::new(), + tools: Vec::new(), + prompts: Vec::new(), + } + }); + + let name = if manifest.name.trim().is_empty() { + dir_name.to_string() + } else { + manifest.name + }; + + // `load_from_legacy_manifest` is only called when SKILL.md is absent + // (see load_skill_dir), so there is no SKILL.md to fall back to here. + let description = if manifest.description.is_empty() { + "No description provided".to_string() + } else { + manifest.description + }; + + let location = Some(manifest_path.to_path_buf()); + + Skill { + name, + description, + version: manifest.version, + author: manifest.author, + tags: manifest.tags, + tools: manifest.tools, + prompts: manifest.prompts, + location, + frontmatter: SkillFrontmatter::default(), + resources: inventory_resources(dir), + scope, + legacy: true, + warnings, + } +} + +/// Split a `SKILL.md` file into parsed frontmatter and the remaining body. +/// +/// Accepts frontmatter delimited by leading `---` lines. Returns `None` when +/// the file cannot be read or the frontmatter block is unterminated. +/// +/// The third element of the tuple carries parse-level diagnostics — for now +/// just the YAML deserialisation error when frontmatter exists but is +/// malformed. Callers merge these into the skill's user-visible warnings so +/// the catalog surfaces the real cause instead of a generic "could not parse" +/// placeholder. +pub fn parse_skill_md(path: &Path) -> Option<(SkillFrontmatter, String, Vec)> { let content = std::fs::read_to_string(path).ok()?; - for line in content.lines() { + let mut lines = content.lines(); + let first = lines.next()?; + if first.trim() != "---" { + // No frontmatter — treat whole file as body. + return Some((SkillFrontmatter::default(), content, Vec::new())); + } + + let mut yaml = String::new(); + let mut terminated = false; + let mut body = String::new(); + 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'); + } + } + + if !terminated { + return None; + } + + let mut parse_warnings = Vec::new(); + let frontmatter = match serde_yaml::from_str::(&yaml) { + Ok(fm) => fm, + Err(err) => { + log::warn!( + "[skills] failed to parse frontmatter in {}: {err}", + path.display() + ); + parse_warnings.push(format!("frontmatter parse error: {err}")); + SkillFrontmatter::default() + } + }; + + Some((frontmatter, body, parse_warnings)) +} + +/// Shallow-scan a skill directory for bundled resources. +/// +/// Returns every file (relative to `dir`) under any of the conventional +/// resource subdirectories (`scripts/`, `references/`, `assets/`). Deeper +/// nesting is walked recursively. +pub fn inventory_resources(dir: &Path) -> Vec { + let mut out = Vec::new(); + for sub in RESOURCE_DIRS { + let root = dir.join(sub); + // `root.is_dir()` follows symlinks, so a `scripts -> /some/other/tree` + // symlink would still pass and `walk_files` would inventory the + // external tree. Use `symlink_metadata` for a non-dereferencing check + // and reject symlinked roots outright; `walk_files` already guards + // deeper symlinks inside the tree. + let meta = match std::fs::symlink_metadata(&root) { + Ok(m) => m, + Err(_) => continue, + }; + if meta.file_type().is_symlink() || !meta.is_dir() { + continue; + } + walk_files(&root, dir, &mut out); + } + out.sort(); + out +} + +fn walk_files(current: &Path, base: &Path, out: &mut Vec) { + let entries = match std::fs::read_dir(current) { + Ok(e) => e, + Err(_) => return, + }; + for entry in entries.flatten() { + // Use `file_type()` — not `is_dir()` / `is_file()` — so we can detect and + // skip symlinks before traversing. `is_dir()`/`is_file()` follow symlinks + // and would cause unbounded recursion on a cycle (e.g. `resources/self -> + // resources/`) or silent leakage outside the skill directory when a + // symlink points at `/`, `/etc`, or another skill's tree. + let Ok(file_type) = entry.file_type() else { + continue; + }; + if file_type.is_symlink() { + continue; + } + let path = entry.path(); + if file_type.is_dir() { + walk_files(&path, base, out); + } else if file_type.is_file() { + if let Ok(rel) = path.strip_prefix(base) { + out.push(rel.to_path_buf()); + } + } + } +} + +fn first_body_line(body: &str) -> Option { + for line in body.lines() { let trimmed = line.trim(); if trimmed.is_empty() || trimmed.starts_with('#') { continue; @@ -175,6 +675,24 @@ fn read_skill_md_description(path: &Path) -> Option { mod tests { use super::*; + fn write(path: &Path, content: &str) { + if let Some(parent) = path.parent() { + std::fs::create_dir_all(parent).unwrap(); + } + std::fs::write(path, content).unwrap(); + } + + /// Workspace-only variant of [`load_skills`] used by tests that care only + /// about project-scope semantics. The production [`load_skills`] now + /// consults `dirs::home_dir()`; in unit tests that would non-deterministically + /// pick up whatever skills the developer has installed under their real + /// home. Tests exercising user-scope delegation drive a tempdir through + /// [`discover_skills`] explicitly (see `load_skills_surfaces_user_scope`). + fn load_skills_ws(workspace_dir: &Path) -> Vec { + let trusted = is_workspace_trusted(workspace_dir); + discover_skills_inner(None, Some(workspace_dir), trusted) + } + #[test] fn init_skills_dir_creates_dir_and_readme() { let dir = tempfile::tempdir().unwrap(); @@ -183,79 +701,328 @@ mod tests { assert!(skills_dir.is_dir()); let readme = skills_dir.join("README.md"); assert!(readme.exists()); - let content = std::fs::read_to_string(&readme).unwrap(); - assert!(content.contains("Skills")); } #[test] - fn init_skills_dir_idempotent() { - let dir = tempfile::tempdir().unwrap(); - init_skills_dir(dir.path()).unwrap(); - init_skills_dir(dir.path()).unwrap(); // should not fail - } - - #[test] - fn load_skills_empty_dir() { - let dir = tempfile::tempdir().unwrap(); - init_skills_dir(dir.path()).unwrap(); - let skills = load_skills(dir.path()); - assert!(skills.is_empty()); - } - - #[test] - fn load_skills_with_manifest() { + fn load_skills_legacy_json_still_works() { let dir = tempfile::tempdir().unwrap(); init_skills_dir(dir.path()).unwrap(); let skill_dir = dir.path().join("skills").join("my-skill"); std::fs::create_dir_all(&skill_dir).unwrap(); - std::fs::write( - skill_dir.join("skill.json"), + write( + &skill_dir.join("skill.json"), r#"{"name":"My Skill","description":"A test","version":"1.0"}"#, - ) - .unwrap(); - let skills = load_skills(dir.path()); + ); + let skills = load_skills_ws(dir.path()); assert_eq!(skills.len(), 1); assert_eq!(skills[0].name, "My Skill"); assert_eq!(skills[0].description, "A test"); - assert_eq!(skills[0].version, "1.0"); + assert!(skills[0].legacy); + assert_eq!(skills[0].scope, SkillScope::Legacy); } #[test] - fn load_skills_fallback_name_from_dir() { + fn load_skills_parses_skill_md_frontmatter() { let dir = tempfile::tempdir().unwrap(); - init_skills_dir(dir.path()).unwrap(); - let skill_dir = dir.path().join("skills").join("fallback-name"); - std::fs::create_dir_all(&skill_dir).unwrap(); - // No skill.json, but has a SKILL.md - std::fs::write( - skill_dir.join("SKILL.md"), - "# Title\n\nThis is the description.", - ) - .unwrap(); - let skills = load_skills(dir.path()); + let ws = dir.path(); + // Trust marker enables project-scope loading. + write(&ws.join(".openhuman").join("trust"), ""); + let skill_dir = ws.join(".openhuman").join("skills").join("hello-world"); + write( + &skill_dir.join("SKILL.md"), + "---\nname: hello-world\ndescription: Say hi\nmetadata:\n version: 0.1.0\n tags: [demo, greeting]\n---\n\nSay hello to the user.\n", + ); + let skills = load_skills_ws(ws); assert_eq!(skills.len(), 1); - assert_eq!(skills[0].name, "fallback-name"); - assert_eq!(skills[0].description, "This is the description."); + let s = &skills[0]; + assert_eq!(s.name, "hello-world"); + assert_eq!(s.description, "Say hi"); + assert_eq!(s.version, "0.1.0"); + assert_eq!(s.tags, vec!["demo", "greeting"]); + assert_eq!(s.scope, SkillScope::Project); + assert!(!s.legacy); + assert!(s.warnings.is_empty(), "warnings: {:?}", s.warnings); } #[test] - fn load_skills_ignores_hidden_dirs() { + fn deprecated_top_level_fields_load_with_migration_warning() { let dir = tempfile::tempdir().unwrap(); - init_skills_dir(dir.path()).unwrap(); - let hidden = dir.path().join("skills").join(".hidden"); - std::fs::create_dir_all(&hidden).unwrap(); - let skills = load_skills(dir.path()); + let ws = dir.path(); + write(&ws.join(".openhuman").join("trust"), ""); + let skill_dir = ws.join(".openhuman").join("skills").join("legacy-fm"); + write( + &skill_dir.join("SKILL.md"), + "---\nname: legacy-fm\ndescription: uses deprecated top-level fields\nversion: 0.2.0\nauthor: Jane\ntags: [old, school]\n---\n", + ); + let skills = load_skills_ws(ws); + assert_eq!(skills.len(), 1); + let s = &skills[0]; + assert_eq!(s.version, "0.2.0"); + assert_eq!(s.author.as_deref(), Some("Jane")); + assert_eq!(s.tags, vec!["old", "school"]); + let warnings = s.warnings.join("\n"); + assert!(warnings.contains("'version' is deprecated"), "{}", warnings); + assert!(warnings.contains("'author' is deprecated"), "{}", warnings); + assert!(warnings.contains("'tags' is deprecated"), "{}", warnings); + } + + #[test] + fn spec_compliant_fields_parse_into_metadata_map() { + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("SKILL.md"); + write( + &path, + "---\nname: s\ndescription: d\nlicense: MIT\ncompatibility: \"node>=18\"\nmetadata:\n version: 1.0.0\n author: Alice\n tags: [a, b]\n---\n", + ); + let (fm, _body, _warnings) = parse_skill_md(&path).unwrap(); + assert_eq!(fm.license.as_deref(), Some("MIT")); + assert_eq!(fm.compatibility.as_deref(), Some("node>=18")); + assert_eq!( + fm.metadata.get("version").and_then(|v| v.as_str()), + Some("1.0.0") + ); + assert_eq!( + fm.metadata.get("author").and_then(|v| v.as_str()), + Some("Alice") + ); + assert!(fm.extra.is_empty(), "extras leaked: {:?}", fm.extra); + } + + #[test] + fn project_skills_skipped_when_not_trusted() { + let dir = tempfile::tempdir().unwrap(); + let ws = dir.path(); + // No trust marker. + let skill_dir = ws.join(".openhuman").join("skills").join("unsafe"); + write( + &skill_dir.join("SKILL.md"), + "---\nname: unsafe\ndescription: should not load\n---\n", + ); + let skills = load_skills_ws(ws); + assert!(skills.is_empty(), "got {skills:?}"); + } + + #[test] + fn frontmatter_missing_name_warns_and_falls_back() { + let dir = tempfile::tempdir().unwrap(); + let ws = dir.path(); + write(&ws.join(".openhuman").join("trust"), ""); + let skill_dir = ws.join(".openhuman").join("skills").join("mystery"); + write( + &skill_dir.join("SKILL.md"), + "---\ndescription: no name here\n---\n\nbody\n", + ); + let skills = load_skills_ws(ws); + assert_eq!(skills.len(), 1); + assert_eq!(skills[0].name, "mystery"); + assert!(skills[0] + .warnings + .iter() + .any(|w| w.contains("missing 'name'"))); + } + + #[test] + fn frontmatter_missing_description_uses_first_body_line() { + let dir = tempfile::tempdir().unwrap(); + let ws = dir.path(); + write(&ws.join(".openhuman").join("trust"), ""); + let skill_dir = ws.join(".openhuman").join("skills").join("s"); + write( + &skill_dir.join("SKILL.md"), + "---\nname: s\n---\n\n# Heading\n\nActual first line.\n", + ); + let skills = load_skills_ws(ws); + assert_eq!(skills[0].description, "Actual first line."); + } + + #[test] + fn directory_name_mismatch_warns_but_loads() { + let dir = tempfile::tempdir().unwrap(); + let ws = dir.path(); + write(&ws.join(".openhuman").join("trust"), ""); + let skill_dir = ws.join(".openhuman").join("skills").join("dir-name"); + write( + &skill_dir.join("SKILL.md"), + "---\nname: other-name\ndescription: mismatch\n---\n", + ); + let skills = load_skills_ws(ws); + assert_eq!(skills.len(), 1); + assert_eq!(skills[0].name, "other-name"); + assert!(skills[0] + .warnings + .iter() + .any(|w| w.contains("does not match directory"))); + } + + #[test] + fn project_scope_shadows_user_scope_on_collision() { + let user_dir = tempfile::tempdir().unwrap(); + let ws_dir = tempfile::tempdir().unwrap(); + write(&ws_dir.path().join(".openhuman").join("trust"), ""); + + let user_skill = user_dir + .path() + .join(".openhuman") + .join("skills") + .join("greet"); + write( + &user_skill.join("SKILL.md"), + "---\nname: greet\ndescription: USER COPY\n---\n", + ); + + let proj_skill = ws_dir + .path() + .join(".openhuman") + .join("skills") + .join("greet"); + write( + &proj_skill.join("SKILL.md"), + "---\nname: greet\ndescription: PROJECT COPY\n---\n", + ); + + let skills = discover_skills(Some(user_dir.path()), Some(ws_dir.path()), true); + assert_eq!(skills.len(), 1); + assert_eq!(skills[0].description, "PROJECT COPY"); + assert!(skills[0].warnings.iter().any(|w| w.contains("shadowed"))); + } + + #[test] + fn inventory_resources_lists_scripts_and_assets() { + let dir = tempfile::tempdir().unwrap(); + let skill = dir.path().join("s"); + write( + &skill.join("SKILL.md"), + "---\nname: s\ndescription: d\n---\n", + ); + write(&skill.join("scripts").join("run.sh"), "echo hi"); + write(&skill.join("references").join("notes.md"), "notes"); + write(&skill.join("assets").join("logo.png"), ""); + write(&skill.join("unrelated").join("x.txt"), "ignored"); + + let mut res = inventory_resources(&skill); + res.sort(); + assert_eq!(res.len(), 3); + assert!(res.iter().any(|p| p.ends_with("run.sh"))); + assert!(res.iter().any(|p| p.ends_with("notes.md"))); + assert!(res.iter().any(|p| p.ends_with("logo.png"))); + assert!(!res.iter().any(|p| p.ends_with("x.txt"))); + } + + #[test] + fn parse_skill_md_without_frontmatter_returns_body() { + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("SKILL.md"); + write(&path, "just a markdown body\n"); + let (fm, body, _warnings) = parse_skill_md(&path).unwrap(); + assert!(fm.name.is_empty()); + assert!(body.contains("markdown body")); + } + + #[test] + fn parse_skill_md_unterminated_frontmatter_returns_none() { + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("SKILL.md"); + write(&path, "---\nname: bad\n\nbody without closing marker\n"); + assert!(parse_skill_md(&path).is_none()); + } + + #[cfg(unix)] + #[test] + fn symlinked_skill_dirs_are_skipped() { + use std::os::unix::fs::symlink; + + let dir = tempfile::tempdir().unwrap(); + let ws = dir.path(); + write(&ws.join(".openhuman").join("trust"), ""); + + // A real out-of-tree skill that would load fine if linked. + let external = tempfile::tempdir().unwrap(); + let external_skill = external.path().join("evil"); + write( + &external_skill.join("SKILL.md"), + "---\nname: evil\ndescription: should not load via symlink\n---\n", + ); + + // Symlink /.openhuman/skills/evil -> external/evil + let skills_root = ws.join(".openhuman").join("skills"); + std::fs::create_dir_all(&skills_root).unwrap(); + symlink(&external_skill, skills_root.join("evil")).unwrap(); + + let skills = load_skills_ws(ws); + assert!( + skills.is_empty(), + "symlinked skill dir should be skipped, got: {skills:?}" + ); + } + + #[cfg(unix)] + #[test] + fn symlinked_resource_roots_are_rejected() { + use std::os::unix::fs::symlink; + + let dir = tempfile::tempdir().unwrap(); + let skill = dir.path().join("s"); + write( + &skill.join("SKILL.md"), + "---\nname: s\ndescription: d\n---\n", + ); + + // External directory that must not be inventoried. + let external = tempfile::tempdir().unwrap(); + write(&external.path().join("leaked.txt"), "should not appear"); + + // Symlink /assets -> external + std::fs::create_dir_all(&skill).unwrap(); + symlink(external.path(), skill.join("assets")).unwrap(); + + let res = inventory_resources(&skill); + assert!( + res.is_empty(), + "symlinked resource root must be rejected, got: {res:?}" + ); + } + + #[test] + fn load_skills_surfaces_user_scope() { + // load_skills now delegates to discover_skills with dirs::home_dir(), + // so user-scope skills reach production callers that still hit the + // backwards-compat shim. Simulate this with an explicit tempdir home + // via discover_skills — we can't safely override the process HOME in + // unit tests. + let user_dir = tempfile::tempdir().unwrap(); + let ws_dir = tempfile::tempdir().unwrap(); + + let user_skill = user_dir + .path() + .join(".openhuman") + .join("skills") + .join("user-only"); + write( + &user_skill.join("SKILL.md"), + "---\nname: user-only\ndescription: from user home\n---\n", + ); + + let skills = discover_skills( + Some(user_dir.path()), + Some(ws_dir.path()), + is_workspace_trusted(ws_dir.path()), + ); + assert_eq!(skills.len(), 1); + assert_eq!(skills[0].name, "user-only"); + assert_eq!(skills[0].scope, SkillScope::User); + } + + #[test] + fn hidden_dirs_are_skipped() { + let dir = tempfile::tempdir().unwrap(); + let ws = dir.path(); + write(&ws.join(".openhuman").join("trust"), ""); + let hidden = ws.join(".openhuman").join("skills").join(".hidden"); + write( + &hidden.join("SKILL.md"), + "---\nname: hidden\ndescription: nope\n---\n", + ); + let skills = load_skills_ws(ws); assert!(skills.is_empty()); } - - #[test] - fn parse_skill_manifest_missing_fields_uses_defaults() { - let dir = tempfile::tempdir().unwrap(); - let path = dir.path().join("skill.json"); - std::fs::write(&path, "{}").unwrap(); - let skill = parse_skill_manifest(&path, "fallback"); - assert_eq!(skill.name, "fallback"); - assert!(skill.description.is_empty()); - assert!(skill.tags.is_empty()); - } } diff --git a/src/openhuman/tools/impl/system/mod.rs b/src/openhuman/tools/impl/system/mod.rs index 2181f5d60..56f9d8387 100644 --- a/src/openhuman/tools/impl/system/mod.rs +++ b/src/openhuman/tools/impl/system/mod.rs @@ -1,5 +1,7 @@ mod current_time; mod insert_sql_record; +mod node_exec; +mod npm_exec; mod proxy_config; mod pushover; mod schedule; @@ -9,6 +11,8 @@ mod workspace_state; pub use current_time::CurrentTimeTool; pub use insert_sql_record::InsertSqlRecordTool; +pub use node_exec::NodeExecTool; +pub use npm_exec::NpmExecTool; pub use proxy_config::ProxyConfigTool; pub use pushover::PushoverTool; pub use schedule::ScheduleTool; diff --git a/src/openhuman/tools/impl/system/node_exec.rs b/src/openhuman/tools/impl/system/node_exec.rs new file mode 100644 index 000000000..6f7f5d320 --- /dev/null +++ b/src/openhuman/tools/impl/system/node_exec.rs @@ -0,0 +1,345 @@ +//! `node_exec` — execute JavaScript via the managed (or system) Node.js +//! toolchain. +//! +//! Sibling to [`crate::openhuman::tools::impl::system::shell::ShellTool`]: same +//! security gates, same env hygiene, but the command is pinned to the `node` +//! binary resolved by +//! [`crate::openhuman::node_runtime::NodeBootstrap`]. +//! +//! Two input modes: +//! +//! | Mode | Params | Resulting invocation | +//! |---------------|------------------------------------------|-------------------------------------| +//! | Inline code | `inline_code: "console.log(1+1)"` | `node -e ''` | +//! | Script path | `script_path: "scripts/run.js"`, `args` | `node ` | +//! +//! Exactly one of `inline_code` / `script_path` must be supplied. Scripts are +//! resolved relative to the workspace; paths escaping the workspace are +//! rejected by the filesystem helpers. +//! +//! The bootstrap is resolved **on first invocation**, which will download + +//! extract a managed Node.js distribution if no compatible `node` is on +//! `PATH`. Subsequent calls reuse the cached install. + +use crate::openhuman::agent::host_runtime::RuntimeAdapter; +use crate::openhuman::node_runtime::NodeBootstrap; +use crate::openhuman::security::SecurityPolicy; +use crate::openhuman::tools::traits::{Tool, ToolResult}; +use async_trait::async_trait; +use serde_json::json; +use std::sync::Arc; +use std::time::Duration; + +/// Maximum node process wall-clock before we kill it. Longer than the shell +/// tool because `npm install` / bundler steps can legitimately exceed 60s, +/// and `node_exec` is often the launcher for those flows. +const NODE_TIMEOUT_SECS: u64 = 300; +/// Maximum combined stdout/stderr size (1 MB each) — same cap as shell. +const MAX_OUTPUT_BYTES: usize = 1_048_576; +/// Env allow-list for child processes. Matches shell.rs — secrets never leak +/// into spawned node processes. `PATH` gets a prepend of the managed bin +/// dir before being forwarded. +const SAFE_ENV_VARS: &[&str] = &[ + "HOME", "TERM", "LANG", "LC_ALL", "LC_CTYPE", "USER", "SHELL", "TMPDIR", +]; + +/// `node_exec` — execute JavaScript through the resolved Node.js runtime. +pub struct NodeExecTool { + security: Arc, + runtime: Arc, + bootstrap: Arc, +} + +impl NodeExecTool { + pub fn new( + security: Arc, + runtime: Arc, + bootstrap: Arc, + ) -> Self { + Self { + security, + runtime, + bootstrap, + } + } +} + +#[async_trait] +impl Tool for NodeExecTool { + fn name(&self) -> &str { + "node_exec" + } + + fn description(&self) -> &str { + "Execute JavaScript through Node.js. Pass either `inline_code` (runs via `node -e`) or `script_path` (runs a file in the workspace). Optional `args` forwards positional arguments to the script." + } + + fn parameters_schema(&self) -> serde_json::Value { + json!({ + "type": "object", + "properties": { + "inline_code": { + "type": "string", + "description": "JavaScript source passed to `node -e`. Mutually exclusive with script_path." + }, + "script_path": { + "type": "string", + "description": "Path (relative to workspace) to a .js/.mjs/.cjs file. Mutually exclusive with inline_code." + }, + "args": { + "type": "array", + "items": { "type": "string" }, + "description": "Positional arguments appended after the script. Ignored for inline_code." + }, + "timeout_secs": { + "type": "integer", + "description": "Optional override for the default 300s timeout. Capped at 1800s." + } + } + }) + } + + async fn execute(&self, args: serde_json::Value) -> anyhow::Result { + let inline_code = args + .get("inline_code") + .and_then(|v| v.as_str()) + .map(str::to_string); + let script_path = args + .get("script_path") + .and_then(|v| v.as_str()) + .map(str::to_string); + + let extra_args: Vec = args + .get("args") + .and_then(|v| v.as_array()) + .map(|arr| { + arr.iter() + .filter_map(|v| v.as_str().map(str::to_string)) + .collect() + }) + .unwrap_or_default(); + + let timeout_secs = args + .get("timeout_secs") + .and_then(|v| v.as_u64()) + .unwrap_or(NODE_TIMEOUT_SECS) + .min(1800); + + if inline_code.is_some() == script_path.is_some() { + return Ok(ToolResult::error( + "node_exec requires exactly one of `inline_code` or `script_path`", + )); + } + + if self.security.is_rate_limited() { + return Ok(ToolResult::error( + "Rate limit exceeded: too many actions in the last hour", + )); + } + if !self.security.record_action() { + return Ok(ToolResult::error( + "Rate limit exceeded: action budget exhausted", + )); + } + + let resolved = match self.bootstrap.resolve().await { + Ok(r) => r, + Err(e) => { + tracing::error!(error = %e, "[node_exec] failed to resolve node runtime"); + return Ok(ToolResult::error(format!( + "Node.js runtime unavailable: {e}" + ))); + } + }; + + tracing::info!( + version = %resolved.version, + source = ?resolved.source, + node_bin = %resolved.node_bin.display(), + "[node_exec] starting invocation" + ); + + let command = if let Some(code) = inline_code.as_deref() { + format!( + "{} -e {}", + shell_quote(&resolved.node_bin.to_string_lossy()), + shell_quote(code) + ) + } else if let Some(path) = script_path.as_deref() { + let resolved_script = match resolve_script_path(&self.security.workspace_dir, path) { + Ok(p) => p, + Err(msg) => return Ok(ToolResult::error(msg)), + }; + let mut parts: Vec = Vec::with_capacity(extra_args.len() + 2); + parts.push(shell_quote(&resolved.node_bin.to_string_lossy())); + parts.push(shell_quote(&resolved_script.to_string_lossy())); + // `extra_args` are opaque positional arguments forwarded to the + // script. They are shell-quoted below so no shell metacharacter + // can escape, but we do NOT treat them as workspace paths — the + // script itself is responsible for any path validation it does + // on its own arguments. + for a in &extra_args { + parts.push(shell_quote(a)); + } + parts.join(" ") + } else { + unreachable!("guarded above") + }; + + let mut cmd = match self + .runtime + .build_shell_command(&command, &self.security.workspace_dir) + { + Ok(cmd) => cmd, + Err(e) => { + return Ok(ToolResult::error(format!( + "Failed to build runtime command: {e}" + ))); + } + }; + + cmd.env_clear(); + + let host_path = std::env::var("PATH").unwrap_or_default(); + let sep = if cfg!(windows) { ";" } else { ":" }; + let prepended_path = if host_path.is_empty() { + resolved.bin_dir.to_string_lossy().into_owned() + } else { + format!("{}{}{}", resolved.bin_dir.display(), sep, host_path) + }; + cmd.env("PATH", &prepended_path); + + for var in SAFE_ENV_VARS { + if let Ok(val) = std::env::var(var) { + cmd.env(var, val); + } + } + + let result = tokio::time::timeout(Duration::from_secs(timeout_secs), cmd.output()).await; + + match result { + Ok(Ok(output)) => { + let mut stdout = String::from_utf8_lossy(&output.stdout).to_string(); + let mut stderr = String::from_utf8_lossy(&output.stderr).to_string(); + + if stdout.len() > MAX_OUTPUT_BYTES { + stdout.truncate(stdout.floor_char_boundary(MAX_OUTPUT_BYTES)); + stdout.push_str("\n... [stdout truncated at 1MB]"); + } + if stderr.len() > MAX_OUTPUT_BYTES { + stderr.truncate(stderr.floor_char_boundary(MAX_OUTPUT_BYTES)); + stderr.push_str("\n... [stderr truncated at 1MB]"); + } + + if output.status.success() { + if stderr.is_empty() { + Ok(ToolResult::success(stdout)) + } else { + Ok(ToolResult::success(format!("{stdout}\n[stderr]\n{stderr}"))) + } + } else { + let err_msg = if stderr.is_empty() { stdout } else { stderr }; + Ok(ToolResult::error(err_msg)) + } + } + Ok(Err(e)) => Ok(ToolResult::error(format!("Failed to execute node: {e}"))), + Err(_) => Ok(ToolResult::error(format!( + "node_exec timed out after {timeout_secs}s and was killed" + ))), + } + } +} + +/// POSIX-safe single-quote escaping. Wraps `s` in `'…'`, turning any embedded +/// single-quote into the four-char sequence `'\''`. Node bin paths and user +/// code pass through untouched semantically, but no shell metacharacter can +/// escape the quoted string. +fn shell_quote(s: &str) -> String { + let escaped = s.replace('\'', "'\\''"); + format!("'{escaped}'") +} + +/// Resolve a caller-supplied `script_path` against the workspace. Mirrors +/// `npm_exec::resolve_cwd` — rejects absolute paths and any component that +/// could escape the workspace (`..`, Windows drive prefixes). Scripts +/// themselves must live inside the workspace. +fn resolve_script_path( + workspace: &std::path::Path, + raw: &str, +) -> Result { + let raw = raw.trim(); + if raw.is_empty() { + return Err("node_exec `script_path` cannot be empty".to_string()); + } + let candidate = std::path::Path::new(raw); + if candidate.is_absolute() { + return Err(format!( + "node_exec `script_path` must be relative to workspace; got absolute {raw:?}" + )); + } + if candidate.components().any(|c| { + matches!( + c, + std::path::Component::ParentDir | std::path::Component::Prefix(_) + ) + }) { + return Err(format!( + "node_exec `script_path` must not escape workspace; got {raw:?}" + )); + } + Ok(workspace.join(candidate)) +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn shell_quote_wraps_plain_strings() { + assert_eq!(shell_quote("node"), "'node'"); + assert_eq!(shell_quote("/opt/bin/node"), "'/opt/bin/node'"); + } + + #[test] + fn shell_quote_escapes_single_quotes() { + assert_eq!(shell_quote("it's"), "'it'\\''s'"); + assert_eq!( + shell_quote("console.log('hi')"), + "'console.log('\\''hi'\\'')'" + ); + } + + #[test] + fn shell_quote_neutralises_metacharacters() { + // $, backticks, && — all inert once wrapped in single quotes. + assert_eq!(shell_quote("$(rm -rf /)"), "'$(rm -rf /)'"); + assert_eq!(shell_quote("a && b"), "'a && b'"); + } + + #[test] + fn resolve_script_path_rejects_empty() { + let ws = std::path::Path::new("/ws"); + assert!(resolve_script_path(ws, "").is_err()); + assert!(resolve_script_path(ws, " ").is_err()); + } + + #[test] + fn resolve_script_path_rejects_absolute() { + let ws = std::path::Path::new("/ws"); + assert!(resolve_script_path(ws, "/etc/passwd").is_err()); + } + + #[test] + fn resolve_script_path_rejects_parent_dir() { + let ws = std::path::Path::new("/ws"); + assert!(resolve_script_path(ws, "../evil.js").is_err()); + assert!(resolve_script_path(ws, "scripts/../../evil.js").is_err()); + } + + #[test] + fn resolve_script_path_accepts_relative_subdir() { + let ws = std::path::Path::new("/ws"); + let resolved = resolve_script_path(ws, "scripts/run.js").unwrap(); + assert_eq!(resolved, std::path::Path::new("/ws/scripts/run.js")); + } +} diff --git a/src/openhuman/tools/impl/system/npm_exec.rs b/src/openhuman/tools/impl/system/npm_exec.rs new file mode 100644 index 000000000..c4986e0fd --- /dev/null +++ b/src/openhuman/tools/impl/system/npm_exec.rs @@ -0,0 +1,363 @@ +//! `npm_exec` — invoke the npm CLI through the managed (or system) Node.js +//! toolchain. +//! +//! Thin wrapper over `npm ` that piggybacks on +//! [`crate::openhuman::node_runtime::NodeBootstrap`] for binary resolution. +//! Same security posture as +//! [`crate::openhuman::tools::impl::system::shell::ShellTool`] and +//! [`crate::openhuman::tools::impl::system::node_exec::NodeExecTool`]: +//! +//! * Host env is cleared before spawning; only functional vars (`HOME`, +//! `TERM`, `LANG`, …) are forwarded. +//! * `PATH` is rebuilt with the resolved bin dir prepended so `npm`'s own +//! `node`/`corepack` lookups hit the managed toolchain first. +//! * Rate limits + action budget tracking piggyback on `SecurityPolicy`. +//! +//! The `subcommand` parameter is required and cannot contain shell +//! metacharacters (guarded server-side). Free-form args go through +//! POSIX-safe single-quoting. + +use crate::openhuman::agent::host_runtime::RuntimeAdapter; +use crate::openhuman::node_runtime::NodeBootstrap; +use crate::openhuman::security::SecurityPolicy; +use crate::openhuman::tools::traits::{Tool, ToolResult}; +use async_trait::async_trait; +use serde_json::json; +use std::sync::Arc; +use std::time::Duration; + +/// Default wall-clock budget for an npm invocation. `npm install` on a cold +/// cache can legitimately take several minutes on slow networks. +const NPM_TIMEOUT_SECS: u64 = 600; +/// Absolute ceiling callers can request via `timeout_secs`. +const NPM_TIMEOUT_MAX_SECS: u64 = 1800; +/// Output cap per stream (1 MB). +const MAX_OUTPUT_BYTES: usize = 1_048_576; +/// Env allow-list — matches the shell / node_exec tools. +const SAFE_ENV_VARS: &[&str] = &[ + "HOME", "TERM", "LANG", "LC_ALL", "LC_CTYPE", "USER", "SHELL", "TMPDIR", +]; + +/// Subcommands we outright refuse to run. These either break the managed +/// cache (`uninstall` of tooling bundled with the install) or perform +/// write actions outside the workspace (`publish` to a registry, `adduser` +/// / `login` / `logout` which mutate `~/.npmrc`). +const DISALLOWED_SUBCOMMANDS: &[&str] = &[ + "publish", + "unpublish", + "adduser", + "login", + "logout", + "token", + "star", + "unstar", + "owner", + "access", + "team", + "hook", + "profile", +]; + +/// `npm_exec` — run npm subcommands (install, run, ci, test, …). +pub struct NpmExecTool { + security: Arc, + runtime: Arc, + bootstrap: Arc, +} + +impl NpmExecTool { + pub fn new( + security: Arc, + runtime: Arc, + bootstrap: Arc, + ) -> Self { + Self { + security, + runtime, + bootstrap, + } + } +} + +#[async_trait] +impl Tool for NpmExecTool { + fn name(&self) -> &str { + "npm_exec" + } + + fn description(&self) -> &str { + "Run an npm subcommand (install, ci, run, test, exec, …) in the workspace. Dangerous registry/auth commands (publish, login, adduser, token, …) are blocked." + } + + fn parameters_schema(&self) -> serde_json::Value { + json!({ + "type": "object", + "properties": { + "subcommand": { + "type": "string", + "description": "npm subcommand, e.g. `install`, `ci`, `run`, `test`, `exec`." + }, + "args": { + "type": "array", + "items": { "type": "string" }, + "description": "Arguments appended after the subcommand (e.g. [\"build\"] for `npm run build`)." + }, + "cwd": { + "type": "string", + "description": "Optional sub-directory (relative to workspace) to run npm in. Defaults to the workspace root." + }, + "timeout_secs": { + "type": "integer", + "description": "Optional override for the default 600s timeout. Capped at 1800s." + } + }, + "required": ["subcommand"] + }) + } + + async fn execute(&self, args: serde_json::Value) -> anyhow::Result { + let subcommand = match args.get("subcommand").and_then(|v| v.as_str()) { + Some(s) => s.trim().to_string(), + None => { + return Ok(ToolResult::error( + "npm_exec requires a `subcommand` (e.g. install, ci, run).", + )); + } + }; + if subcommand.is_empty() { + return Ok(ToolResult::error("npm_exec `subcommand` cannot be empty")); + } + if !is_sane_subcommand(&subcommand) { + return Ok(ToolResult::error(format!( + "npm_exec rejected subcommand {subcommand:?}: only alphanumeric/._- characters allowed" + ))); + } + if DISALLOWED_SUBCOMMANDS + .iter() + .any(|d| d.eq_ignore_ascii_case(&subcommand)) + { + return Ok(ToolResult::error(format!( + "npm_exec refuses to run `npm {subcommand}` — registry/auth mutations are blocked" + ))); + } + + let extra_args: Vec = args + .get("args") + .and_then(|v| v.as_array()) + .map(|arr| { + arr.iter() + .filter_map(|v| v.as_str().map(str::to_string)) + .collect() + }) + .unwrap_or_default(); + + let cwd_override = args.get("cwd").and_then(|v| v.as_str()).map(str::to_string); + + let timeout_secs = args + .get("timeout_secs") + .and_then(|v| v.as_u64()) + .unwrap_or(NPM_TIMEOUT_SECS) + .min(NPM_TIMEOUT_MAX_SECS); + + if self.security.is_rate_limited() { + return Ok(ToolResult::error( + "Rate limit exceeded: too many actions in the last hour", + )); + } + if !self.security.record_action() { + return Ok(ToolResult::error( + "Rate limit exceeded: action budget exhausted", + )); + } + + let cwd = match resolve_cwd(&self.security.workspace_dir, cwd_override.as_deref()) { + Ok(p) => p, + Err(msg) => return Ok(ToolResult::error(msg)), + }; + + let resolved = match self.bootstrap.resolve().await { + Ok(r) => r, + Err(e) => { + tracing::error!(error = %e, "[npm_exec] failed to resolve node runtime"); + return Ok(ToolResult::error(format!( + "Node.js runtime unavailable: {e}" + ))); + } + }; + + tracing::info!( + version = %resolved.version, + source = ?resolved.source, + npm_bin = %resolved.npm_bin.display(), + subcommand = %subcommand, + "[npm_exec] starting invocation" + ); + + let mut parts: Vec = Vec::with_capacity(extra_args.len() + 2); + parts.push(shell_quote(&resolved.npm_bin.to_string_lossy())); + parts.push(shell_quote(&subcommand)); + for a in &extra_args { + parts.push(shell_quote(a)); + } + let command = parts.join(" "); + + let mut cmd = match self.runtime.build_shell_command(&command, &cwd) { + Ok(cmd) => cmd, + Err(e) => { + return Ok(ToolResult::error(format!( + "Failed to build runtime command: {e}" + ))); + } + }; + + cmd.env_clear(); + + let host_path = std::env::var("PATH").unwrap_or_default(); + let sep = if cfg!(windows) { ";" } else { ":" }; + let prepended_path = if host_path.is_empty() { + resolved.bin_dir.to_string_lossy().into_owned() + } else { + format!("{}{}{}", resolved.bin_dir.display(), sep, host_path) + }; + cmd.env("PATH", &prepended_path); + + for var in SAFE_ENV_VARS { + if let Ok(val) = std::env::var(var) { + cmd.env(var, val); + } + } + + let result = tokio::time::timeout(Duration::from_secs(timeout_secs), cmd.output()).await; + + match result { + Ok(Ok(output)) => { + let mut stdout = String::from_utf8_lossy(&output.stdout).to_string(); + let mut stderr = String::from_utf8_lossy(&output.stderr).to_string(); + + if stdout.len() > MAX_OUTPUT_BYTES { + stdout.truncate(stdout.floor_char_boundary(MAX_OUTPUT_BYTES)); + stdout.push_str("\n... [stdout truncated at 1MB]"); + } + if stderr.len() > MAX_OUTPUT_BYTES { + stderr.truncate(stderr.floor_char_boundary(MAX_OUTPUT_BYTES)); + stderr.push_str("\n... [stderr truncated at 1MB]"); + } + + if output.status.success() { + if stderr.is_empty() { + Ok(ToolResult::success(stdout)) + } else { + Ok(ToolResult::success(format!("{stdout}\n[stderr]\n{stderr}"))) + } + } else { + let err_msg = if stderr.is_empty() { stdout } else { stderr }; + Ok(ToolResult::error(err_msg)) + } + } + Ok(Err(e)) => Ok(ToolResult::error(format!("Failed to execute npm: {e}"))), + Err(_) => Ok(ToolResult::error(format!( + "npm_exec timed out after {timeout_secs}s and was killed" + ))), + } + } +} + +/// POSIX-safe single-quote escaping (mirrors the helper in `node_exec`). +/// Wraps `s` in `'…'`, turning any embedded single-quote into `'\''` so no +/// shell metacharacter can escape the quoted string. +fn shell_quote(s: &str) -> String { + let escaped = s.replace('\'', "'\\''"); + format!("'{escaped}'") +} + +/// Subcommands must be plain identifiers (`install`, `run`, `ci`, `exec`, +/// `test:watch`) — never a command substitution or redirection payload. +fn is_sane_subcommand(s: &str) -> bool { + !s.is_empty() + && s.chars() + .all(|c| c.is_ascii_alphanumeric() || matches!(c, '.' | '-' | '_' | ':')) +} + +/// Resolve an optional `cwd` override against the workspace. Rejects any +/// path that escapes the workspace via `..` or absolute components. +fn resolve_cwd( + workspace: &std::path::Path, + override_path: Option<&str>, +) -> Result { + match override_path { + None => Ok(workspace.to_path_buf()), + Some(raw) => { + let raw = raw.trim(); + if raw.is_empty() || raw == "." { + return Ok(workspace.to_path_buf()); + } + let candidate = std::path::Path::new(raw); + if candidate.is_absolute() { + return Err(format!( + "npm_exec `cwd` must be relative to workspace; got absolute path {raw:?}" + )); + } + if candidate.components().any(|c| { + matches!( + c, + std::path::Component::ParentDir | std::path::Component::Prefix(_) + ) + }) { + return Err(format!( + "npm_exec `cwd` must not escape workspace; got {raw:?}" + )); + } + Ok(workspace.join(candidate)) + } + } +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn is_sane_subcommand_accepts_common_npm_verbs() { + for v in &[ + "install", + "ci", + "run", + "exec", + "test", + "test:watch", + "run-script", + ] { + assert!(is_sane_subcommand(v), "{v} should be accepted"); + } + } + + #[test] + fn is_sane_subcommand_rejects_metacharacters() { + for v in &["install; rm -rf /", "run && echo", "|cat", "$(whoami)", ""] { + assert!(!is_sane_subcommand(v), "{v} should be rejected"); + } + } + + #[test] + fn resolve_cwd_defaults_to_workspace() { + let ws = std::path::Path::new("/tmp/ws"); + assert_eq!(resolve_cwd(ws, None).unwrap(), ws); + assert_eq!(resolve_cwd(ws, Some("")).unwrap(), ws); + assert_eq!(resolve_cwd(ws, Some(".")).unwrap(), ws); + } + + #[test] + fn resolve_cwd_rejects_absolute_and_parent() { + let ws = std::path::Path::new("/tmp/ws"); + assert!(resolve_cwd(ws, Some("/etc")).is_err()); + assert!(resolve_cwd(ws, Some("../other")).is_err()); + assert!(resolve_cwd(ws, Some("sub/../../../etc")).is_err()); + } + + #[test] + fn resolve_cwd_allows_relative_subdir() { + let ws = std::path::Path::new("/tmp/ws"); + let got = resolve_cwd(ws, Some("app")).unwrap(); + assert_eq!(got, std::path::PathBuf::from("/tmp/ws/app")); + } +} diff --git a/src/openhuman/tools/impl/system/shell.rs b/src/openhuman/tools/impl/system/shell.rs index f4ddc43d6..3d5715ba7 100644 --- a/src/openhuman/tools/impl/system/shell.rs +++ b/src/openhuman/tools/impl/system/shell.rs @@ -1,4 +1,5 @@ use crate::openhuman::agent::host_runtime::RuntimeAdapter; +use crate::openhuman::node_runtime::NodeBootstrap; use crate::openhuman::security::SecurityPolicy; use crate::openhuman::tools::traits::{Tool, ToolResult}; use async_trait::async_trait; @@ -20,11 +21,37 @@ const SAFE_ENV_VARS: &[&str] = &[ pub struct ShellTool { security: Arc, runtime: Arc, + /// Optional managed Node.js bootstrap. When provided **and** a prior + /// `NodeBootstrap::resolve()` has already succeeded, every shell invocation + /// transparently prepends the managed `bin/` dir to `PATH` — so skills + /// shelling out to `node`/`npm`/`npx`/`corepack` resolve to the managed + /// toolchain. Non-blocking: never triggers a download for unrelated + /// commands (we use `try_cached()`). + node_bootstrap: Option>, } impl ShellTool { pub fn new(security: Arc, runtime: Arc) -> Self { - Self { security, runtime } + Self { + security, + runtime, + node_bootstrap: None, + } + } + + /// Same as `new` but attaches a managed Node.js bootstrap for transparent + /// `PATH` injection. The bootstrap is consulted via `try_cached()` on each + /// invocation, so calling a non-node shell command never forces a download. + pub fn with_node_bootstrap( + security: Arc, + runtime: Arc, + bootstrap: Arc, + ) -> Self { + Self { + security, + runtime, + node_bootstrap: Some(bootstrap), + } } } @@ -107,6 +134,28 @@ impl Tool for ShellTool { } } + // If a managed Node.js install has already been resolved, transparently + // prepend its bin dir to PATH so this shell sees the managed toolchain. + // `try_cached()` never blocks and never triggers a download — unrelated + // commands (e.g. `ls`) stay fast and byte-identical to before. + if let Some(bootstrap) = self.node_bootstrap.as_ref() { + if let Some(resolved) = bootstrap.try_cached() { + let host_path = std::env::var("PATH").unwrap_or_default(); + let sep = if cfg!(windows) { ";" } else { ":" }; + let prepended = if host_path.is_empty() { + resolved.bin_dir.to_string_lossy().into_owned() + } else { + format!("{}{}{}", resolved.bin_dir.display(), sep, host_path) + }; + tracing::debug!( + bin_dir = %resolved.bin_dir.display(), + version = %resolved.version, + "[shell] prepending managed node bin to PATH" + ); + cmd.env("PATH", prepended); + } + } + let result = tokio::time::timeout(Duration::from_secs(SHELL_TIMEOUT_SECS), cmd.output()).await; diff --git a/src/openhuman/tools/ops.rs b/src/openhuman/tools/ops.rs index 1efcf7799..19679e03a 100644 --- a/src/openhuman/tools/ops.rs +++ b/src/openhuman/tools/ops.rs @@ -3,6 +3,7 @@ use super::*; use crate::openhuman::agent::host_runtime::{NativeRuntime, RuntimeAdapter}; use crate::openhuman::config::{Config, DelegateAgentConfig}; use crate::openhuman::memory::Memory; +use crate::openhuman::node_runtime::NodeBootstrap; use crate::openhuman::security::SecurityPolicy; use std::collections::HashMap; use std::sync::Arc; @@ -71,8 +72,40 @@ pub fn all_tools_with_runtime( fallback_api_key: Option<&str>, root_config: &crate::openhuman::config::Config, ) -> Vec> { + // Build a session-scoped managed Node.js bootstrap once, so ShellTool, + // NodeExecTool, and NpmExecTool all share the same memoised resolution + // state. Disabled when `node.enabled = false` — in that case shell skips + // PATH injection and node/npm tools are not registered. + let node_bootstrap: Option> = if root_config.node.enabled { + tracing::debug!( + version = %root_config.node.version, + prefer_system = root_config.node.prefer_system, + "[tools::ops] node runtime enabled — constructing shared NodeBootstrap" + ); + Some(Arc::new(NodeBootstrap::new( + root_config.node.clone(), + workspace_dir.to_path_buf(), + reqwest::Client::new(), + ))) + } else { + tracing::debug!( + "[tools::ops] node runtime disabled — shell PATH injection + node_exec/npm_exec suppressed" + ); + None + }; + + let shell: Box = if let Some(bootstrap) = node_bootstrap.as_ref() { + Box::new(ShellTool::with_node_bootstrap( + security.clone(), + Arc::clone(&runtime), + Arc::clone(bootstrap), + )) + } else { + Box::new(ShellTool::new(security.clone(), Arc::clone(&runtime))) + }; + let mut tools: Vec> = vec![ - Box::new(ShellTool::new(security.clone(), runtime)), + shell, Box::new(FileReadTool::new(security.clone())), Box::new(FileWriteTool::new(security.clone())), Box::new(CsvExportTool::new(security.clone())), @@ -156,6 +189,23 @@ pub fn all_tools_with_runtime( root_config.web_search.timeout_secs, ))); + // Managed Node.js exec tools — gated on `root_config.node.enabled`. + // Both share the same `NodeBootstrap` as ShellTool so the download + + // extract + install pipeline runs at most once per session. + if let Some(bootstrap) = node_bootstrap.as_ref() { + tools.push(Box::new(NodeExecTool::new( + security.clone(), + Arc::clone(&runtime), + Arc::clone(bootstrap), + ))); + tools.push(Box::new(NpmExecTool::new( + security.clone(), + Arc::clone(&runtime), + Arc::clone(bootstrap), + ))); + tracing::debug!("[tools::ops] registered node_exec + npm_exec"); + } + // Vision tools are always available tools.push(Box::new(ScreenshotTool::new(security.clone()))); tools.push(Box::new(ImageInfoTool::new(security.clone()))); @@ -670,6 +720,89 @@ mod tests { assert!(!names.contains(&"delegate")); } + #[test] + fn all_tools_registers_node_exec_when_node_enabled() { + // Default NodeConfig has `enabled = true`, so both `node_exec` and + // `npm_exec` must appear in the registry. Regression guard for the + // skills integration — if this fires, managed-node skills silently + // lose both tools. + let tmp = TempDir::new().unwrap(); + let security = Arc::new(SecurityPolicy::default()); + let mem_cfg = MemoryConfig { + backend: "markdown".into(), + ..MemoryConfig::default() + }; + let mem: Arc = + Arc::from(crate::openhuman::memory::create_memory(&mem_cfg, tmp.path(), None).unwrap()); + + let browser = BrowserConfig::default(); + let http = crate::openhuman::config::HttpRequestConfig::default(); + let cfg = test_config(&tmp); + + let tools = all_tools( + Arc::new(Config::default()), + &security, + mem, + None, + None, + &browser, + &http, + tmp.path(), + &HashMap::new(), + None, + &cfg, + ); + let names: Vec<&str> = tools.iter().map(|t| t.name()).collect(); + assert!( + names.contains(&"node_exec"), + "node_exec must be registered when node.enabled=true; got: {names:?}" + ); + assert!( + names.contains(&"npm_exec"), + "npm_exec must be registered when node.enabled=true; got: {names:?}" + ); + } + + #[test] + fn all_tools_excludes_node_exec_when_node_disabled() { + let tmp = TempDir::new().unwrap(); + let security = Arc::new(SecurityPolicy::default()); + let mem_cfg = MemoryConfig { + backend: "markdown".into(), + ..MemoryConfig::default() + }; + let mem: Arc = + Arc::from(crate::openhuman::memory::create_memory(&mem_cfg, tmp.path(), None).unwrap()); + + let browser = BrowserConfig::default(); + let http = crate::openhuman::config::HttpRequestConfig::default(); + let mut cfg = test_config(&tmp); + cfg.node.enabled = false; + + let tools = all_tools( + Arc::new(Config::default()), + &security, + mem, + None, + None, + &browser, + &http, + tmp.path(), + &HashMap::new(), + None, + &cfg, + ); + let names: Vec<&str> = tools.iter().map(|t| t.name()).collect(); + assert!( + !names.contains(&"node_exec"), + "node_exec must NOT be registered when node.enabled=false; got: {names:?}" + ); + assert!( + !names.contains(&"npm_exec"), + "npm_exec must NOT be registered when node.enabled=false; got: {names:?}" + ); + } + #[test] fn all_tools_excludes_computer_control_when_disabled() { let tmp = TempDir::new().unwrap();