* fix(skillscan): keep secret literals bound by kwargs, defaults and walrus in view
#5648 replaced the line-oriented `name[:=]value` sweep with an AST walk that
only inspected Assign/AnnAssign. A keyword argument, a parameter default and a
walrus all still read as `name=value` to that sweep, so moving an embedded
credential into a call silenced a HIGH-severity rule.
* fix(skillscan): fold constant-built secret values back into the AST scan
The pre-#5648 text sweep reported `API_KEY = "sk-" + "a1b2c3d4e5f6"`,
because its value capture stopped at the first closing quote of `"sk-`
and the remainder was never examined. Splitting the quotes does not
change the bound value, but `ast.Constant` alone now requires it to:
`+`, an adjacent literal run, and a placeholder-free f-string all bind
a compile-time constant that the HIGH-severity rule no longer sees,
while the same bytes in a file Python cannot parse still trip the text
fallback. `_python_secret_literal` folds those forms, so the AST pass
stays a precision-only change for values as well as binding forms.
Runtime-composed values (`os.environ[...] + "…"`, `%`-formatting) stay
unreported, which is the precision #5648 was after.
* fix(skillscan): fold a literal chain without reaching the recursion limit
The constant fold recursed once per operand, so a concatenation of ~1000
literals raised RecursionError on input ast.parse accepts. That exception
escaped past the per-file analyzer guard in scan_skill_dir, which drops every
finding collected for the file, so the deep chain cost the file its other
findings as well. The fold is now a stack walk that collects operands in
source order for either nesting, and the file's remaining rules keep reporting.
* fix(skillscan): report credential defaults on lambda parameters
_python_secret_bindings walked parameter defaults only for FunctionDef and
AsyncFunctionDef, so a credential moved into a lambda default (positional or
keyword-only) escaped the secret-env-assignment gate even though the
line-oriented sweep this rule replaced reported it. ast.Lambda exposes the same
ast.arguments structure, so add it to the tuple and pin both spellings.
Addresses review feedback on #5691.
---------
Co-authored-by: sxh313 <sxh313@users.noreply.github.com>
* fix(skills): resolve the user-scoped install scan from its own config
UserScopedSkillStorage.ainstall_skill_from_archive re-implements the parent
body to redirect the install target into the per-user custom root, and the
copy dropped the app_config argument. _scan_skill_archive_contents_or_raise
therefore fell back to the process-global get_app_config() for the static
scan, while the archive preflight it inherits still read self._app_config --
so the two gates of a single install could consult different configs.
The divergence is reachable whenever the storage outlives a config edit:
DeerFlowClient snapshots get_app_config() at construction, and the Gateway
hands its per-request get_config() to get_or_new_user_skill_storage, while
get_app_config() hot-reloads a later edit of config.yaml. With the storage's
config enabling skill_scan and the global disabling it, the preflight ran,
the content scan silently skipped, and a CRITICAL archive installed.
* fix(skills): thread app_config into the per-file LLM scan
_scan_skill_archive_contents_or_raise takes app_config and uses it for the
static gate, but never forwarded it to _scan_skill_file_or_raise, so the
per-file LLM scan fell back to the process-global get_app_config() for
skill_evolution.moderation_model_name and the model it constructs. Under
the stale-snapshot scenario the content-scan fix addresses, the two halves
of one install still read two different configs.
Raised in review of #5703 by willem-bd. The gateway's own skill-write
routes already pass app_config to scan_skill_content; this brings the
install path in line.
* fix(skills): install for a user keeps its custom-dir setup off the event loop
UserScopedSkillStorage.ainstall_skill_from_archive re-implemented the install
pipeline and created the per-user custom directory inline, so the await behind
POST /api/skills/install blocked the Gateway loop on os.mkdir. The base class
offloads every filesystem phase around its LLM scan — and says so — but the
override's setup step sat above that comment and escaped it, and the existing
blocking-IO anchor drove only the host-scoped class.
Offload the mkdir through the same worker thread and anchor the override itself
under the strict Blockbuster gate.
* test(blocking-io): pin per-user staging of supporting skill files
Review follow-up on #5650: the user-scoped anchor asserted only the staged
SKILL.md, so a regression that mis-stages the references/ tree under the
per-user root would pass both anchors. Assert the supporting file against the
per-user layout as well, mirroring the host-scoped twin.
* fix(skills): render an empty allowed-tools as no tools, not as all
`_render_skill_metadata` truthiness-tested `Skill.allowed_tools`, so an
explicitly empty allowlist (`allowed-tools: []`, parsed to `()` by
`parse_allowed_tools`) rendered "Allowed tools: (all)" -- the same text an
omitted field (`None`, unrestricted) produces. `allowed_tool_names_for_skills`
distinguishes the two and strips every business tool for `()`, so the
describe_skill output contradicted the policy applied to the same skill.
* docs(skills): scope the rendered (all) to the skill's own declaration
Review note: allowed_tool_names_for_skills makes a legacy None skill
contribute no tools once any loaded skill declares allowed-tools, so
"(all)" on that line reports the frontmatter of this skill rather than
the tool set the middleware will allow in a mixed set. Say so where the
tri-state is rendered; behaviour is unchanged.
* 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.
* 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>
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>
* 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.
* 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>
* 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>
* 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
* fix(skills): fail closed on drifted projection namespace on all platforms
* test(skills): add regression test simulating swallowed unlink on drifted namespace
* 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>
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.
* 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>
* 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>
Normalize YAML frontmatter keys in the shared parser so validation and review report malformed fields instead of failing while sorting mixed key types.
* 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.
* 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>
* 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
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.
* 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.
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.
_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.
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>
* 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.
* 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.
* fix(provisioner): gate legacy skills mount by user visibility
* fix(test): aio sandbox provider
* fix: use shared legacy skill visibility helper for sandbox mounts
* 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.
* 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>
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>
* 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>
* 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>
* 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>
