mirror of
https://github.com/bytedance/deer-flow.git
synced 2026-11-11 13:30:37 +00:00
12 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
trent87
|
0d73798beb
|
feature(skills): keep allowed-tools as compatibility metadata (#6935)
* refactor(skills): keep allowed-tools as compatibility metadata * docs(skills): align current guides with agent tool authority * test(skills): simplify authority regressions and sync Korean docs |
||
|
Totoro
|
92e9afe6ea
|
feat(context): support client context capabilities (#7051)
* feat: support client context capabilities (cherry picked from commit b7c223f9415dcd545cbe05a5f5a6e47073a42d68) [Totoro-qaq: resolved onto main. The persisted-reminder middleware hunks and their tests are dropped (delivery is redesigned in a follow-up); the RunCreateRequest description moved to app/gateway/run_models.py.] * fix: address client context review comments (cherry picked from commit bce6da6b819228eda58e887872720b6f9b5fab69) [Totoro-qaq: middleware hunks dropped with the redesigned delivery.] * feat(agents): deliver client context as a request-scoped data block The cherry-picked #3106 put context.client into the persisted first-turn reminder, with change detection and a "not provided" clear marker. Main has since moved the date reminder to a SystemMessage, made request-scoped context (the project block, #5443) request-only, and agents/AGENTS.md now requires user-influenced values to ride the HumanMessage data channel, never system text. Render runtime.context["client"] on every model call in DynamicContextMiddleware.wrap_model_call/awrap_model_call as one hidden HumanMessage right after the leading SystemMessages, where DurableContext puts its data. It is never a state update and never flagged dynamic_context_reminder, so nothing is checkpointed, summarized or replayed, and a run without a client gets no block. An unchanged client keeps the previous request as the next request's prefix. The renderer sanitizes again (direct graph callers bypass the Gateway), html-escapes values and frames them with frame_untrusted_text: the same --- BEGIN/END USER INPUT --- boundary the user's own turn and the MCP task notification get. The lead prompt treats everything outside those markers as framework data, so unframed values would rank above the user's own text; framed, context.client can say nothing the same caller could not already say in input.messages. client_context joins _BLOCKED_TAG_NAMES so a forged block in user input or a tool result is escaped, and one static, value-free sentence in the lead prompt names the block as untrusted hints, never instructions, permissions or identity. The now unused empty-context marker is removed. * fix(gateway): log dropped context.client fields without values A run request is never rejected over context.client: unknown fields, nested values and malformed entries are silently dropped, the same leniency as the other context keys. That left no signal when a custom client sent something the Gateway ignored (review on #3106). Route both the config.context.client and the body context.client paths through one helper that emits a debug line whenever sanitizing changed the value. Like the malformed client_timezone branch, the line carries no values; unlike client_timezone, client.name and preference strings are free caller-controlled text, so they are never logged, not even sanitized. A clean client, an explicit null or an absent key logs nothing. Tests also pin the config-only path (sanitized before the early return) and the end-to-end start_run admission: the sanitized client reaches config["context"] and never configurable. * fix(runtime): bound numeric client context preferences String preferences are capped at 200 characters, but integers were kept at any size. A JSON body may carry integers of up to 4300 digits, so 32 of them rendered a block of about 138,000 characters, re-sent on every model call of the run. In-process callers could also pass an int past the str() digit limit, which failed the model call, or a number subclass with its own __str__. Keep integers within JavaScript's Number.MAX_SAFE_INTEGER, the range a browser client can send exactly, and coerce numbers to plain int and float. * docs(api): describe client context as advisory request-scoped hints The cherry-picked section described a hidden reminder that no longer exists. Document what a custom client can rely on: the allow-list and its limits (strings, the integer range, and only the first 32 entries of each map), debug-level logging of dropped fields, runtime-only forwarding, and delivery as a per-request user-role block, framed like user input, rather than system text or thread history. Only top-level context.client is documented. An explicit config.context.client is sanitized for the run as well, but the run record keeps the caller's config as sent, so it is not offered as an alternative. It also answers the #3106 review: capabilities and preferences are advisory hints, not enforcement, which belongs outside the model; name is informational and never for authorization; the same object should be sent on every run to keep the cached prefix stable; subagents and embedded DeerFlowClient runs do not receive it, and server-started runs (schedules, IM channels, GitHub agents, MCP notifications, extensions unless the plugin passes one) carry none. * fix(context): declare client delivery and keep client out of configurable The request-only <client_context> block is model-visible, so DynamicContextMiddleware now declares its delivery and the sanitizer's limits in release_policy_parameters(); runs before and after this change no longer share a policy fingerprint. build_run_config copies body.config['configurable'] verbatim and configurable is persisted with checkpoints, so a client placed there is dropped instead of being stored raw. Client context stays runtime-only. Document context.client in the README and in the middleware guide. * fix(context): apply the PII redaction policy to the client block DynamicContextMiddleware adds the <client_context> message inside the outer PiiRedactionMiddleware, and the hidden message is not re-sanitized, so with pii_redaction enabled a preference such as an email address reached the model verbatim while the same address in the user turn was redacted. Pass the rendered block through the shared redact_text seam with this middleware's app_config.pii_redaction, and declare the setting in release_policy_parameters(). * fix(context): keep client hints out of stored run config --------- Co-authored-by: Blair Leng <25675774+BlairLeng@users.noreply.github.com> Co-authored-by: Totoro-qaq <279883115+Totoro-qaq@users.noreply.github.com> |
||
|
Hyeonsang Cho
|
16b4fcd968
|
fix(skills): keep a slash skill activation when the model call is retried (#6506)
* fix(skills): keep a slash skill activation when the model call is retried SkillActivationMiddleware records the activated slash message in the run context before it calls the model. LLMErrorHandlingMiddleware sits outside it and retries a failed (429/503/timeout) or empty call by running this wrap again on the same request; the retry found the run key already set and went out without the SKILL.md body, while the slash source it had published kept the skill's tool restrictions in force. Record the activation itself next to the run key and replay it while nothing has answered the slash message yet: a retry sees no AI message after the target, a tool-loop follow-up call does. The replay carries the same reminder and usage stamp without re-reading the skill or repeating the audit event and usage record, which now sit with the other once-per-activation side effects. Two existing tests sent the same unanswered request twice to model a follow-up call; that is the retry shape, so their second call now includes the first response, as the real graph does. * docs(changelog): reference #6506 --------- Co-authored-by: Willem Jiang <willem.jiang@gmail.com> |
||
|
Wenchao An
|
beaae22088
|
feat(extensions): add host-bound Agent run control (#6190)
* feat(extensions): add host-bound Agent run control * fix(extensions): preserve internal launches and scope run control * docs(extensions): use a plugin-local run operation key |
||
|
Wenchao An
|
4c05e88c52
|
feat: add inline @ references with multiple skill activation (#6063)
* feat(frontend): unify composer references with an @ picker * feat: edit references inline and activate multiple skills * docs: describe inline references and multiple skill selection * fix: prevent sent drafts from returning and repair CI checks * fix: reconcile inline references and address review feedback * fix: preserve references through discovery errors and polish |
||
|
hataa
|
1cd3eb742b
|
feat(authz): enforce skill authorization at assembly and slash-activation (#4063 Phase 3) (#4541)
* feat(authz): enforce skill authorization at assembly and slash-activation (#4063 Phase 3) Phase 3 / Skills — the second of three resource-type PRs (Models, Skills, Sandbox). The RBAC provider already maps "skill" → config key "skills" (rbac.py _RESOURCE_POLICY_KEYS), so no schema change is needed. Layer 1 (assembly-time, mirrors Phase 1B): - filter_available_skills_by_authorization() in skill_filter.py filters the skill-name allowlist (set[str] | None) by the provider's "skill" policy. In lead agent, runs after _available_skill_names() so the catalog (describe_skill) and SkillActivationMiddleware share one filtered set. In subagent executor _load_skills(), filters config.skills before disk load. - filter_resources_by_authorization() in enforcement.py — generic version of filter_tools_by_authorization for any resource with a name attribute. Layer 2 (slash-activation): - No new middleware code needed. SkillActivationMiddleware._resolve_activation already checks "reference.name not in self._available_skills". Since Layer 1 filters the allowlist, denied skills cannot be slash-activated. authorization.enabled: false is a complete no-op. available_skills=None (no agent allowlist) + enabled: resolves all skill names from config and filters. 12 new tests + 243 existing authz/skills tests pass. * test(subagents): wait on the chained future instead of polling done() in caller-loop teardown test * feat(authz): enforce action-scoped skill:activate at runtime and reuse the catalog loader for filter candidates * feat(authz): arm skill:activate on the subagent chain, unify the client skill-surface user, and record the Layer 2 reversal in the design notes * chore(authz): align the subagent filter's fallback user with the chain's bucket convention * feat(authz): gate autonomous skill loading on skill:activate, close the bootstrap assembly leak, and reuse one provider per subagent assembly Review round 4 (willem-bd, CHANGES_REQUESTED): - [P1] Autonomous load path: describe_skill filters its match set on the action-scoped skill:activate decision (all-denied is indistinguishable from "no match"), and ToolErrorHandlingMiddleware stamps skill_context entries only when the action allows it — a denied SKILL.md read gets a skill_context_denied marker instead, so durable context, allowed-tools policy, and autonomous secret bindings never activate a denied skill. File visibility itself stays Layer 1 (sandbox-projection semantics). - [P1] Bootstrap assembly: build_middlewares/apply_prompt_template now receive the authorization-filtered set instead of the raw _BOOTSTRAP_SKILL_NAMES, and an empty skill_names index is preserved when deferred discovery is on rather than round-tripping through None into the legacy full-metadata prompt that re-advertises a denied skill (deferred-off keeps the legacy rendering; the filtered empty allowlist blanks it on denial). - [P2] SubagentExecutor resolves ResolvedSkillAuthorization once per assembly (_resolve_skill_authorization, memoized per executor) and shares it across the Layer 1 filter (authorization=), the activation middleware, and both new gates — uncached provider factories can no longer hand the layers different instances/policy snapshots. All gates share one helper (skill_filter.skill_activation_allowed) so slash, describe, and skill-file-load cannot drift; provider errors keep the configured fail-closed/fail-open policy. skill_authorization is threaded through _build_runtime_middlewares into both runtime builders. Tests: describe gating (deny/fail-closed/fail-open/disabled), read stamping (denied marker vs entry) + extract_skills skip without warning, bootstrap three-way param (allow/deny x deferred on/off), lead + subagent chain wiring for the stamp gate, and the distinct-instance provider factory repro. Each fix is mutation-verified (reverting it fails its test). Full A/B suite run: no new failures vs HEAD. * feat(authz): re-authorize persisted skill_context before reuse and route async paths through aauthorize Review round 5 (willem-bd): - [P1] Persisted entries: skill_context survives across runs, so both consumers now re-check the action-scoped skill:activate decision before reuse. SkillToolPolicyMiddleware._active_skills_for_paths skips a denied skill (same skip as a disabled/unallowlisted one; all-denied keeps the existing fail-closed-to-builtins semantics) and _in_context_secret_sources binds no secrets for a denied entry. skill_authorization is threaded into SkillToolPolicyMiddleware on both chains. - [P2] Async provider API: skill_activation_allowed_async mirrors the sync helper through await provider.aauthorize() (same pattern as authorize_sandbox_execution_async). Async hooks precompute an activation_decisions map on the event loop (slash target + persisted entry names, one batch of awaits) and hand it to the worker-thread handler, which consults the map instead of the sync API: SkillActivationMiddleware.awrap_model_call (activation + secret binding), ToolErrorHandlingMiddleware.awrap_tool_call (_amaybe_stamp awaits the decision before stamping), and SkillToolPolicyMiddleware's awrap_model_call/awrap_tool_call (entry names + slash source name). describe_skill becomes a StructuredTool with both a sync function (invoke -> authorize) and a coroutine (ainvoke -> aauthorize), so async agent execution never routes a loop-affine provider through the sync API while sync callers keep their semantics. Tests: allow-read-then-deny-next-run regressions for secret binding and allowed-tools (partial deny does not poison, all-denied fails closed to builtins), and an _AsyncOnlyProvider (sync authorize always fails, aauthorize works) driving four async-path assertions (activation, stamp allow/deny, describe, tool policy). All six fixes are mutation-verified; full A/B suite run against HEAD shows no new failures. * fix(authz): authorize skill activation by declared Skill.name, not directory basename ShenAC-SAC's P2: bundled skills may declare a name that differs from their directory (skills/public/vercel-deploy-claimable declares name: vercel-deploy), but the runtime gates derived authorization targets from the container path. - New deerflow/skills/container_registry.py: build_container_path_registry() + canonical_skill_name() — the one path-to-Skill.name resolution shared by the activation, tool-policy, and read-stamping middlewares. - ToolErrorHandlingMiddleware resolves the read path through the (user-scoped, via new user_id kwarg threaded from both runtime builders) registry before the skill:activate decision, sync and async (registry lookup in a worker thread, aauthorize() on the loop). RBAC allow by the declared name now activates; allow by the directory name does not. - SkillToolPolicyMiddleware async hooks canonicalize the policy paths first (three-phase: thread registry -> loop aauthorize -> thread filter reusing the registry; the cached per-model-step decision still short-circuits), so the decision map is keyed by the names _active_skills_for_paths checks — no more map miss falling back to the sync provider API from the thread. _entry_names removed: persisted entry["name"] is path-derived by construction; entries are resolved by path instead. - SkillActivationMiddleware collects slash names (already canonical) plus persisted entry *paths*, canonicalized off-loop via _canonical_names_for_paths(). Regressions (test_skills_authorization.py 45->50): sync stamp target, RBAC allow declared-name/deny directory-name both directions, async stamp via _AsyncOnlyProvider, composed async slash -> tool-policy keeps the skill's tools with zero sync fallback, persisted-entry secret binding survives the name mismatch. All three mutations (stamp target, policy map key, entry canonicalization) fail their tests. * fix(authz): close the sync-fallback class on skill activation paths willem-bd's two P2s on the canonical-name fix, plus a fourth instance of the same class found while sweeping the membership: - SkillToolPolicyMiddleware: a transient prepass registry failure is now preserved (_REGISTRY_LOAD_FAILED marker) — the worker-side filter applies its own fail-closed treatment instead of silently retrying storage, which recovered skills absent from the decision map and fell back to the synchronous provider API from the thread (fail-open retaining a denied skill's allowed-tools). - SkillActivationMiddleware: an empty entry-path list no longer pays a full skill-tree scan on ordinary authorization-enabled async model steps (previously a passive path with zero registry I/O). - 4th instance (reproduced with the PR fixtures, then fixed): the entry prepass failure on the secret-binding path let the consumer-side _load_skill_registry_by_path silently recover and resolve entry names outside the decision map — the same sync fallback, fail-open binding the aauthorize-denied skill's declared secret. The fix goes one step further than failure propagation: the prepass hands down the registry *snapshot* the decision map was keyed by, and entry sources resolve against that snapshot, so a map miss is impossible by construction (this also closes the narrower window where both loads succeed but disagree because storage changed mid-step). The slash source is deliberately unaffected: it does not consult the decision map (validated once at activation) and keeps its fresh per-call load. - Class membership swept: skill_activation_allowed() sync call sites are exactly four (stamping sync path and describe's sync implementation are legitimately sync); the only two async-reachable fallback points (the _activation_allowed helpers) are both construction-guarded now. Structural hardening (typed async-batch decision object where a map miss is fail-closed by construction) recorded in the plan notes as a follow-up, anchored at those two helpers. - _RegistryArg type alias replaces the degenerate dict|object|None union in the policy middleware signatures. Tests (test_skills_authorization.py 50->54): transient prepass failure -> builtins-only with exactly one storage attempt and zero provider calls; ordinary async step -> zero registry scans; two-call scenario -> entry binding zeroed while the slash binding survives; entries resolve against the prepass snapshot under a mid-step rename. Mutations verified red: marker reverted to None, over-broad slash kill, snapshot bypass. * fix(authz): correct secret-binding reference points and gate the rendered skill reminder Review round 9 (willem-bd) plus construction-audit follow-ups on the same class — every async divergence between two loads of the same data now has an explicit, pinned reference point: - Slash secret source resolves from a FRESH post-activation registry: the prepass snapshot is taken before the aauthorize awaits, so binding the slash source from it would activate NEW_KEY content while injecting OLD_KEY when a skill's declared secrets change in that window. The 'one scan per step' bonus that motivated sharing the snapshot is withdrawn — consistency wins. The two secret sources resolve independently: a transient fresh-load failure zeroes only the slash binding; entries keep binding from their own snapshot. - Same-skill dominance: when a skill is both the slash source (fresh) and a persisted entry (snapshot), only the activation-era declaration binds — unioning both eras injected a secret the skill no longer declares. Aligns with the tool-policy rule (slash activation dominates for the run); different-skill entries still stack additively. - The rendered 'Active skills' reminder (DurableContextMiddleware) no longer advertises skills the provider denies — surface consistency with describe_skill. Publication/consumption split: the activation middleware publishes per-step path-keyed skill:activate decisions into the run context (async: reused from the prepass snapshot, zero extra I/O; sync: one scan + authorize() only when entries exist — passive steps stay scan-free), the renderer consumes them and filters its render copy (state untouched; unpublished = permissive, matching the other run-context carriers). New carrier key listed in REDACTED_CONTEXT_KEYS with a redaction regression. - Wiring regressions on both chains for everything added above (user_id to the stamp gate, skill_authorization + token pairing + activation- before-durable ordering to the durable renderer): each mutation verified red (M4-M13 across this change set). - Tests 54->65: slash post-activation lookup, source independence, dominance, sync-chain sync-API path, render filter (async/sync/ disabled with load-count assertions), chain wiring x2, redaction listing. Plan notes record rounds 9-2..6 including the reference-point post-mortem and the audit axes exhausted. * fix(authz): anchor same-skill dominance on the slash identity, not binding results Review round 10 (willem-bd) plus a self-audit extension the new checklist items caught in the first fix itself: - R10 (P2): the same-skill exclusion was derived from successfully BOUND slash sources. When the prepass snapshots required-secrets [OLD_KEY] and the operator strips the declaration during the await window, the slash source binds nothing (required_secrets gate) — the exclusion set stayed empty and the stale entry view kept injecting OLD_KEY for a skill that now declares no secrets. The exclusion now derives from the AUTHENTICATED slash activation identity (run-context source path, owner-token checked): the identity's declared name resolves from the fresh registry first, then the entry snapshot, and joins the exclusion set regardless of binding success. - Self-audit (value->gate->downstream on the fix itself): the declared name can also change mid-run (same path, foo->bar). A name-anchored exclusion then holds only the new name while the snapshot entry resolves the same path to the old one — both eras bound again. The PATH is the stable identity shared by the activation and every earlier read: entries at the slash identity's path are now excluded before registry resolution (exclude_paths), independent of what either version calls the skill; the name-based set remains for cross-path same-name shadowing. Tests 65->67: no-secrets-era dominance (ACTIVE secrets None under the OLD_KEY-to-empty interleaving), path-anchored dominance across a mid-run rename (only the activation-era key binds). Mutations verified red: identity-derived exclusion disabled (M14), path exclusion disabled (M16). * test(authz): pass explicit config to lead middleware wiring test --------- Co-authored-by: Willem Jiang <willem.jiang@gmail.com> |
||
|
Hyeonsang Cho
|
9146bfa03d
|
feature(gateway): issue request trace ids unconditionally (#5119)
* refactor(gateway): issue request trace ids unconditionally The request trace id was gated behind logging.enhance.enabled at every entry point, so downstream code had to keep asking whether one existed: a header-provenance flag in its own ContextVar, a precedence resolver, and three-level carrier fallbacks at each consumer. Bind one unconditionally instead. TraceMiddleware covers Gateway HTTP; ensure_trace_context covers the entry points that never touch ASGI -- scheduled occurrences, MCP task notification runs, IM channel messages, and the embedded client -- each scoped to one unit of work so a long-lived worker task cannot leak one occurrence's id into the next. The ContextVar becomes the only source; the response header, runtime context, run metadata and log records are derived outputs. Consumers now use ensure_trace_id() or resolve_trace_id(*carriers) and drop their presence guards. Removed: resolve_deerflow_trace_id, the header-provenance flag and its three helpers, set/reset_current_trace_id, is_trace_correlation_enabled and its gateway alias. BREAKING CHANGE: every Gateway HTTP response now carries X-Trace-Id and it cannot be turned off; logging.enhance.enabled controls log output only. Installations on the default enabled: false will start seeing the header. No config keys were added or removed. * fix(gateway): stop persisting a caller-supplied trace id on the run record body.metadata forks two ways: through build_run_config into the live run config, which the run worker restamps, and through create_or_reject into the run record that the runs API echoes verbatim. Only the first was covered, so a client sending metadata.deerflow_trace_id made the most durable and most visible surface of a run disagree with the X-Trace-Id and the log lines the same request produced -- a correlation id that does not match the logs is worse than none. Stamp the server-issued id once at the trust boundary so both forks receive it, preserving the caller's own metadata keys. Close the same gap on config.context, which reaches the runtime context by a separate path: _build_runtime_context no longer merges server-owned keys from the caller, and _install_runtime_context assigns rather than setdefaults. A thread's metadata is no longer seeded with the run-scoped id of whichever run created it -- one thread spans many runs and as many trace ids. Found by driving a real run through the Gateway and reading the run back from the runs API; every unit test built its metadata by hand and so could not see it. * fix(gateway): expose X-Trace-Id to split-origin browser clients X-Trace-Id is not on the CORS safelist, so a browser client served from a separate origin could not read it -- and those are exactly the clients that cannot read the Gateway's logs either, leaving them with nothing to quote in a bug report. Same-origin nginx deployments were unaffected, which is why this stayed hidden. Add it to CORS_EXPOSED_HEADERS beside Content-Location, referencing TRACE_ID_HEADER rather than repeating the literal. * fix(gateway): keep X-Trace-Id on unhandled-exception 500s Starlette's ServerErrorMiddleware sits outside every user middleware and emits unhandled-exception 500s through the raw send, so those responses never pass TraceMiddleware's header-writing wrapper. The 500 for a server bug is exactly the response a user most needs to correlate with a log line, and it was the one response that shipped without the id. TraceMiddleware now tracks whether http.response.start has been sent. On an exception with no response started it emits its own plain 500 carrying the header, then re-raises: the outer ServerErrorMiddleware sees the response already started and only re-raises too, so the server's exception logging is untouched. An exception mid-stream keeps propagating unchanged — a second response start cannot be sent, and the already-written header stands. The trace id is printable ASCII by construction (normalize_trace_id / generate_trace_id), which is what makes the raw latin-1 header encoding safe. * fix(gateway): strip the forged trace id from the persisted request echo The run-record fix stopped a forged metadata.deerflow_trace_id on the authoritative metadata surface, but the raw request echo still carried one: create_or_reject persists body.config verbatim as runs.kwargs_json, which the runs API serves back. A client posting config.context.deerflow_trace_id therefore still got its forged value stored and echoed on one API surface while the header, logs, run metadata, and checkpoint all carried the real id — the id is ignored as input there, so echoing it back only manufactures disagreement. Two changes close it. redact_config_secrets — already the shared scrub for that echo, applied at admission and again at serve time, so historical records are covered too — now also drops deerflow_trace_id from config.metadata and config.context. And build_run_config now merges run metadata onto a copy of the caller's config["metadata"] instead of updating it in place: the nested values of the request config are reference copies, so the in-place merge was writing the server-stamped key through into body.config, contaminating the "what the client sent" record before it was persisted (and incidentally masking the forged-value echo on the metadata container). The regression test posts a forged id through body.metadata, config.metadata, and config.context at once and reads the kwargs echo back off the run record, failing if either leak returns. * docs(harness): record the trace-echo scrub, 500 fallback, and accepted retry divergence The trace section of the harness AGENTS.md now covers the two fixes that close the derived-output rule (the kwargs-echo scrub in redact_config_secrets plus build_run_config's copy merge, and TraceMiddleware's own 500 for unhandled exceptions), and CHANGELOG gains their Fixed entries. It also writes down the one accepted divergence: a crash-recovered scheduled launch reuses the durable run through its idempotency key, and start_run returns early on idempotency_reused without restamping — so the run record keeps the first attempt's deerflow_trace_id while the retry's own log lines carry the freshly minted id of its ensure_trace_context binding. The divergence is confined to the crash-recovery window and is accepted rather than fixed: restamping on reuse would rewrite a persisted record for a run that already exists, which is worse than two ids that each correlate their own attempt's logs. Written down so the next reader of the scheduler recovery path does not diagnose it as a bug. * docs(config): align the logging.enhance schema note with the unconditional trace id The config-module AGENTS.md still described logging.enhance as the gate for the Gateway X-Trace-Id header and Langfuse deerflow_trace_id. That model is gone: ids are issued unconditionally and this block decides log output only. Left as-is, the stale wording invites an agent to "restore" a header gate it believes was lost. Reworded to match the sibling AGENTS.md files and config.example.yaml, with a pointer to the Request Trace Context section that owns the full model. * docs(changelog): link the trace entries to #5119 The five new entries pointed at the ([#XXXX]) placeholder with no reference definition, rendering as literal text instead of a link — and RELEASING.md step 2 relies on those references when the section becomes release notes. All five now point at #5119, with the definition appended to the reference block. * refactor(harness): rename _stream_without_trace_context to _stream_turn The name asserted the opposite of what the method now does. It was accurate while logging.enhance.enabled could route stream() around the trace scope; with the gate gone it is the only stream implementation left, and it binds the id itself via ensure_trace_id(). Private, so the rename touches only the definition and the one stream() call site. * docs(harness): fit the trace-context guidance inside the AGENTS.md chain budget The expanded Request Trace Context section pushed the effective AGENTS.md chain for agents/middlewares to 99,815 bytes, past the 98,304 hard limit scripts/check_agent_guidance.py enforces in CI (AG002). Compressed the section from 7,359 to 4592 bytes with no facts removed: the entry-point table, the derived-output rule and its enforcement points, the accepted scheduled-retry divergence, the two resolution helpers, the stream() binding rationale, the log-output-only gate, the CORS listing, the 500 fallback, and the test map all remain. Sized against the merge, not just the branch: current main grew the same chain by ~724 bytes, so the check was verified on the merged tree as well (97,772 bytes; branch tree 97,048). * fix(gateway): declare content-length on the fallback 500 The pre-response 500 declared content-type but no content-length, leaving the framing to the ASGI server: chunked on HTTP/1.1, close-delimited on HTTP/1.0 — the one wire difference from the ServerErrorMiddleware response it replaces, which sends content-length: 21. The explicit header keeps the fallback byte-identical to what clients saw before. * docs(readme): drop the trace-correlation condition from the translations The zh/ja/fr/ru Langfuse sections still said metadata.deerflow_trace_id matches X-Trace-Id "when request trace correlation is enabled". The id now always matches and that condition no longer exists, so each bullet states the unconditional match and that logging.enhance.enabled only controls whether the id is printed into logs — the one piece of the feature a user can still configure. * test(gateway): pin TraceMiddleware wiring through create_app() Every X-Trace-Id test exercised a hand-built four-route app, so the real stack's add_middleware(TraceMiddleware) line was pinned by nothing: deleting it — or short-circuiting above it — passed CI while silently dropping both the response header and the ambient id the run-record stamp and enhanced log records derive from. One case now drives /health through create_app() and asserts the inbound id round-trips; mutation-checked by removing the wiring line, which fails exactly this test. * docs(gateway): note the fallback 500 is CORS-opaque The pre-response 500 is emitted outside CORSMiddleware — the exception has already unwound past it — so it carries no Access-Control-Allow-Origin and a split-origin browser client cannot read the id on this one response, unchanged from the ServerErrorMiddleware 500 it replaces. Documented on the class and in the CHANGELOG entry rather than fixed: replicating the origin allowlist outside CORSMiddleware would let the two policies drift. * fix(harness): keep abandoned-stream cleanup inside the trace binding stream() binds the turn's id around each next(inner) and resets it before yielding, but the finally's inner.close() ran after that binding was gone. Abandoning the stream therefore drove the inner LangGraph generator's GeneratorExit/finally path with no trace id — or an unrelated ambient one from whichever context ran the close — so cancellation and finalization logs and callbacks did not correlate with the turn they belong to. inner.close() is now wrapped in a local bind/reset of the same turn id. The token is set and reset in the same frame, never across a yield, so the per-step cross-context safety is preserved even when GC closes the generator from another Context — pinned by the existing copy_context close test, which now exercises this path. The regression test records the id from the inner generator's finally and fails without the binding. * test(harness): teach the worker-trace fake about RunManager.cleanup Upstream #5112 (bound gateway memory after terminal runs) added a run_manager.cleanup(run_id) call to run_agent's finalization, so the merge-commit CI run failed all five worker-trace-binding tests with AttributeError on this PR's _FakeRunManager. The fake gains the same no-op shape as its other methods. * docs(gateway): bring the gateway AGENTS.md back under its soft budget Upstream #5092 grew backend/app/gateway/AGENTS.md to 40,966 bytes, 6 over the 40,960 soft budget that test_agent_guidance_check.py::test_repository_guidance_stays_below_soft_budgets_and_avoids_doc_indexes enforces — its Unit Tests run on main was cancelled by push concurrency, so main is currently red on that test and every PR merge-run inherits the failure. Two whitespace/wording trims in the row #5092 touched (a doubled space, and "its configured `context_window`" → "its `context_window`") bring the file to 40,953 with no content change. --------- Co-authored-by: Willem Jiang <willem.jiang@gmail.com> |
||
|
ShitK
|
b1984cf4ab
|
fix(security): reject legacy MCP credentials in run metadata (#4448)
* docs: design run metadata secret admission * docs: refine run metadata secret boundaries * docs: plan run metadata secret fix * fix(security): centralize legacy run metadata policy * fix(security): reject secrets at run admission * fix(security): hide legacy secrets from history APIs * docs(security): migrate MCP credentials to secret context * fix(security): redact legacy runnable config metadata * fix(security): reject legacy config metadata credentials * fix(security): hide legacy secrets from run kwargs * docs(security): clarify config redaction boundary * docs: keep issue 4416 planning local |
||
|
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 |
||
|
Daoyuan Li
|
2fa0505070
|
fix(skills): activate a slash skill once per run, not per model call (#4103)
* fix(skills): activate a slash skill once per run, not per model call SkillActivationMiddleware injects the activation reminder for a slash command via request.override(messages=...), which LangChain's create_agent uses for a single model call and never writes back to graph state. The dedup guard scans request.messages for a prior reminder, but model_node rebuilds request.messages fresh from persisted state on every tool-loop step, so the reminder is never present on the 2nd..Nth model call of a turn. Every model call therefore re-parsed the command, re-read SKILL.md from disk, re-injected the multi-KB body, and re-recorded an "activate" audit event, despite the code intending a single activation per run (#3861 semantics: one activation call, many follow-up model calls). Key the dedup off the run context instead, which LangGraph threads through every model-node call of a run (the same durable signal the request-scoped secret source already uses). The activation call records the slash message's identity in context; later calls for the same message skip re-activation. A new user slash message keys differently and still activates. Secret binding is unaffected: it already re-resolves from the persisted slash source on every call. Adds regression tests that rebuild the real multi-call turn state and assert a single activation across the tool loop, plus a test proving a new slash command still activates. * fix(skills): address review nits on run-scoped activation dedup - Extract _already_activated(run_context, run_key) so the dedup check mirrors the existing _has_existing_activation_for_target sibling instead of an inline dense conditional. - Compute _activation_run_key() once in _find_activation_target and thread it through _prepare_model_request instead of recomputing it at the write site, making the "same key for check and write" invariant explicit in the code rather than implicit. - Document why the run-context write is an overwrite rather than an append/set: only the latest real user message is ever considered an activation target, so there is nothing earlier in the run worth preserving. - Add a regression test locking in the degraded-path contract: when runtime.context is None, the middleware still activates per-call instead of crashing or wrongly no-op'ing. |
||
|
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> |
||
|
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. |