test(skills): unit tests for resolver + serialize env-mutation test; docs

neo-wanderer · neo-wanderer · commit 1dfb44e3c980 · 2026-04-26T13:32:37.000Z
Test coverage for the env_passthrough resolver (5 unit tests): - Forbidden vars (LD_PRELOAD, PYTHONPATH) are blocked. - Kernel-reserved vars (PATH, HOME, …) are blocked. - Forbidden match is case-insensitive. - Operator deny patterns (AWS_*, *_KEY) work. - Per-skill override unblocks denied vars only for the named skill, and cannot bypass the FORBIDDEN_PASSTHROUGH hard block. Mark the existing env-mutating integration test #[serial_test::serial(skill_env_passthrough)] (review #4): it calls std::env::set_var/remove_var which mutates process-global state and would race with any other env-touching test under the parallel default of cargo test. The serial harness is already used by hands/ extensions for the same reason. Docs (review #5): add an Environment Variable Passthrough section to docs/src/app/agent/skills/page.mdx covering the two-party opt-in model (skill author declares; operator gates), the resolution order, the FORBIDDEN list, and a worked gog example. Calls out the distinction from skill config variables — credentials should go through config, not env.
diff --git a/Cargo.lock b/Cargo.lock
diff --git a/crates/librefang-skills/Cargo.toml b/crates/librefang-skills/Cargo.toml
@@ -29,3 +29,4 @@ fs2 = "0.4"
 
 [dev-dependencies]
 tempfile = { workspace = true }
+serial_test = "3"
diff --git a/docs/src/app/agent/skills/page.mdx b/docs/src/app/agent/skills/page.mdx
@@ -221,6 +221,73 @@ db.host = postgres.internal
 
 ---
 
+## Environment Variable Passthrough
+
+Skill subprocesses run with `env_clear()` by default — no host environment variables are inherited. This is the right default for third-party code: API keys, tokens, and other secrets in the host environment must not silently leak into a skill's subprocess.
+
+Some skills legitimately need a specific host variable. The canonical example is a skill that wraps a CLI tool which uses an env-based credential helper (e.g. `gog`'s file-backed keyring needs `GOG_KEYRING_PASSWORD`).
+
+This works as a **two-party opt-in**: the skill author declares which variables the skill *wants*, and the operator (the person running LibreFang) decides which of those requests to *grant*.
+
+### Skill author: declare in `skill.toml`
+
+Add `env_passthrough` at the top level of the manifest, sibling to `[skill]` and `[runtime]`:
+
+```toml
+env_passthrough = ["GOG_KEYRING_PASSWORD", "GOG_KEYRING_BACKEND"]
+
+[skill]
+name = "gog"
+# …
+```
+
+The variable *names* are public (they live in the manifest); only their host-side *values* cross the subprocess boundary, and only when the operator has not blocked the name.
+
+### Operator: gate via `[skills]` config
+
+The operator's config in `~/.librefang/config.toml` decides which requests are honored:
+
+```toml
+[skills]
+# Glob patterns that block matching env-var names regardless of what the
+# skill manifest declares. These are the defaults; replace with your own
+# list, or set to [] to disable the deny check.
+env_passthrough_denied_patterns = [
+    "*_KEY",
+    "*_TOKEN",
+    "*_PASSWORD",
+    "*_SECRET",
+    "*_API_KEY",
+    "AWS_*",
+    "GITHUB_*",
+]
+
+# Per-skill explicit allow overrides. Lets you grant a specific skill an
+# env var that would otherwise be blocked by env_passthrough_denied_patterns.
+[skills.env_passthrough_per_skill]
+gog = ["GOG_KEYRING_PASSWORD"]
+```
+
+If you don't configure `[skills]` at all, the defaults above apply.
+
+### Resolution
+
+For each variable name in a skill's `env_passthrough`, in order:
+
+1. **Hard block** — names like `LD_PRELOAD`, `PYTHONPATH`, `NODE_OPTIONS`, etc. are dropped regardless of skill manifest or operator config. These either inject code or redirect imports/library lookup, and would defeat the `env_clear` isolation. The full list is in `librefang-skills::loader::FORBIDDEN_PASSTHROUGH`.
+2. **Kernel-reserved** — `PATH`, `HOME`, `PYTHONIOENCODING`, etc. are dropped. The kernel sets these explicitly per-runtime (it may have deliberately narrowed `PATH`); skills cannot override them.
+3. **Operator deny** — names matching `env_passthrough_denied_patterns` are dropped *unless* listed under `env_passthrough_per_skill` for the running skill.
+4. Anything that survives is forwarded if it's set in the host environment. Variables not present in the host environment are silently skipped.
+
+Each rejection is logged at `WARN` level so operators can debug why a declared variable did not reach a skill subprocess.
+
+### When to use it
+
+- **Use `env_passthrough`** when a skill calls out to a CLI that authenticates via env-based credential helpers (keyring backends, `*_PASSWORD` vars, etc.).
+- **Don't use `env_passthrough` for API keys/tokens.** Use [Skill Config Variables](#skill-config-variables) instead — those go through `~/.librefang/config.toml` and are injected via the system prompt without giving the skill subprocess access to host secrets.
+
+---
+
 ## Python Skills
 
 Python skills are the simplest to write. They run as subprocesses and communicate via JSON over stdin/stdout.

Original file line number	Diff line number	Diff line change
`@@ -29,3 +29,4 @@ fs2 = "0.4"`
`29`	`29`
`30`	`30`	`[dev-dependencies]`
`31`	`31`	`tempfile = { workspace = true }`
	`32`	`+serial_test = "3"`