4 Commits

Author	SHA1	Message	Date
Huixin615	65afc9b1d2	fix(skills): apply allowed-tools only to active skills (#4098) ... * fix(skills): scope allowed-tools to active skills * fix(skills): tolerate stale active skill paths * chore: retrigger CI * fix(skills): document policy activation limits * perf(skills): reuse per-step tool policy decisions * fix(skills): harden runtime tool policy contracts * fix(skills): redact cached policy decisions * fix(skills): make slash tool policy authoritative * fix(skills): preserve policy-safe discovery tools * test(skills): cover explicit task delegation policy	2026-07-16 14:12:02 +08:00
Daoyuan Li	2fa0505070	fix(skills): activate a slash skill once per run, not per model call (#4103) ... * fix(skills): activate a slash skill once per run, not per model call SkillActivationMiddleware injects the activation reminder for a slash command via request.override(messages=...), which LangChain's create_agent uses for a single model call and never writes back to graph state. The dedup guard scans request.messages for a prior reminder, but model_node rebuilds request.messages fresh from persisted state on every tool-loop step, so the reminder is never present on the 2nd..Nth model call of a turn. Every model call therefore re-parsed the command, re-read SKILL.md from disk, re-injected the multi-KB body, and re-recorded an "activate" audit event, despite the code intending a single activation per run (#3861 semantics: one activation call, many follow-up model calls). Key the dedup off the run context instead, which LangGraph threads through every model-node call of a run (the same durable signal the request-scoped secret source already uses). The activation call records the slash message's identity in context; later calls for the same message skip re-activation. A new user slash message keys differently and still activates. Secret binding is unaffected: it already re-resolves from the persisted slash source on every call. Adds regression tests that rebuild the real multi-call turn state and assert a single activation across the tool loop, plus a test proving a new slash command still activates. * fix(skills): address review nits on run-scoped activation dedup - Extract _already_activated(run_context, run_key) so the dedup check mirrors the existing _has_existing_activation_for_target sibling instead of an inline dense conditional. - Compute _activation_run_key() once in _find_activation_target and thread it through _prepare_model_request instead of recomputing it at the write site, making the "same key for check and write" invariant explicit in the code rather than implicit. - Document why the run-context write is an overwrite rather than an append/set: only the latest real user message is ever considered an activation target, so there is nothing earlier in the run worth preserving. - Add a regression test locking in the degraded-path contract: when runtime.context is None, the middleware still activates per-call instead of crashing or wrongly no-op'ing.	2026-07-13 18:38:32 +08:00
Xinmin Zeng	4d660b202a	feat(skills): bind request-scoped secrets for autonomously-invoked skills (A+) (#3938) ... * feat(skills): bind request-scoped secrets for in-context (autonomously invoked) skills Extends the #3861 binding point A (slash-activation only) to A+: the injection set is recomputed on every model call from two unioned sources — the run's most recent slash activation (persisted on the run context so the tool loop keeps the binding) and skills the model actually loaded in this thread (ThreadState.skill_context), re-validated against the live registry each call. Authorization stays three-gated regardless of activation style: skill enabled by the operator, values supplied per-request by the caller in context.secrets (never persisted server-side, never from the host env), names declared in the skill's required-secrets frontmatter. Because the set is replaced per call, eviction from skill_context or a caller that stops supplying a value revokes injection on the next call. New frontmatter field secrets-autonomous (default true) lets a skill restrict binding to explicit slash activation; malformed values fail closed to false. Binding changes are recorded as a middleware:skill_secrets journal event carrying names only. Design informed by a survey of peer systems (Claude Code, Codex CLI, opencode, pi, deepagents, hermes-agent, QwenPaw) and specs (agentskills.io, MCP 2025-11-25): the industry trust boundary is enable-time consent plus caller-scoped credentials, not per-invocation ceremony; no surveyed system scopes secrets to an activation turn. Part of #3914 * refactor(skills): centralize secret context keys, document intentional per-call reload Review follow-ups (no behavior change): move the two private binding keys (__slash_skill_secret_source, __skill_secrets_binding_audit) into secret_context.py and add them to REDACTED_CONTEXT_KEYS so the redaction allowlist stays a complete guard even though both keys hold names only. Document why _in_context_secret_sources reloads skills every call rather than caching: load_skills re-reads enabled state so an operator disabling a skill revokes its binding on the next model call — an mtime cache would miss enable/disable toggles and keep injecting after a disable. * fix(skills): match in-context secret bindings by path only, never by name Review finding (confused deputy): _in_context_secret_sources fell back to name matching when a skill_context path did not resolve. DeerFlow lets a custom skill shadow a same-named public/legacy one (load_skills de-dupes by name, custom wins), so a thread that read public/foo could bind the custom foo's declared secrets although the custom skill was never loaded in the thread. The recent user-isolation path changes make by-path misses (and thus the dangerous fallback) more likely. Drop the by-name fallback: match strictly by the exact container file path the model read; an unresolved path simply does not bind (the safe direction). Regression tests cover the shadowing case and a stale path. Part of #3914 * fix(skills): resolve secret-binding sources via registry; strip caller __-keys Security review (willem-bd, #3938): 1. Forged `__slash_skill_secret_source` bypassed the enabled/allowlist/ secrets-autonomous gates. runtime.context is caller-mergeable, and the slash source was trusted as authoritative (its stored requirements were injected directly). Now the slash source records only the activated skill's canonical container path, and BOTH the slash and in-context sources resolve the live registry skill by normalized path each call (_resolve_registry_skill) — binding only that real, enabled, allowlisted skill's own declared secrets. A forged path resolves to nothing. As defense in depth, build_run_config strips caller-supplied __-prefixed context keys at the gateway boundary. 2. Malformed caller requirements crashed the run (unguarded tuple unpack / DoS). The middleware no longer unpacks caller-provided requirement data at all — declarations come from the registry — so a malformed source fails closed instead of raising. 3. Path-normalization asymmetry silently disabled in-context binding on a trailing-slash container_path config. Both the registry keys and the lookup path are now posixpath.normpath'd. Regression tests: forged source rejected, forged-but-real path ignores caller requirements + allowlist, malformed source fails closed, trailing- slash config binds, gateway strips __-keys. Part of #3914 * docs(skills): correct _SLASH_SECRET_SOURCE_KEY comment and note fail-closed trade-off Post-review cleanup: the key now stores only the canonical container path (the comment still described the pre-fix skill-name+requirements shape), and document that a transient registry-load failure fails closed (drops the binding for that call) rather than trusting stale data. --------- Co-authored-by: Willem Jiang <willem.jiang@gmail.com>	2026-07-04 23:34:32 +08:00
Xinmin Zeng	09988caf95	feat(skills): request-scoped secrets for skills (closes #3861) (#3871) ... * feat(sandbox): per-call env injection + platform-secret scrubbing for skills Add an env parameter to Sandbox.execute_command (abstract + local + AIO) so request-scoped secrets can be injected into skill subprocesses, and scrub platform credentials (*KEY*/*SECRET*/*TOKEN*/*PASSWORD*/*CREDENTIAL*) from the inherited environment by default so scoped injection is not security theatre. LocalSandbox always passes an explicit scrubbed env; AioSandbox routes env-bearing commands through bash.exec(env=) on a fresh session and leaves the legacy persistent-shell path unchanged. Part of #3861. BEHAVIOR CHANGE: execute_command no longer inherits the full os.environ; Windows encoding tests updated to assert the scrubbed dict. * feat(skills): parse required-secrets frontmatter declaration Add SecretRequirement and Skill.required_secrets, and parse the required-secrets SKILL.md frontmatter field (a string list or {name, optional} mappings), dropping malformed entries with a warning so one bad declaration does not invalidate the skill. The declared name is both the context.secrets key and the env var injected at activation. Part of #3861. * feat(runtime): request-scoped secret carrier (context.secrets) Add SECRETS_CONTEXT_KEY + extract_request_secrets, centralising the context.secrets carrier contract. The existing context passthrough (build_run_config -> _build_runtime_context) already carries the sub-key to runtime.context without mirroring it into configurable; characterization tests lock that behaviour. Part of #3861. * feat(skills): inject declared secrets at slash-activation into bash env Binding point A: when a skill is slash-activated, SkillActivationMiddleware resolves its declared required-secrets against the request's context.secrets and writes the per-run injection set to runtime.context. The bash tool forwards that set to execute_command(env=). A skill cannot harvest a host platform credential (is_host_platform_secret guard, cf. GHSA-rhgp-j443-p4rf), and injected values are redacted from bash output (mask_secret_values) so an echoed secret never re-enters the prompt/trace. Part of #3861. * test(skills): lock the five secret leak surfaces + add trace redaction helper Regression tests assert the secret value is absent from all five surfaces: prompt (activation message), checkpoint (graph state vs context separation), audit (journal records names only), trace (metadata builder never copies context; never mirrored to configurable), and stdout (mask_secret_values). Add redact_secret_context_keys as a defensive helper for any context serialization. Part of #3861. * docs(backend): document request-scoped secrets for skills Add Request-Scoped Secrets subsection (Skills) + env policy note (Sandbox) and the execute_command(env=) signature change, per the doc-sync policy. Part of #3861. * fix(skills): close gaps found by end-to-end verification of request-scoped secrets Real-gateway e2e + independent review of #3861 surfaced three defects, now fixed: 1. Slash activation never fired in the live chain. InputSanitizationMiddleware wraps user input in BEGIN/END markers before SkillActivationMiddleware sees it, and the original text was only preserved when an upload or IM channel set it. For a plain text message the slash command became undetectable, so no secret was ever resolved. Fix: the sanitizer now setdefaults the pre-wrap text into ORIGINAL_USER_CONTENT_KEY (additive; sanitization behaviour unchanged), so slash activation works for all messages. Pre-existing latent bug surfaced here. 2. The raw request config (with context.secrets) was persisted to runs.kwargs_json and echoed by the run API (RunResponse.kwargs). Fix: redact_config_secrets() strips secret-bearing context keys from the persisted/echoed copy in start_run; the live config that drives the run keeps them. build_run_config now also sets configurable.thread_id on the context path (the checkpointer requires it). 3. Connection-string credentials (DATABASE_URL, REDIS_URL, SENTRY_DSN, GH_PAT, ...) were not scrubbed from the inherited sandbox env. Fix: env_policy adds a *DSN* pattern plus an explicit connection-string denylist (no blanket *URL* — benign service URLs stay readable). Verified end-to-end via a real gateway run (real LLM + skill activation + bash): the secret reaches the sandbox subprocess and appears in NONE of prompt, trace, checkpoint, audit, stdout, runs.kwargs_json, or the run API. Part of #3861. * docs(backend): document the env scrub, persistence redaction, and sanitizer interaction Sync the Request-Scoped Secrets section with the verification-driven fixes: inherited-env scrub (incl. connection-string denylist), run-record/run-API redaction as the 6th sealed leak surface, and the sanitizer preserving original content so slash activation fires. Part of #3861. * fix(skills): inject caller secret over scrubbed host value; drop redundant host-name guard A real-world demo (a skill calling a third-party cloud API with a request-scoped key) exposed that the is_host_platform_secret guard was both wrong and harmful: it refused to inject a caller-supplied secret whenever a same-named variable existed in the Gateway env — which is exactly the #3861 use case (a per-user key overriding a shared platform key). The guard was also redundant: build_sandbox_env already scrubs secret-looking names from the inherited env before injection, so a skill can never read a host credential — it only ever receives the caller's value. Remove the guard; the injected (caller) value simply wins over the scrubbed host value. Verified end-to-end: the agent called the real cloud API successfully with the caller's key, the host's same-named key was scrubbed and never used, and the caller's key leaked to none of the surfaces. Part of #3861. * fix(skills): address review on request-scoped secrets (#3861) Review fixes from PR #3871: - E2BSandbox.execute_command now accepts env/timeout and routes them to commands.run(envs=, timeout=). The bash tool passes env= unconditionally, so the prior signature (command only) raised TypeError on every e2b bash call and broke e2b deployments entirely. env=None stays backward-compatible. - SkillActivationMiddleware clears the active-secret set before resolving each activation, so a later skill in the same run never inherits an earlier skill's injection set (the #3861 contract: a skill only receives what the caller supplied AND that skill declared). - AioSandbox env path uses a dedicated _DEFAULT_HARD_TIMEOUT — bash.exec exposes no idle/no-change timeout, so the prior reuse of the legacy idle constant conflated wall-clock vs idle semantics. The env path also retries on the ErrorObservation signature now, sharing the legacy persistent-shell recovery contract. - mask_secret_values skips values below a minimum length floor so a short declared secret (e.g. "42") cannot shred unrelated bytes (exit codes, timestamps, sizes) of tool output. The secret is still injected into the subprocess; only the output mask skips it. session_id reuse on the env path is intentionally NOT added: a shared session could let request-scoped secrets ride the session env into later commands, which the SDK does not contractually forbid. The fresh-session choice matches the LocalSandbox model (each call is a fresh subprocess); the trade-off (consecutive env-bearing calls do not share cwd/venv/exports) is documented on _execute_with_env.	2026-07-03 07:51:22 +08:00