mirror of
https://github.com/bytedance/deer-flow.git
synced 2026-10-10 05:16:28 +00:00
70 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
kbkb628
|
5202068a0e
|
fix(skillscan): read Python secret assignments from the AST (#5648)
* fix(skillscan): read Python secret assignments from the AST
The `secret-env-assignment` rule swept every text file with a
`name[:=]value` regex, which misreads Python syntax in two ways:
- `def __init__(self, token: Optional[str] = None):` — the captured
"value" is a type annotation, not embedded secret material.
- `api_key = os.getenv("MINIMAX_API_KEY")` — reading a secret from the
environment is this rule's own documented remediation, yet it was
reported as a hardcoded credential.
Both are HIGH severity, so they map to a review `error` and fail the
Skill Review gate. Two bundled public skills therefore failed CI on an
unchanged checkout:
- skills/public/github-deep-research/scripts/github_api.py:56
- skills/public/music-generation/scripts/generate.py:27
Python sources now go through the AST instead of the line-oriented
sweep, keeping only real literal values. The text sweep is unchanged for
config, shell, YAML, and Markdown. Annotated assignments are still
reported, and now at the literal rather than at the annotation.
Tests: `secret-env-assignment` previously had no coverage anywhere in
backend/tests. Added six tests, including a regression test that scans
every bundled public skill script. Verified red on main and green here.
* fix(skillscan): keep text coverage for unparseable Python
Reviewer feedback on #5648: when `ast.parse` failed, the rule returned no
findings at all. One syntax error -- or a NUL byte, which `ast.parse` rejects
with the same exception -- therefore silenced the HIGH-severity
`secret-env-assignment` rule for the whole file, where `main` still swept the
raw text and reported it. For a review-gate rule that is a trivial evasion.
The line-oriented sweep moves into `_scan_secret_assignments_by_text`, which
the non-Python path now calls and which `_scan_python_secret_assignments` falls
back to when the file will not parse. Parseable files keep the precise AST
semantics this change introduces; unparseable ones keep main-level coverage
instead of losing the rule entirely.
Tests: both fallback paths added (syntax error, NUL byte); both fail before this
commit and pass after.
|
||
|
Undermoon1412
|
aa7f616734
|
fix(composer): reserve context slash command alias (#5279)
* fix(composer): reserve context slash command alias * fix(skills): align slash docs and formatting Signed-off-by: Undermoon1412 <80385295+Undermoon1412@users.noreply.github.com> * fix(skills): allow context skill outside compact alias Signed-off-by: Undermoon1412 <80385295+Undermoon1412@users.noreply.github.com> * docs(tui): sync context skill command policy Signed-off-by: Undermoon1412 <80385295+Undermoon1412@users.noreply.github.com> --------- Signed-off-by: Undermoon1412 <80385295+Undermoon1412@users.noreply.github.com> |
||
|
PeaceMaker-best
|
e831720304
|
feat(skills): rank deferred discovery by agent intent (#5369)
* feat(skills): rank deferred discovery by intent * fix(skills): preserve exact selections and cache search metadata --------- Co-authored-by: PeaceMaker-best <221849497+PeaceMaker-best@users.noreply.github.com> Co-authored-by: Willem Jiang <willem.jiang@gmail.com> |
||
|
Hyeonsang Cho
|
6469833886
|
fix(skills): close SkillScan bypasses in the skill review gate (#5431)
* fix(skills): close SkillScan bypasses in the skill review gate The public skill review gate re-materialized a package snapshot into a temp directory for SkillScan, but copied only entries the reader had decoded as text and skipped every file under any evals/fixtures/ directory. Executable binaries and nested archives never reached the package rules, and a fixture-shaped path hid any script from the scan. Readers now keep binary bytes as content_base64, the analyzer writes every non-symlink file byte for byte, and only eval fixture SKILL.md samples stay exempt. Files are created exclusively, so a duplicate archive member or a case-folded name fails the scan closed instead of overwriting an earlier file. SkillScan itself skipped any file that was not NUL-free UTF-8. One Latin-1 byte in a comment hid a reverse shell from the review gate, and a NUL byte skipped static analysis at install. Code files that fail strict decoding now raise package-undecodable-script (HIGH) and are analyzed over a lossy decode, so CRITICAL matches keep blocking. "Code file" and "executable magic" were defined separately in the installer and SkillScan and had drifted: SkillScan missed 32-bit little-endian and fat Mach-O variants the installer blocks. Both rules now live in skills/package_files.py, shared by the installer, the export guard, and SkillScan. * docs(changelog): link the skill review gate fix to #5431 * fix(skills): fail closed on bytes-less snapshot entries and skip text rules for executables The review analyzer skipped any snapshot entry it could not turn into bytes. Readers only emit such entries for oversized files, and they also mark the snapshot truncated, but content_base64 is optional in the contract, so a reader regression or a hand-built snapshot would silently drop a file from SkillScan. An entry without bytes now fails the scan closed (not_assessed: skillscan) unless the snapshot is truncated, and a text entry without content no longer materializes as an empty file. A real executable under scripts/ is a code file, so SkillScan decoded it lossily and ran the text rules over its string tables. An OpenSSH binary produced a CRITICAL secret-private-key finding from the key-format banner it embeds. An undecodable file with executable magic still reports package-undecodable-script, and its CRITICAL package-executable-binary finding already blocks it, so it now skips the text rules. Decodable files keep full text analysis. --------- Co-authored-by: Willem Jiang <willem.jiang@gmail.com> |
||
|
Hyeonsang Cho
|
bc4a33aba7
|
fix(skills): stop persisting resolved secrets when toggling skills (#5357)
Toggling a skill wrote resolved secrets into extensions_config.json. The Gateway skill toggle and DeerFlowClient.update_skill loaded the file with ExtensionsConfig.from_file(), which replaces every "$VAR" string with the environment value (and an unset variable with ""), then serialized that model back through to_file_dict(). A "$GITHUB_TOKEN" reference was persisted as the plaintext token, and an unset reference was erased for good. DeerFlowClient.update_mcp_config had the same flaw for every key other than mcpServers. Every writer now does a raw read-modify-write, the way the MCP router already did: read_raw_extensions_config reads the on-disk JSON, set_raw_skill_enabled changes only the target entry, and validate_raw_extensions_config checks the candidate the way the runtime will load it before the atomic write. When the file does not exist yet, the Gateway seeds it with the cached skill states only, never the resolved cached model. The MCP router's raw loader and candidate validation delegate to the same helpers, so the rule lives in one place, and to_file_dict() is removed so the unsafe serialization has no entry point left. Co-authored-by: Willem Jiang <willem.jiang@gmail.com> |
||
|
Ryker_Feng
|
f52818fe5e
|
feat(skills): export custom skill packages with revision-bound preview (#5332)
* feat(skills): export custom skill packages with revision preview * docs(gateway): keep export guidance within size budget * ci: retry checks after transient uv setup download failure * docs: focus skill export agent guidance on maintenance invariants * fix(skills): handle export disconnects and bound archive transfers * docs(gateway): remove redundant export guidance to fit merged budget * fix(skills): reset export idle deadline after transfer progress |
||
|
PeaceMaker-best
|
e3df6ea4a8
|
feat(channels): select custom agents per conversation (#5168)
* feat(channels): select custom agents per conversation Signed-off-by: PeaceMaker-best <221849497+PeaceMaker-best@users.noreply.github.com> * fix(channels): reserve agent slash command across clients Signed-off-by: PeaceMaker-best <221849497+PeaceMaker-best@users.noreply.github.com> * fix(tui): hide reserved slash commands from skills Signed-off-by: PeaceMaker-best <221849497+PeaceMaker-best@users.noreply.github.com> * fix(channels): preserve selected agent across clients --------- Signed-off-by: PeaceMaker-best <221849497+PeaceMaker-best@users.noreply.github.com> Co-authored-by: PeaceMaker-best <221849497+PeaceMaker-best@users.noreply.github.com> |
||
|
Madan kumar
|
df68b59149
|
fix(skills): reject a blank SKILL.md description at the write gate (#4867)
* fix(skills): reject a blank SKILL.md description at the write gate
_validate_skill_frontmatter only applied its description rules when the
value was truthy, so a blank or whitespace-only description passed the
gate. The loader rejects it, so the file was written to disk and then
disappeared from every consumer.
On the PUT edit endpoint that meant the write committed first and the
response was a 404 -- the previously working skill was gone with no
rollback. Same shape via the .skill install path and the agent-facing
skill_manage tool, which reported success for a skill that never loads.
Empty names were already rejected; description was the only field where
the write gate and the loader disagreed.
* test(skills): pin rollback rejection of a blank-description history entry
Add a regression test for the rollback path: restoring a history entry
whose stored content has an empty description must return 400
("Description cannot be empty") and leave the on-disk SKILL.md untouched,
rather than the previous destructive path that wrote the unloadable
content and then 404'd. On main this returns 404, so the test also pins
the intended status-code change on this path.
|
||
|
Aari
|
317577e285
|
fix: enforce custom agent skill allowlists in sandboxes (#5077)
* fix: enforce agent skill allowlists in sandboxes * fix: guard E2B skill projection resets * fix: preserve agent skill isolation across delegation * fix: close sandbox skill isolation bypasses * fix(sandbox): close skill isolation review gaps * fix(sandbox): harden skill isolation lifecycle |
||
|
georgelichen
|
24001e80b7
|
fix(skills): safely tokenize portable allowed-tools patterns (#4984)
* fix(skills): accept portable frontmatter forms * fix(skills): normalize portable tool names * Safely preserve parenthesized portable skill tool patterns Portable Agent Skills declarations such as Bash(tvly *) contain spaces inside a command pattern. Keep those patterns as single literal entries while preserving exact names from the existing YAML-list form, so skill loading no longer fragments valid metadata or rewrites mixed-case MCP tools. Constraint: DeerFlow's current skill policy matches exact tool names and does not inspect Bash arguments Constraint: Agent Skills scalar syntax uses whitespace-separated entries with parenthesized command patterns Rejected: raw.split() | fragments Bash(tvly *) into unrelated tool names Rejected: normalize YAML-list entries | breaks case-sensitive MCP/runtime tool names Rejected: map Bash(...) to bash | broadens command-scoped declarations into unrestricted shell access Confidence: high Scope-risk: narrow Reversibility: clean Directive: Keep Bash(...) entries literal and inactive until DeerFlow has an explicit command-pattern authorization model Tested: 175 focused parser, validation, installer, review, loader, and tool-policy tests; Ruff check and format; compileall; git diff --check Not-tested: Full backend suite stopped at pre-existing Windows mode assertion test_runtime_config_store_file_is_owner_only Related: #4912 * Preserve exact custom tool names in portable skill parsing Portable scalar frontmatter needs alias normalization for known DeerFlow-compatible names, but generic case conversion corrupts MCP and custom tool identifiers. The tokenizer also treated quoted or escaped parentheses as structural delimiters, rejecting valid command patterns. Preserve unknown names and parse quoted or escaped patterns without broadening Bash(...) into bash. Constraint: Runtime skill policy uses exact tool-name matching Constraint: Parenthesized patterns remain literal because argument-level authorization is not implemented Rejected: Generic CamelCase-to-snake_case for every scalar | rewrites custom/MCP names Rejected: Map Bash(...) to bash | broadens command-scoped declarations into unrestricted shell access Confidence: high Scope-risk: narrow Reversibility: clean Directive: Add an explicit alias before supporting another portable tool name; keep command-pattern authorization separate Tested: 225 skills tests passed, 1 skipped; Ruff check; Ruff format --check; compileall; git diff --check Not-tested: Full backend suite remains affected by unrelated Windows permissions/path and missing Lark CLI tests Related: #4984; #4912 * Preserve case-sensitive exact tool authorities Case-folding a scalar declaration before alias lookup can turn literal write into write_file, substituting a different runtime authority. Keep exact portable spellings as aliases and preserve lowercase, custom, and MCP names; strengthen activation coverage for spaced Bash patterns and command fragments. Constraint: Runtime skill policy uses exact tool-name matching Constraint: Bash(...) remains literal and inactive because command-pattern authorization is not implemented Rejected: Case-insensitive alias lookup | maps lowercase runtime tools onto built-in authorities Rejected: Broaden the parser into command-pattern authorization | outside this PR's scope Confidence: high Scope-risk: narrow Reversibility: clean Directive: Add aliases only for documented portable spellings; preserve all other scalar names verbatim Tested: 226 skills tests passed, 1 skipped; Ruff check; Ruff format --check; compileall; git diff --check Not-tested: Full backend suite remains affected by unrelated Windows permissions/path and missing Lark CLI tests; GitNexus index refresh remains stale Related: #4984; #5016297602 * Support portable Glob and Grep skill aliases Portable Agent Skills commonly declare Glob and Grep, but DeerFlow exposes the runtime tools as glob and grep. Add explicit exact-spelling aliases and activation coverage so imported skills retain search-tool access without broad normalization. Constraint: Runtime skill policy uses exact tool-name matching Constraint: Alias conversion is limited to documented portable spellings Rejected: Case-fold all scalar names | can substitute custom or MCP authorities Rejected: Map arbitrary names by convention | breaks exact runtime compatibility Confidence: high Scope-risk: narrow Reversibility: clean Directive: Keep the alias table explicit and preserve unknown scalar names verbatim Tested: 228 skills tests passed, 1 skipped; Ruff check; Ruff format --check; compileall; git diff --check Not-tested: Full backend suite has unrelated environment failures on Windows; GitNexus index reports stale line mappings Related: #4984; #5026257899 --------- Co-authored-by: kriptoburak <kriptoburak@users.noreply.github.com> |
||
|
青榆牧
|
a94b2d8897
|
feat(mcp): map request-scoped secrets to MCP HTTP/SSE headers (#5010)
* feat(mcp): map request-scoped secrets to HTTP/SSE headers `user_auth` binds a credential to a configured DeerFlow user, so a caller that picks the credential per request — a multi-tenant gateway, a per-run API key, one shared MCP server fronting several environments — had to register one MCP server entry per credential. Add a declarative `mcpServers.<server>.headers_from_context` block mapping HTTP header names to keys of the run request's `config.context.secrets` carrier. A new built-in interceptor resolves the mapping on every tool call and rewrites those headers, mirroring `user_scoped_auth`. The config file stores names only, never a credential, so the Gateway returns the block unmasked. Registered after OAuth and `user_auth` in the interceptor chain: the later interceptor runs closer to the transport, and the value chosen for this one request is the most specific, so it wins. Fail-closed by default — a mapped key missing from the request raises a `ToolException` naming only that key, because falling back to the server's discovery credential would send one tenant's call under another tenant's authority. `on_missing: "passthrough"` opts out. Durable background tasks are excluded: `McpTaskToolCaller` drives status and cancel polls after the Agent run ends, where no run context exists, so the fail-closed interceptor would deny every poll. Those calls keep using server-level credentials, and a server declaring both `headers_from_context` and `task_toolsets` now logs a warning. Also corrects the custom-interceptor example in docs/MCP_SERVER.md (and the matching claim in skills/AGENTS.md), which read request secrets from `langgraph.config.get_config()["context"]`. That key is `None` inside a tool call — the run context rides the LangGraph runtime, not the RunnableConfig propagated to child runnables — so interceptors written from that example never saw a value. The example now reads `request.runtime`, and tests/test_mcp_context_headers.py pins LangGraph's runtime-injection rule by driving a real langchain-mcp-adapters tool through a real graph with the ambient-runtime fallback disabled. Closes #5005 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(mcp): resolve credential headers case-insensitively, carry them on durable submit Review follow-ups on `headers_from_context`. HTTP field names are case-insensitive, but every dict on the path to the wire is not: `build_server_params` copies the operator's static `headers` spelling verbatim, and langchain-mcp-adapters merges interceptor overrides into the connection with a plain `{**connection_headers, **override_headers}` splat. A static `authorization` and an injected `Authorization` therefore both reached httpx as separate field lines, and a server reading the field with a single-value accessor got the static discovery credential — inverting the documented `headers` < `oauth` < `user_auth` < `headers_from_context` precedence and running a per-request call under the shared credential. Normalizing inside the interceptor cannot fix that on its own: the adapter builds the request with `headers=None`, so an interceptor never sees the connection's static headers and cannot displace them however it spells its own key. A new `mcp/headers.py::apply_header_overrides` therefore drops any key differing only in case and emits the spelling the connection already uses. Applied to `headers_from_context`, `user_auth`, the OAuth interceptor, the OAuth discovery-header write, and the durable-task connection merge, which all carried the same collision. `headers_from_context.headers` now also rejects one header mapped under two spellings at config load, in both the harness model and the Gateway mirror. Durable submit now carries the mapped headers, as docs/MCP_SERVER.md already promised. `McpTaskToolCaller` disabled the interceptor for the whole caller, but that caller serves submit as well as the polls, and submit is awaited inline inside the Agent's tool call — where the run's LangGraph runtime is still the ambient contextvar, so no secret has to be threaded through `TaskSubmitRequest` or reach durable storage. The caller builds one chain and keeps a second view of it without the context-headers interceptor; `call_tool` takes `request_scoped_headers`, set only by `OrdinaryMcpTaskDriver.submit`. Status and cancel keep server-level credentials, so background polls still cannot fail closed, and the startup warning now describes the half it actually covers. `_merge_preserving_secrets` restores masked extras inside `headers_from_context` instead of writing the `***` sentinel back over the stored value, matching the treatment `user_auth` extras and server-level extras already get; extras a PUT omits carry over as well, while the declared mapping still replaces verbatim so a round trip can remove an entry. `extra="allow"` plus name-based sensitivity detection means the usual casualty is a name-valued key such as `tokenHeader`, not only a credential. The existing override test seeded the static header onto `request.headers`, which production never does, so it modelled a merge that really happens one layer down; the new tests drive a real adapter tool through a real connection and assert on the headers the session is opened with, and the durable-submit test runs through a real tool node with no runtime patching. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(mcp): reject case-insensitive duplicate static header names * fix(mcp): preserve omitted headers_from_context fields on partial updates --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com> |
||
|
wutongyuonce
|
013dca6352
|
fix(skills): refresh user storage after config reload (#4972)
Co-authored-by: Willem Jiang <willem.jiang@gmail.com> |
||
|
georgelichen
|
641a4147e7
|
fix(skills): parse Responses API content blocks in moderation scanner (#4936)
* Fix skill moderation parsing for Responses API content blocks Normalize LangChain Responses API text blocks before parsing the security moderation decision, while preserving the existing fail-closed behavior for unavailable or invalid moderation results. Add regression coverage for mixed content blocks and document the compatibility boundary. Constraint: Responses API AIMessage content is list-shaped while Chat Completions content is string-shaped Rejected: Disable security scanning | would weaken the skill write safety boundary Confidence: high Scope-risk: narrow Reversibility: clean Directive: Keep moderation parsing provider-format tolerant without including reasoning or tool blocks in the decision payload Tested: 27 security scanner tests; ruff check; ruff format check Not-tested: Live moderation request against the configured external endpoint * Reuse shared LLM response text normalization Route skill moderation responses through the existing provider-format normalizer so only text and output_text blocks participate in JSON parsing. Strengthen regression coverage with reasoning and tool blocks that contain misleading text fields.\n\nConstraint: Responses API content is shared across multiple harness consumers\nRejected: Keep a private normalizer | duplicated provider-shape policy diverges and can reintroduce reasoning-block contamination\nConfidence: high\nScope-risk: narrow\nReversibility: clean\nDirective: Extend the shared normalizer when a new provider content shape is verified; do not add divergent local parsers\nTested: 118 related backend tests; regression test red against the previous parser; Ruff check and format check\nNot-tested: Live GitHub CLA status refresh * Restore trusted external skill package loading Skill discovery follows one-level package-directory symlinks, but activation path validation rejected the resolved external path. Restore that compatibility for configured custom-skill category roots while keeping file-level symlinks and deeper escapes blocked. Add regression coverage for local and user-scoped storage plus slash activation, and document the boundary. Constraint: Existing skill discovery follows directory symlinks and operator-managed external packages must remain loadable Rejected: Allow arbitrary resolved paths | would weaken the skill path trust boundary Confidence: high Scope-risk: moderate Directive: Keep the final SKILL.md file symlink-free and preserve one-level category-root validation Tested: 79 targeted skill storage, loader, slash activation, and user-scoped tests passed; Ruff check and format check passed; GitNexus staged change detection reported low risk Not-tested: Real symlink activation on this Windows host lacks SeCreateSymbolicLinkPrivilege and is skipped Related: Skill projection copies sources into sandbox-visible views * Exercise real filesystem symlink boundaries in skill storage tests Replace global Path.resolve/is_symlink mocks with real directory and file symlinks, preserving the Windows privilege skip. Add regression coverage for deeper custom-root escapes and symlinks under non-custom categories so the one-level allowance remains explicit. Constraint: Symlink creation requires SeCreateSymbolicLinkPrivilege on some Windows runners Rejected: Keep global path-method mocks | they validate the mock behavior rather than filesystem semantics Confidence: high Scope-risk: narrow Reversibility: clean Directive: Keep security-boundary tests on real filesystem primitives; skip only when the runner lacks symlink privilege Tested: 76 targeted loader/storage/slash tests; Ruff check; Ruff format check Not-tested: Windows symlink-enabled execution on this host Related: #4936 * Pin the actual nested symlink escape boundary Place the second symlink below a real custom package directory so the test reaches the one-level relative-parent guard instead of returning early on a non-symlink parent. Keep the public-category rejection coverage unchanged. Constraint: The security boundary depends on both symlink depth and category root Rejected: Link the outer package directory directly | the parent is not a symlink at validation time, so the depth guard is never evaluated Confidence: high Scope-risk: narrow Reversibility: clean Directive: Keep this regression tied to the exact relative_parent.parts depth check Tested: Targeted storage, loader, and slash suites; GitNexus staged detection Not-tested: Symlink-enabled execution on this Windows host Related: #4936 * Make the nested symlink regression reach the depth guard The test now validates the SKILL.md directly through the nested symlink, so the symlink is the immediate parent and the relative-parent depth check is executed. Constraint: Windows test execution may skip when symlink privilege is unavailable Rejected: Keep the extra nested path segment | it bypasses the symlink-depth guard through an early return Confidence: high Scope-risk: narrow Reversibility: clean Directive: Mutation tests must fail when the depth restriction is removed Tested: Targeted test (skipped on this Windows host without symlink privilege); Ruff check and format check Not-tested: Real symlink execution on Windows; Linux CI will exercise the case Related: #4936 * Keep sandbox projections fresh for linked external skill packages The storage layer intentionally accepts one-level custom package-directory symlinks, but projection freshness previously hashed only the link inode. Follow the permitted target tree during custom and legacy source-signature scans so edits to SKILL.md, scripts, references, or assets trigger a rebuild before sandbox use. Constraint: Preserve the existing one-level custom/legacy symlink boundary and do not follow public, integration, nested, or file symlinks Rejected: Invalidate projections only from /api/skills/reload | sandbox acquisition must also detect edits made directly in external targets Confidence: high Scope-risk: narrow Reversibility: clean Directive: Keep target-tree traversal limited to the storage paths that explicitly permit external package-directory links Tested: 77 projection, user-scoped storage, and lifecycle tests passed; Ruff check and format check passed; git diff --check passed; GitNexus staged detection reported low risk Not-tested: Real external symlink execution on this Windows host without SeCreateSymbolicLinkPrivilege; existing tests skip that platform limitation |
||
|
Aari
|
5ffc2d3e27
|
feat(mcp): complete durable task notifications and chat UI (#4833)
* feat(mcp): add reliable task notifications and cancellation * feat(mcp): add background task chat UI * fix(mcp): hide and sanitize task notification prompts * fix(mcp): sanitize projected task names * fix(mcp): harden task notifications and details * fix(mcp): harden task lifecycle recovery * fix(mcp): gate task UI and isolate cancellations * test: scope plain-text response locator * fix(mcp): align task notification boundaries * fix(mcp): bound task delivery retries * fix background task notification races |
||
|
Willem Jiang
|
37e19bc445 | fix(ci): fix the lint and unit test errors in backend | ||
|
Nefelibata
|
9668b35b1a
|
fix(skills): copy projected skill files instead of hardlinking (#4825) | ||
|
Nefelibata
|
adf6c422c2
|
fix(skills): fail closed on drifted projection namespace on all platforms (#4830)
* fix(skills): fail closed on drifted projection namespace on all platforms * test(skills): add regression test simulating swallowed unlink on drifted namespace |
||
|
Ryker_Feng
|
ccff5f5ce7
|
docs: govern agent guidance size (#4799)
* docs: govern agent guidance size * refactor: split agent guidance by code scope * Clarify virtual path handling in AGENTS.md Updated the translation section to clarify the role of `LocalSandboxProvider` and the handling of virtual paths in the tool layer. --------- Co-authored-by: Willem Jiang <willem.jiang@gmail.com> |
||
|
therenansimoes
|
bf2cb19ce7
|
test(skillscan): document known instance-client false negatives from #4296 (#4644)
The instance-client signal is a one-level lexical-scope analysis, and issue #4296 enumerates the cases it deliberately does not report. Four of them were only described in prose: a handle reached through a container item, a factory return, a locally aliased constructor, or a dynamic getattr, plus sinks invoked as anything other than name.method(...). Pin them in test_python_declared_false_negatives_stay_unreported alongside the cases already covered, so each is asserted against the runtime oracle -- the client really is called and the scanner really is silent -- rather than assumed. Re-widening or narrowing the model now has to change this test. No behaviour change. |
||
|
Xinmin Zeng
|
f2e832330e
|
fix(sandbox): enforce disabled skills in filesystem views (#4178)
* fix(sandbox): project enabled skills into sandbox views * fix(skills): keep projection mutations consistent * fix(skills): fail closed on projection errors * fix(skills): isolate per-scope failures during boot projection rebuild rebuild_all_skill_projections() propagated any exception from the public rebuild or from a single user's rebuild straight out of the gateway lifespan startup, uncaught. A single broken user directory (bad permissions, corrupted _skill_states.json, unreadable content) would therefore abort gateway boot for every user, not just that one - _rebuild_*_locked already fails closed internally (clears the view and re-raises), so the boot loop only needed to stop treating that re-raise as fatal. Each scope's rebuild now fails closed independently and boot continues; a scope left empty by a boot failure self-heals on the next sandbox acquire via ensure_skill_projections(). Also patches deerflow.skills.projection.rebuild_all_skill_projections in the memory-flush lifespan test fixture, matching the two sibling fixtures in the same file — this call is now on the lifespan startup path and the fixture's minimal SimpleNamespace config predates it. * test(skills): update authz test for the projection-aware public toggle _persist_shared_skill_state (introduced earlier in this branch) reads the shared extensions_config.json fresh from disk under the projection lock instead of through the cached get_extensions_config() singleton - that's the whole point of the fix (stale worker caches must not clobber another worker's concurrent update). The name no longer exists on the skills router module, so the test's monkeypatch of it started raising AttributeError instead of exercising the endpoint. The mock storage in this test isn't a real LocalSkillStorage instance, so _persist_shared_skill_state's projection-mutation branch is already skipped (nullcontext) and it falls back to a fresh ExtensionsConfig() for the nonexistent tmp config_path - no replacement monkeypatch needed. * fix(sandbox): make skill projection ensure best-effort in acquire acquire() called _ensure_skills_projection() directly, outside any try/except, in both LocalSandboxProvider and AioSandboxProvider. Every other skill-mount setup path in these providers has always caught exceptions and logged a warning rather than failing sandbox acquire outright (e.g. when config.yaml can't be resolved) - these two new call sites broke that contract, so any projection failure (including simply not having a config.yaml, as in CI's test environment) now failed acquire() itself instead of just leaving skill mounts off. _ensure_skills_projection now catches its own exceptions and returns None; both providers' callers already tolerate that (a None projection skips the skill-specific mounts, matching the existing degrade path) after making _append_public_skill_mapping and the custom/legacy mount block in LocalSandboxProvider explicitly None-safe. Caught by running the full suite with config.yaml removed, matching CI's environment - not caught locally because a real config.yaml was present, masking the failure. * fix(sandbox): make E2B skill projection mounts best-effort _skill_projection_mounts called ensure_skill_projections with no guard, unlike Local/AIO's _ensure_skills_projection. A raise propagated out of _apply_mounts before the configured-mounts loop ran, so a skills projection failure dropped the operator's own configured mounts too - only caught by create()'s outer warning, with nothing applied at all. Swallow here and return an empty mount list on failure, matching the Local/AIO pattern: still fail-closed for skills, but no longer widens the blast radius to unrelated configured mounts. Review feedback from PR #4178. * docs(skills): document projection trade-offs flagged in review - _update_tree_digest: note the metadata-only (not content) hashing trade-off and why runtime writes through this codebase are still covered regardless (rebuild-under-lock + rename always changes inode). - LocalSandboxProvider.acquire: note the acquire-time self-heal cost (cheap on a fresh manifest, ~400ms rebuild under lock on stale/drift). - skill_projection_mutation: drop the no-op except-Exception-then-raise; a raise from the mutation already propagates past the yield with the view left cleared, no explicit re-raise needed. - provisioner README: spell out that hostPath skills volumes require the gateway and K8s node to share DEER_FLOW_HOST_BASE_DIR (single-node or shared storage), and that the custom/legacy volumes' hostPath type Directory (not DirectoryOrCreate) makes a violation of that assumption a visible Pod-creation failure instead of a silent empty mount. Review feedback from PR #4178. * fix(skills): lazily repair user projections * fix(skills): close projection review gaps * fix(skills): refresh user projection enable state * fix(skills): close projection review follow-ups * fix(skills): preserve state across projection writes --------- Co-authored-by: Willem Jiang <willem.jiang@gmail.com> |
||
|
Huixin615
|
d726ae60c3
|
fix(skills): use managed integrations root for slash activation (#4570)
* fix(skills): use managed integrations root for slash activation * refactor(skills): clarify integrations root getter |
||
|
Ryker_Feng
|
7aa314b4c1
|
feat: add Lark CLI integration (#3971)
* feat: add lark cli integration * fix: polish lark integration actions * feat: support lark incremental permissions * fix: detect lark authorization completion * fix: harden lark integration install * feat: expand lark auth scopes and reuse host auth in sandbox Default lark auth to least-privilege (recommend=false, base sign-in only) and expose the full set of lark-cli --domain business domains as native --domain grants instead of a 4-domain read-only mapping. Resolve the skill pack from the latest larksuite/cli GitHub release at install time with content-hash integrity, and surface version/runtime drift in status. Share the per-user lark-cli config/data profile between the Gateway Settings auth flow and agent conversations by mounting the integration dirs into the AIO sandbox and injecting the matching env for lark-cli commands, with an allowlisted extra_mounts path in the provisioner/K8s backend and traversal guards on integration paths. * style: fix lint issues from ruff and prettier Sort imports in the provisioner PVC test and re-wrap two long i18n description strings to satisfy backend ruff and frontend prettier CI. * fix(lark): address managed integration review feedback * fix(frontend): stabilize integrations settings e2e * test(sandbox): isolate remote backend legacy visibility check * test: fix backend unit failures after merge * Harden Lark integration review fixes * Format Lark integration E2E test * fix(lark): harden sandbox credential exposure and status disclosure Address willem_bd's security review on PR #3971: - Mount the per-user lark-cli config dir (long-lived appSecret) read-only into the AIO sandbox; only the refreshable-token data dir stays writable. - Redact host filesystem paths (install_path, cli.path) from GET /lark/status and the config/auth complete responses for non-admin callers, fail-closed on any auth error. - Document the npm postinstall trade-off (--ignore-scripts is not viable because @larksuite/cli fetches its platform binary in postinstall). - Document the sandbox credential trust boundary in AGENTS.md and README, pointing at the sidecar-broker follow-up (#4338). --------- Co-authored-by: Willem Jiang <willem.jiang@gmail.com> |
||
|
Daoyuan Li
|
159b774944
|
fix(skills): handle non-string frontmatter keys (#4167)
Normalize YAML frontmatter keys in the shared parser so validation and review report malformed fields instead of failing while sorting mixed key types. |
||
|
Aari
|
3c0a45ad77
|
fix(skills): inject Langfuse metadata into the standalone skill scan (#4321) | ||
|
Aari
|
e66f455d51
|
fix(skills): don't treat a lazily evaluated PEP 695 type alias as a network sink (#4315)
* fix(skills): don't treat a lazily evaluated PEP 695 type alias as a network sink * test(skills): cover type alias parameter bounds |
||
|
Daoyuan Li
|
6544d96cc4
|
fix(skills): close AST literal-only shell=True bypass in SkillScan (#4057)
* fix(skills): close AST literal-only shell=True bypass in SkillScan SkillScan's Python analyzer only classified a subprocess call as the CRITICAL, hard-blocked python-shell-exec rule when its shell= keyword was the literal AST constant True. Any non-literal value with the same runtime effect - a variable (shell=shell_flag) or an expression (shell=bool(1)) - fell through to the HIGH, non-blocking python-subprocess classification instead, silently bypassing enforce_static_scan's deterministic CRITICAL gate despite behaving identically to shell=True at runtime. _call_has_shell_true is renamed to _call_shell_may_be_true and now fails closed on ambiguity: any shell= value that is not a literal, statically-provable False is treated as CRITICAL, matching the literal shell=True case. A call with no shell= keyword at all is unaffected (subprocess already defaults to shell=False). Adds regression tests for the variable and expression bypass shapes, plus a boundary test locking in that literal shell=False remains a non-blocking warning. * fix(skills): fail closed on **-unpacked shell= in SkillScan _call_shell_may_be_true only checked keyword.arg == "shell", so a subprocess.* call that supplies shell via **-unpacking (a keyword node with arg is None) fell through to the non-blocking python-subprocess classification instead of the CRITICAL python-shell-exec path. Treat any **-unpacked keyword as shell-ambiguous and fail closed, same as the existing shell=variable/shell=expression handling. This intentionally over-blocks a **-unpack that carries no shell key, since a mapping's contents are not knowable by static analysis; that tradeoff is documented inline and covered by a dedicated test. |
||
|
Aari
|
cd34a1a504
|
fix(skills): don't attach model tracing to the in-graph skill security scan (#4252)
* fix(skills): don't attach model tracing to the in-graph skill security scan * fix(skills): pass attach_tracing explicitly from the in-graph scan call site Follow the tracing INVARIANT's own convention rather than detecting the call context: scan_skill_content takes an attach_tracing flag, and _scan_or_raise -- the single in-graph choke point -- passes False. Standalone callers (Gateway skill routes, installer) keep the default True. The INVARIANT list named four sites and asks that new in-graph calls be added to it; record this fifth one so a future audit of that list finds it. --------- Co-authored-by: Willem Jiang <willem.jiang@gmail.com> |
||
|
Aari
|
a8bf54cbbb
|
feat(skillscan): detect exfil through instance/dataflow network clients (#4265)
* feat(skillscan): detect exfil through instance/dataflow network clients * fix(skillscan): resolve client handles with Python lexical scopes _walk_client_nested_scope copied every live handle into a nested scope after excluding only parameters. That is not Python's name resolution: a class namespace is not a closure scope for its methods, a function-local binding shadows an enclosing name across the whole body, and comprehensions bind their targets in a scope of their own. Benign skills were therefore blocked as python-env-dump-exfil (CRITICAL) even where the outbound-looking call provably cannot run on the tracked client. Derive each scope's bindings lexically instead: - a class body reads its enclosing scope, but the names it binds are not passed into the methods defined in it; - comprehensions are their own scope, so only the outermost iterable is evaluated outside and every for-target shadows first; - a function-local prepass excludes names the body binds anywhere, with global/nonlocal opting back out. Detection is unchanged where the client really is reachable: a comprehension calling an unshadowed handle, a method closing over an enclosing function's handle, and a global-declared module handle all still block. * fix(skillscan): apply target rebinding in Python evaluation order The generic ast.iter_child_nodes() fallback yields fields in declaration order, which for `for`/`async for`, `:=` and `+=` puts the binding target ahead of the expression Python evaluates to produce it. Visiting the target first dropped the handle before the call that actually runs on it, so `for s in s.post(host, ...)` reported nothing even though Python calls post() on the client before binding the loop target. Match captures bind through a plain `name` string rather than a Name node, so the Store branch never saw them and a rebound name kept a stale handle. Assignment expressions inside a comprehension were applied only to the comprehension-local map, leaving the containing scope stale as well. Give every bind-after-evaluate construct its own branch: - `for`/`async for` walk the iterable against the pre-loop binding, then rebind the target, then walk the body; - `:=` walks its value first and binds into the containing scope too, since PEP 572 puts a comprehension's walrus target there; - `+=` walks its value before dropping the target; - match captures drop the handle, matching how a rebind under `if`/`try` that may equally not execute is already treated. The bypasses this closes are the reason detection widens here; the comprehension walrus and match cases narrow it back where the client provably cannot be the receiver. * fix(skillscan): preserve scoped client handle semantics * fix(skillscan): scan sinks inside assignment target expressions The Assign/AnnAssign, AugAssign, For/AsyncFor, with, and comprehension branches evaluated only the value and rebound the target, so a client call placed in an attribute receiver or a subscript value/index -- both evaluated at bind time -- was never scanned. os.environ could exfil through a tracked client while python-env-dump-exfil reported nothing. Add _walk_client_target_exprs to walk the executable parts of a binding target (attribute receiver, subscript value+slice, recursing through tuple/list/starred) in Python evaluation order, without treating Store name leaves as reads. The AnnAssign annotation expression is walked too. Name-leaf invalidation is unchanged. * fix(skillscan): apply assignment targets and annotations in runtime order The round of assignment-target scanning walked every target then rebound the names in one batch, and always walked an AnnAssign annotation before the target. Python instead binds chained and destructured targets left to right (so `session = out[session.post(...)] = cfg` runs the subscript on the already-rebound name), evaluates a variable annotation only in module or class scope (never in a function, never under `from __future__ import annotations`), and evaluates an executed annotation after the target. The scanner therefore hard-blocked benign skills. Bind each target left to right via _bind_client_targets (walk its executable sub-expressions against the current bindings, then rebind before the next target), and walk an annotation only for the nodes _evaluated_annotation_nodes marks as actually evaluated, after the target. Target-expression scanning and name-leaf invalidation are otherwise unchanged. * fix(skillscan): scan client sinks in evaluated function-signature annotations A function's parameter and return annotations are evaluated at def time in the enclosing scope, like its decorators and defaults, so a tracked client sink placed in one is a real egress. _client_scope_prelude walked the decorators and defaults but not the annotations, so os.environ could exfil through `def f() -> session.post(host, json=dict(os.environ))` while python-env-dump-exfil reported nothing. Record function/async-function defs in _evaluated_annotation_nodes (their signatures evaluate at def time unless from __future__ import annotations postpones them) and, for those nodes, add the parameter and return annotation expressions to the enclosing-scope prelude walk. * fix(skillscan): match runtime evaluation order for annotations and except handlers * fix(skillscan): scope try/match branches to their own selection state * fix(skillscan): propagate branch bindings to everything that observes them A branch's net effect has to be visible exactly where Python makes it visible. The walker isolated `except`/`else`/`match` bodies into scope copies and then discarded them, so a client created on the branch was invisible to `finally`, to the code after the statement, and to anything defined inside the branch, while a name the branch replaced stayed a sink receiver. - `except*` clauses are sequential, not alternatives: thread one scope through body, clause types and clause bodies in source order instead of reusing the mutually exclusive copies ordinary `except` needs. - Fold each `except`/`else`/`match` branch's net effect back into the scope that `finally` and the following code read, and make the branch scope what nested definitions close over. - Keep a fallthrough scope across `match` guards, so a guard that returned false still hands its side effects to the next case, while pattern captures stay isolated to their own case. - Keep an `as` target path-local: Python unbinds it only on the path that ran, so dropping it for every path erased a live handle where no such handler executed. Adds a 14-case runtime-oracle regression covering both directions at every branch site; each of the ten guards was deleted on its own to confirm the test that pins it goes red. * fix(skillscan): join alternative branches instead of overwriting one with another Only one of a statement's alternative branches runs, but the walker folded each one into a single destructive binding map. Whichever branch was visited last therefore decided the state: a handler that rebinds the name erased a sibling that leaves the client in place (missing a client Python really calls), and a handler that builds one was credited on paths where it never ran (inventing a CRITICAL sink). Alias targets had the mirror problem, since only key presence was compared, so replacing `import x as name` on a live branch was ignored. - Join alternatives into a may-state: a name stays a sink receiver when any feasible branch leaves it a client, and stops being one only when every feasible branch replaced it. Alias targets join toward the target that can still name a constructor. - Treat each `except*` clause as optional rather than threading every clause body unconditionally, so a clause whose type never matched cannot erase a handle the next clause calls. - Keep the fall-through state (no exception raised, no case matched) as one more alternative, and drop it only where the source decides the outcome: a literal always-raising body selecting one handler, a literal exception group choosing `except*` clauses, a wildcard or literal-equal `match` case. Also corrects an existing match-capture test that asserted the non-exhaustive case is benign: the runtime oracle shows the original client still takes the call on the path where nothing matched. Adds runtime-oracle regressions for both directions at every alternative site; each of the nine guards was deleted on its own to confirm the test pinning it goes red. * fix(skillscan): model feasible conditional client flow * fix(skillscan): preserve feasible control-flow outcomes * fix(skillscan): model expression evaluation paths * fix(skillscan): narrow instance-client detection to lexical statement order The construction-to-use signal had grown into a path-aware interpreter: exception selection, except* subgroup consumption, match capture timing, finally override, comprehension laziness, annotation evaluation order, and may-state joins over feasible branches. That is the heavyweight analysis RFC #2634 rules out of Phase 5, and because the signal feeds a CRITICAL rule it hard-blocks skill installation, so every ambiguity it resolved by over-reporting cost a benign skill instead of a human review. Replace it with ordinary statement order over a one-level handle map: a known constructor bound to a simple name (including `with ... as`), a direct outbound method call on that name in the same lexical scope, rebinding invalidation, and name-to-name alias propagation so `s = session` does not shed the handle. A sink is recorded at the call, so a rebind after the call cannot retract it. Compound statements are not interpreted. Every name an if/try/except*/loop/ match may bind is dropped before its bodies are walked, and each body is walked from an isolated copy. Dropping before rather than after is what keeps a `finally` that runs after a handler rebound the name, a later except* clause, and a second loop iteration from reporting a client the runtime never calls. Bodies are still walked, or wrapping any construction in `if True:` would be a universal bypass. Lexical scoping is unchanged: class namespaces are not closures for their methods, comprehension targets and function-local bindings shadow, and alias visibility stays per scope. The cases this gives up are false negatives by construction and are recorded in #4296, pinned by a test that asserts the runtime really calls the client while the scanner stays silent. Verified: 102 SkillScan tests; full suite 8 failed / 7851 passed, the same network-dependent web-fetch tests that fail on clean main; per-clause red check 12/12 guards red; 925 repo-owned files scanned branch vs main with 0 new and 0 lost CRITICAL findings; #4158 bypass BLOCKED and #4153 false positive allowed with 0 findings through the real enforce_static_scan gate. * test(skillscan): pin closure boundary * refactor(skillscan): narrow client handle analysis * fix(skillscan): close client handle correctness gaps * fix(skillscan): require proven client imports |
||
|
Daoyuan Li
|
0cd55067f3
|
fix(skills): reject colon in zip member names to close NTFS ADS smuggling gap (#4236)
Neither is_unsafe_zip_member (installer.py) nor its duplicated check in skillscan/orchestrator.py rejected a colon in a zip member name. On Windows/NTFS, a name like scripts/run.sh:hidden.txt addresses an Alternate Data Stream on run.sh instead of creating a new file, so the hidden content is invisible to Path.rglob()/os.walk()-based scanning in both the deterministic static scanner and the extracted-file content scanner, while still landing genuinely on disk. Reject any colon in a zip member's relative path outright in both files; a colon has no legitimate use there since zip entries use forward slashes and a real Windows drive prefix is already caught by the existing absolute-path check. |
||
|
Hanchen Qiu
|
d2f8f61e3a
|
fix(skills): add security_fail_closed option for moderation model outages (#4297)
* fix(skills): add security_fail_closed option for moderation model outages When the skill security moderation model call fails, scan_skill_content previously blocked ALL content (executable and non-executable), which turns a moderation-model outage into a denial of service for skill writes. Add a skill_evolution.security_fail_closed option (default True, preserving current behavior). When set to False, non-executable content is allowed with a warn decision during an outage while executable content is still blocked. Closes #3021 * fix(config): bump config_version to 27 and format skill_evolution config Address review feedback on #4297: - Bump config_version 26 -> 27 so existing installs are flagged outdated and pick up skill_evolution.security_fail_closed via make config-upgrade. - Apply ruff format to skill_evolution_config.py to satisfy the backend formatting gate. - Add config-version/upgrade regression tests covering the v26 outdated warning and merging security_fail_closed without changing user values. * fix(helm): bump chart config_version to 27 to match config.example.yaml Keeps deploy/helm/deer-flow/values.yaml and its README example in sync with the config schema bump, satisfying scripts/check_config_version.sh (validate-chart CI). * fix(skills): surface fail-open security scan in logs Address @willem-bd review feedback on #4297: - Log an operator-visible warning when the moderation model is unavailable and fail-open lets non-executable skill content through as a warn, so a skipped scan is no longer silent. - Reword the model-call-failed log so it stays accurate under both fail-closed and fail-open policy instead of always claiming a "conservative fallback". - Add a regression test asserting the fail-open warn path emits the warning log. |
||
|
Daoyuan Li
|
1ae02913ea
|
fix(skills): cap archive entry count in safe_extract_skill_archive (#4241)
safe_extract_skill_archive() capped total uncompressed bytes (zip bomb defence) but had no limit on member count, so a small archive with tens of thousands of tiny/empty entries extracted with no error. The same entry-count cap already existed in scan_archive_preflight() (skillscan orchestrator, 4096 members) with the comment "a huge member count is a bounded DoS vector even when the total size is small" -- but that scan only runs when the optional skill_scan.enabled kill switch is on (default true, but operator-configurable), so disabling it silently dropped this specific protection while config.example.yaml's comment implied safe archive extraction alone still covered it. Move the same 4096 cap into safe_extract_skill_archive itself as an early-abort before any per-member work, so it applies unconditionally regardless of skill_scan.enabled. Leaving scan_archive_preflight's own cap in place as defense in depth (it fires earlier, on preflight, with a structured finding for reporting). Related: #2618 requested exactly this hardening; #2619 (closed, unmerged) implemented a broader version of it, including this same entry-count cap directly in the extractor. |
||
|
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 |
||
|
Aari
|
81b3ed0188
|
fix(skillscan): recognize remaining outbound network sinks (#4153)
_call_is_network_sink missed the HEAD/OPTIONS verbs on requests/httpx, socket.create_connection, and urllib.request.urlretrieve. A bulk env dump or reverse shell shipped through any of these slipped past the CRITICAL exfil/reverse-shell rules whenever the URL was assembled at runtime (the non-literal case the string-literal URL check can't cover). Also treat socket.create_connection as the socket primitive in the reverse-shell shape. http.client.HTTP(S)Connection is intentionally left out: only the lazy constructor is statically visible (the request()/connect() that performs the I/O is an instance method the call-name analyzer can't resolve), so flagging the constructor would hard-block benign code that only builds a connection object. Cover the alias-resolved forms too: the sink check runs on the name after from-import / import-as resolution, a path the suite exercised only on the env-read side (#4087) and not on the sink side. |
||
|
Yufeng He
|
cbbd72a1ab
|
fix(skillscan): recognize remaining requests/httpx HTTP methods as network sinks (#4130)
python-env-dump-exfil flags a file that both reads the bulk process environment and reaches a network sink. The call-based sink check only listed requests get/post/put/request and httpx get/post, so a bulk env dump sent through an equally body-carrying method (requests.patch/delete, httpx.put/patch/delete, or the generic httpx.request/stream) evaded the CRITICAL finding whenever the destination URL was not a plain string literal (e.g. built at runtime) -- the exact evasion the string-literal URL sink is meant to resist. requests.post was caught but requests.patch was not, an arbitrary gap on clients the analyzer already covers. Complete the requests and httpx HTTP-verb surface in _call_is_network_sink so an obfuscated-URL exfil through those methods is flagged like post/put. Signed-off-by: Yufeng He <40085740+he-yufeng@users.noreply.github.com> |
||
|
Aari
|
42544755ac
|
fix(skills): escape untrusted skill metadata before it enters the model prompt (#4128)
* fix(skills): escape untrusted skill metadata before it enters the model prompt Skill name/description/allowed-tools come from the frontmatter of a user-installable .skill archive (POST /api/skills/install or a drop into skills/custom/); the parser only strips them. The slash-activation and durable-context siblings already html.escape these exact fields before rendering them into a model-visible block -- but five other render sites emit them raw. The sharpest is the default path, <available_skills> in the system prompt (skills.deferred_discovery: false): a community skill whose description closes the block can forge a framework-trusted <system-reminder> into the lead-agent system prompt. Driven through the real apply_prompt_template(), the forged tag reaches the system prompt raw on main and is neutralized here. Escape at every render site that emits untrusted skill metadata/content: - <available_skills> (name/description/location) and <disabled_skills> (name) in lead_agent/prompt.py; - describe_skill output (name/description/allowed-tools/location) and <skill_index> (name) in skills/describe.py; - the subagent <skill name=...> attribute plus the raw SKILL.md body in subagents/executor.py::_load_skill_messages -- its direct sibling skill_activation escapes both, this escaped neither. quote=False in element-text positions (matching skill_context and the #4097 correction), quote=True in the one attribute position (matching skill_activation). category is a controlled enum and is left as-is; escaping is render-time only, so stored skills are unchanged and re-rendering never double-escapes. * fix(skills): escape skill name in the slash-activation prose line The slash-activation reminder emitted `activation.skill_name` raw in its prose line while escaping the same value in the adjacent <skill name="..."> attribute. skill_name is grammar-gated to [a-z0-9-] by resolve_slash_skill before it reaches the renderer, so this is a defense-in-depth / consistency fix rather than a reachable injection: the two positions can never drift if a future caller builds an activation from an unconstrained name. Reuse the already-computed escaped_skill_name. |
||
|
黄云龙
|
897be7e064
|
fix(skillscan): detect os.environ access via from-import pattern (#4087)
* fix(skillscan): detect os.environ access via from-import pattern * fix(skillscan): detect os.environ access via from-import pattern |
||
|
Ryker_Feng
|
41658c5ff4
|
feat(skills): add skill review quality gate (#4037)
* feat(skills): add skill review quality gate * fix(skills): skip review eval fixtures in CI * fix(skills): ignore review eval fixtures in bundled scans * fix(skill-review): harden review gate boundaries * fix(skills): address skill review gate feedback |
||
|
Ryker_Feng
|
c640b52a7d
|
feat(frontend): render slash-skill activations as inline chips (#3981)
* feat(frontend): render slash-skill activations as inline chips Show an explicit `/skill` activation as a compact inline chip in both the composer and the chat transcript instead of raw slash text. - Composer: selecting a skill suggestion stores it as a removable chip aligned inline with the textarea; the leading `/skill ` prefix is reattached only at submit time, so the backend activation protocol is unchanged. Backspace on an empty input or the chip's close button clears it; history navigation is disabled while a chip is active. - Transcript: human messages that begin with `/skill` render the skill as a read-only chip followed by the task text. - Add a shared `core/skills/slash.ts` (`parseSlashSkillReference` + `resolveSlashSkillDisplay`) mirroring the backend `slash.py` gate, so the transcript only shows a chip when the skill actually exists and is enabled. This removes a duplicated regex/reserved-name list and keeps display semantics consistent with backend activation. Add unit tests for the shared slash parser and extend the chat e2e to assert the composer still submits `/skill <task>` after showing a chip. * chore(frontend): format chat e2e test * refactor(skills): address slash-skill chip review feedback Follow-up to the inline slash-skill chip PR, resolving three second-order review findings: - Drive the reserved-command set and skill-name grammar from a shared contracts/slash_skill_contract.json instead of a hand-copied "keep in sync" pair. slash.ts and slash.py now reference the fixture, and contract tests on both sides fail CI if either drifts. - Extract a shared SlashSkillChip so the composer and transcript chips stay in lockstep, and normalize the off-scale /8 and /12 opacity steps to the standard /10 and /20 tokens. - Split HumanMessageText into a pure parse gate plus a slash-only subtree that owns the useSkills() lookup, so a skill-enabled toggle no longer re-renders every plain-text human turn. Verified: frontend eslint + tsc clean, pnpm test 572 pass (incl. new slash-contract test); backend slash contract + slash-skills tests 31 pass. * style(tests): sort slash skill contract imports * fix(composer): inline the slash-skill text so the chip aligns with input Address the "composer body layout change" review on #3981 by rendering the active skill as an inline chip in the same text flow as the prompt, rather than a separate flex row that drifted the box model across states. - Render the chip + prompt inside one leading-6 wrapper and edit the prompt through a `contentEditable` span, so the chip sits inline with the first line and long/multi-line input wraps naturally back to the container edge. - Align the chip with `align-top`: its h-6 (24px) height matches the text line height, so chip and first-line centers coincide exactly (measured delta 0), fixing the chip being raised above the baseline. - Restore the placeholder in chip mode via a `data-empty` CSS `::before`, which also gives the empty editable span width so it is no longer treated as hidden. - Widen the IME helper to `HTMLElement` and route the span's keydown/paste through the shared skill-suggestion, prompt-history, backspace-to-clear, and IME-composition handlers so contentEditable behaves like the textarea. - Extend chat.spec.ts to drive the inline skill editor instead of the textarea after a chip is shown. * style(frontend): fix composer class order formatting * fix(composer): break long unbroken input inside the slash-skill row The inline slash-skill editor wrapped with `break-words` (overflow-wrap: break-word), which only moves an over-long token to the next line before breaking it. A long unbroken string therefore started on the line below the chip, and when the string contained a break opportunity such as a hyphen the browser wrapped there and pushed the remaining run to the next line, leaving a wide gap on the right. Switch to `break-all` (word-break: break-all) so the text fills each line from the chip and packs tightly regardless of hyphens or CJK. |
||
|
Vanzeren
|
e3137b13cf
|
fix(provisioner): gate legacy skills mount by user visibility (#3985)
* fix(provisioner): gate legacy skills mount by user visibility * fix(test): aio sandbox provider * fix: use shared legacy skill visibility helper for sandbox mounts |
||
|
AochenShen99
|
658c39ccf7
|
feat(skills): Add native SkillScan phase 1 for skills (#3033)
* Add phase 1 skill static scanning * Rework SkillScan phase 1 as native scanner * refactor(skillscan): align phase 1 with trimmed RFC contract - SecurityFinding: 7 fields (rule_id, severity, file, line, message, remediation, evidence); category/analyzer derive from the rule_id prefix, confidence/column/fingerprint/metadata removed - scan_archive_preflight()/scan_skill_dir() are pure functions: no ScanContext, no policy schema; CRITICAL-blocks is a code constant and skill_scan.enabled is applied by enforce_static_scan()/callers - secret-* evidence is redacted before findings leave the scanner - de-dup keys on (rule_id, file, line) so repeated occurrences keep distinct locations for agent self-correction - cloud-metadata detection consolidated into network-cloud-metadata - nested zip members get a one-level stdlib magic-byte peek; an executable member escalates package-nested-archive to CRITICAL - install metadata sidecar removed (Phase 7 decides if it is needed) - rule specs moved next to their analyzers; skillscan/rules/ removed - tests updated + new anchors: redaction, dedup lines, nested-zip escalation, single cloud-metadata rule, bundled-skill zero-CRITICAL * fix(skillscan): tighten reverse-shell/secret/archive scan rules from review Address PR #3033 review feedback on the native SkillScan analyzers: - Reverse-shell false positives: split shell detection by signal strength (/dev/tcp/, nc -e stay CRITICAL; bash -i, mkfifo -> new HIGH shell-reverse-shell-heuristic, warn->LLM). The Python check is now AST-anchored on real socket.socket/os.dup2/subprocess call sites instead of raw-text substring matching, so prose/docstrings no longer hard-block. - Secret evidence: _redact_secret_evidence returns [redacted] with no secret bytes (was value[:6], which leaked 2 real token bytes past the prefix). - Archive DoS: cap outer archive member count (_MAX_ARCHIVE_MEMBERS=4096); scan_archive_preflight early-aborts with a package-too-many-members CRITICAL finding (routes through the existing blocked->400 fail-closed path). - shell-destructive-command: broaden the rm -rf matcher to sensitive system roots (/home, /usr, /*, --no-preserve-root /) while leaving safe subpaths unflagged. - Dead code: collapse _decode_text_for_analysis to a single decode path and drop the unused _TEXT_SUFFIXES set and _has_text_shebang helper. - local_skill_storage: document why the host_path branch keeps app_config possibly-None (lazy kill-switch resolution; avoids eager get_app_config in config-free environments such as CI). Tests: new negative/positive coverage in test_skillscan_native.py. Full backend suite 6616 passed, 26 skipped. |
||
|
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> |
||
|
Tianye Song
|
15454b6fec
|
feat(skills): deferred skill discovery via describe_skill tool (#3775)
Replace the full-metadata <available_skills> system-prompt block with a compact <skill_index> (names only) and an on-demand describe_skill tool when skills.deferred_discovery: true (default: false / backward compat). New modules: - skills/catalog.py — SkillCatalog (immutable, searchable; select: has no cap, keyword/prefix search caps at MAX_RESULTS=5) - skills/describe.py — build_describe_skill_tool(catalog) closure; build_skill_search_setup() wires SkillSearchSetup into both the LangGraph agent factory (agent.py) and DeerFlowClient (client.py) Changes: - Skill @dataclass(frozen=True); allowed_tools/required_secrets list→tuple - Skill First prompt line gated on skill_names (deferred vs legacy wording) - get_skills_prompt_section: short-circuit storage on deferred path; merge user_id (upstream) + skill_names (this PR) params - describe_skill tool parameter named "name" (matches prompt wording) - select: branch removes [:MAX_RESULTS] cap (exact request, not ranking) - AGENTS.md: document deferred_discovery config field + new modules Co-authored-by: Willem Jiang <willem.jiang@gmail.com> |
||
|
Zhipeng Zheng
|
53a80d3ad1
|
feat(skills): per-user custom skill isolation with sandbox mounting (#3889)
* feat(skills): per-user skill isolation (#2905) Implement user-scoped skill storage that isolates custom skills between users while sharing public skills globally. Key changes: - Add UserScopedSkillStorage class for per-user custom skill directories - Introduce get_or_new_user_skill_storage() factory with user_id context - Auth middleware sets effective_user_id for request-scoped storage - Agent/prompt/middleware now use user-scoped storage and prompt cache - Sandbox mounts user-scoped skill directories for search/read tools - Add validate_skill_file_path() to SkillStorage for path security - Migration script supports --all-users bulk migration - Frontend: add editable field to Skill type, error check in enableSkill - All skill categories can be toggled (custom skills default to enabled) - Update skill-creator SKILL.md with isolation-aware instructions Tests: - Add test_user_scoped_skill_storage.py (new) - Update all existing skill tests for user-scoped storage - Update sandbox, client, and router tests * fix(skills): address second-round PR review feedback (#3889) - P1-1: restrict legacy skill mount to users without custom skills - P1-2: fail-closed for _is_disabled_skill_path (OSError → return True) - P2-1: AND-merge global extensions_config skill disabled state - P2-2: atomic write for _skill_states.json (mkstemp + replace) - P2-3: normalize X-DeerFlow-Owner-User-Id in trusted boundary - P2-4: LRU-bounded _enabled_skills_by_config_cache (OrderedDict, maxsize=256) - P2-5: clear global prompt cache on PUBLIC skill toggle - P2-6: invalidate skill caches on client.update_skill * fix(tests): correct tool policy test after merge * fix(skills): use DEFAULT_SKILLS_CONTAINER_PATH in UserScopedSkillStorage The "/mnt/skills" literal in UserScopedSkillStorage.__init__ triggers test_skill_container_path_defaults::test_mnt_skills_literal_is_owned_by_skill_constants_module on CI. Migrate the default to the existing deerflow.constants constant, matching the pattern already used by LocalSkillStorage, SkillStorage, and the durable/tool_error middlewares. --------- Co-authored-by: Willem Jiang <willem.jiang@gmail.com> |
||
|
AochenShen99
|
66b9e7f212
|
feat: emit structured runtime metadata (follow-up#3887) (#3906)
* feat: emit structured runtime metadata * fix: avoid subagent import cycle in replay gateway * fix: preserve legacy subtask result parsing * refactor: tighten runtime metadata contracts * fix(middleware): keep recovery hint on task exception wrapper content The structured-metadata stamp overwrote the wrapper text with the bare task-failure message, dropping the model-facing 'Continue with available context, or choose an alternative tool.' guidance that every other tool exception keeps. Append the shared hint after the formatted message. * fix(subagents): require lowercase hex for result_sha256 reader Length-only validation accepted any 64-char string; a faulty serializer or relaying wrapper could store a non-digest value in the delegation ledger. Enforce the producer's hexdigest shape with a fullmatch. --------- Co-authored-by: Willem Jiang <willem.jiang@gmail.com> |
||
|
AochenShen99
|
9a088805d7
|
fix(skills): close skill install security scan coverage gap (#3924)
* fix(skills): close install-path scan coverage gap Skill installs only sent scripts/* and document files under references/ and templates/ to the LLM security scanner. Code at the skill root or under lib/, bin/, src/, etc., and binary files could be installed without any scan. - Scan code files anywhere in the skill tree (by extension, plus shebang detection for extensionless files) with the executable policy: only an explicit allow admits them. - Reject ELF/PE/Mach-O executable binaries by magic bytes during safe archive extraction; non-executable binary assets remain allowed. Interim hardening ahead of the SkillScan framework (RFC #2634); the deterministic full-tree scanner from PR #3033 supersedes the per-file LLM coverage when it lands. * Apply suggestions from code review Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> * test(skills): pin magic coverage and shebang offload boundary Follow-up to the applied review suggestions: cover every Mach-O magic variant with tests (plus the fat64 pair and a partial-prefix asset that must stay installable), name the pure classification helper so the call-site logic reads as policy, drop the now-unused sync _is_code_file, and pin that only extensionless files get the shebang sniff. --------- Co-authored-by: Willem Jiang <willem.jiang@gmail.com> Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> |
||
|
kongxiangxin
|
9d7e131340
|
fix(skills): preserve read_file for lead skill loading (#3862) (#3863) | ||
|
DanielWalnut
|
25ea6970a6
|
feat(runtime): implement goal continuations (#3858)
* implement goal continuations * fix(goal): address review findings for goal continuations - goal: key the no-progress breaker on a signature of the latest visible assistant evidence instead of the evaluator's volatile free-text, so it actually fires on stalled turns; thread the signature through every worker persist / no-progress call site - goal: align _stand_down_reason default caps with should_continue_goal (8 / 2) so the two gate functions agree on goals missing the fields - runtime: offload the synchronous checkpointer fallback via asyncio.to_thread (goal.py + worker.py) to keep blocking IO off the loop - frontend: i18n the GoalStatus "Goal" label (goalLabel in en/zh/types) - frontend: extract pure composer helpers into input-box-helpers.ts with unit tests (parseGoalCommand, readGoalResponseError, skill suggestions) - tests: cover the evidence-based no-progress and default-cap behavior - docs: align backend/AGENTS.md goal paragraph with actual behavior - e2e: prettier-format chat.spec.ts (fixes the lint-frontend CI failure) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * feat(frontend): hide goal continuation counter until the agent continues The goal status bar rendered a raw "0/8" before any auto-continuation, which read as a mysterious score. Now the counter is hidden until continuation_count > 0, then shows "Continuing N/M" with a tooltip explaining the auto-continuation cap. - Extract getGoalContinuationDisplay into a pure helper (hides at 0) + unit tests - Add goalContinuing / goalContinuationTooltip i18n keys (en/zh/types) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(goal): address review findings for goal continuations Frontend correctness - Fix the optimistic /goal result permanently shadowing server goal state: the streamed continuation counter never surfaced for a goal set in-session. Extract a shared useActiveGoal hook (used by both chat pages) that reconciles the optimistic copy with server state via a goalReconciliationKey, de-duping the copy-pasted goal block across the two pages. - Stop /goal status|clear failures from escaping handleSubmit as unhandled rejections (handleGoalCommand now returns success; the run only starts when a goal was actually saved). - Use a function replacer for the goal-status toast so an objective containing $&/$1 isn't treated as a replacement pattern. Backend cleanliness / correctness - De-duplicate four byte-identical helpers (_call_checkpointer_method, _message_type, _additional_kwargs, _is_visible_message) by importing them from runtime.goal instead of re-defining them in the run worker. - Remove the dead `checkpoint_tuple.tasks` durability guard (CheckpointTuple has no tasks field) and document that pending_writes is the durability signal. - Decompose the 176-line _prepare_goal_continuation_input: extract _reread_goal_and_checkpoint and a _persist closure so the thread-unchanged guard and stand-down persistence aren't open-coded three times. Document the last-writer-wins write-window limitation as a follow-up. - Add a shared parse_goal_command helper and use it from the TUI and IM-channel /goal handlers (one place for the status/clear/set semantics). Tests - Restore the 11 command-registry tests dropped by the previous goal change (filter_commands ranking/description, build_registry builtins/skills, resolve cases) alongside the new goal tests. - Add coverage for the IM-channel _handle_goal_command, the TUI _handle_goal handler, parse_goal_command, and goalReconciliationKey. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix goal review feedback * fix goal continuation checkpoint races * prioritize goal commands while streaming Route composer submits through a shared helper so /goal commands can be handled before the streaming stop shortcut, while ordinary streaming submits still stop the active run. Testing: cd frontend && pnpm exec rstest run tests/unit/components/workspace/input-box-helpers.test.ts tests/unit/components/workspace/goal-status-helpers.test.ts; cd frontend && pnpm check * preserve goal status during clarification Keep omitted stream goal fields distinct from explicit null clears so clarification interrupts do not hide an active thread goal that is still present in the checkpoint. Testing: pnpm exec rstest run tests/unit/components/workspace/use-active-goal.test.ts tests/unit/components/workspace/input-box-helpers.test.ts tests/unit/components/workspace/goal-status-helpers.test.ts; pnpm check; git diff --check * style: format active goal hook Run Prettier on use-active-goal.ts to satisfy the frontend lint workflow formatting gate. Testing: pnpm format; pnpm exec rstest run tests/unit/components/workspace/use-active-goal.test.ts tests/unit/components/workspace/input-box-helpers.test.ts tests/unit/components/workspace/goal-status-helpers.test.ts; pnpm check; git diff --check * fix goal review followups --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> |
||
|
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. |
||
|
Yufeng He
|
2e789eae18
|
fix(skills): synchronize skill storage singleton lifecycle (#3778)
get_or_new_skill_storage() and reset_skill_storage() touch the process-global skill storage singleton without a lock - the same unsynchronized check-then-create that #3730 just fixed in sandbox_provider.py, the module this file documents itself as mirroring. Two callers racing a cold start can both see _default_skill_storage is None and each build a SkillStorage, so the second overwrites the first; a reset_skill_storage() racing a get can also null the global between the None-check and the return. Guard the build/return and the reset with a module-level threading.Lock and a double check, mirroring get_memory_storage(). Construction stays inside the lock (rather than sandbox_provider's build-outside-then- discard-the-loser) because SkillStorage has no teardown hook, so a losing racer's orphan could not be cleaned up. Add backend/tests/test_skill_storage_lifecycle.py with concurrency regression tests (8-thread cold-start race asserting a single instance; reset racing gets asserting no None is returned). Signed-off-by: Yufeng He <40085740+he-yufeng@users.noreply.github.com> |
||
|
AochenShen99
|
b8f5ed360f
|
fix(skills): keep skill archive installation off the event loop (#3505)
* fix(skills): keep skill archive installation off the event loop ainstall_skill_from_archive — the async entry point awaited by the gateway POST /skills/install route — ran its entire filesystem pipeline inline on the event loop: zip extraction, frontmatter validation, rglob enumeration, per-file read_text, shutil.copytree staging, and tempdir cleanup. Restructure into offloaded phases: prepare (extract + validate) and commit (stage + move) run via asyncio.to_thread, the tempdir lifecycle is offloaded, and the security scanner's file enumeration and reads move off the loop — only the per-file LLM scan (genuinely async) stays awaited. Security decision logic and exception contract are unchanged. Anchor: tests/blocking_io/test_skills_install.py drives the real install pipeline (real .skill archive, real FS; only scan_skill_content stubbed) under the strict Blockbuster gate. Verified red on pre-fix code (BlockingError: os.stat), green with the fix. * fix(skills): log temp-dir cleanup failures instead of swallowing them Review follow-up on the install offload: rmtree(ignore_errors=True) kept the primary install exception but silently leaked the extraction dir on cleanup failure. Keep the never-mask behaviour, add a warning log. * fix(skills): bound install tmp cleanup and pass skill_dir explicitly (review) - Wrap the best-effort temp-dir cleanup in asyncio.wait_for (5s) so a hung filesystem in the finally block cannot stall or mask the install outcome; timeout is logged like the existing OSError path. - Hoist _collect_scannable_files to module level with skill_dir as an explicit argument instead of a closure capture. |