mirror of
https://github.com/bytedance/deer-flow.git
synced 2026-09-27 23:16:16 +00:00
11 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
zhangwei-way
|
b6503e9a35
|
feat(knowledge): add per-message RAGFlow retrieval scope (#5238)
* feat(knowledge): integrate RAGFlow retrieval and management * test(knowledge): cover merged listing tool * feat(knowledge): add per-message retrieval scope * chore(docs): remove unrelated document * docs(knowledge): add interaction screenshots * feat(knowledge): simplify scope selector trigger * docs(knowledge): refresh selector screenshot * feat(knowledge): defer standalone management * docs(knowledge): show chat-only scope UI * fix(knowledge): honor scope on clarification replies * fix(knowledge): harden scoped replay validation * docs(knowledge): clarify replay scope precedence * fix(knowledge): keep provider settings on tools * fix(config): preserve tools-only knowledge settings * fix(knowledge): submit custom assistant identity * refactor(knowledge): trim PR scope changes * fix(knowledge): sanitize document scope display * feat(knowledge): enable scope selection in main chat * fix(knowledge): emphasize active scope icon without button frame * fix(knowledge): close context scrubbing and refresh e2e checks * fix(knowledge): preserve idempotent canonical retries * fix(knowledge): accept promptless conversation runs * style(knowledge): format backend regression tests * chore(knowledge): trim PR scope and fix frontend format * fix(knowledge): remove shared-scope notice * fix(knowledge): remove scope persistence notice * docs(knowledge): include main chat in catalog scope * fix(knowledge): preserve scope recovery and upgrades * fix(config): preserve LightRAG knowledge upgrades --------- Co-authored-by: foreleven <for-eleven@hotmail.com> |
||
|
Nan Gao
|
cf556fa9d4
|
feat(agents): elide superseded write_file payloads from model-bound requests (#5374)
* feat(agents): elide superseded write_file payloads from model-bound requests Step 2 of #5328. After a successful write_file the file on disk is the source of truth, and the read-before-write gate forces a read_file before the next modification of that path, so once a later successful read or write of the same path exists the historical `content` argument is redundant with it. Long report-writing runs (append-in-chunks) therefore carried every section twice, once as the write argument and once as the following read output, until summarization compacted the whole turn. - ToolOutputBudgetMiddleware's model-call hooks now replace such superseded content with a short deterministic placeholder pointing at read_file, in the model-bound request only: state["messages"], checkpoints, receipts, loop detection, and the run journal keep the original arguments, and nothing is externalized to disk. The newest `keep_recent_writes` successful writes (default 1) always stay visible; str_replace payloads are never touched; a same-turn read never supersedes (parallel calls run in no fixed order); only results stamped deerflow_tool_meta.status == "success" count, so failed, gate-blocked, partial, or unstamped writes are never candidates. - New `tool_output.elide_superseded_writes` (default on), `tool_output.superseded_write_min_chars` (default 2000), and `tool_output.keep_recent_writes` (default 1); config_version 41 -> 42 in config.example.yaml and the Helm chart. - The per-occurrence call/result pairing the gate introduced in #5329 moves into the shared `tool_call_args.pair_tool_call_results` helper so both policies pair the same way; the gate now uses it. * fix(agents): scope tool-call result pairing to the issuing turn Review finding on #5374 (P2): pair_tool_call_results consumed results from a history-wide per-id queue, so an interrupted write_file with no result whose tool-call id a later turn reused inherited that later call's success. With the default elision the unconfirmed draft was then replaced by a placeholder claiming the write succeeded, and the gate's blocked-call pairing had the mirror-image hole. Pair results the way DanglingToolCallMiddleware does: walk in document order, open each AIMessage's calls, and let a ToolMessage answer only a still-open call of the most recent preceding AIMessage. A result never answers a call from an earlier turn, so the interrupted call stays unanswered (never a candidate, never labeled blocked) and stray or duplicate results are ignored. Regressions cover the helper, the superseded-write policy, and the gate. * fix(agents): never rewrite tool-call ids duplicated within one AIMessage Review finding on #5374 (P2): the policies select calls per occurrence, but every provider surface is addressed by tool-call id, so when a malformed provider payload repeats an id inside one assistant turn the rewriter could only replace all of its occurrences at once. A failed write_file sibling then took on the superseded successful call's path and elided content and was presented as a success; the gate's blocked-call elision had the mirror-image hole (a successful sibling rewritten into the blocked call). rewrite_messages_tool_call_args now never offers an id that repeats within its message to the selector and leaves those calls untouched on every surface. Both policies are covered by the shared helper; regressions cover the helper, the superseded-write policy, and the gate. * fix(agents): skip unhashable tool-call ids in the duplicate-id guard Review finding on #5374 (round 3): _duplicated_call_ids fed every id into a Counter before the string guard, so a list or dict id from a malformed provider payload raised TypeError out of wrap_model_call and failed the whole model call whenever the history also held a rewrite candidate. The pre-PR loop and pair_tool_call_results skip such ids; only this helper regressed. Count non-empty string ids only, and pin it with regressions for the helper, the superseded-write policy, and the gate. * docs(agents): keep middleware guidance within size limit --------- Co-authored-by: Willem Jiang <willem.jiang@gmail.com> |
||
|
Nan Gao
|
13f0a7f263
|
feat(extensions): let an out-of-tree extension observe what the agent did (#4863)
* feat(extensions): let an out-of-tree extension observe what the agent did
DeerFlow's extension system can contribute middleware, services and routes,
but an extension cannot answer basic questions about a run without reaching
into host internals. Several of the facts it would need are destroyed by the
operations that produce them:
* The middleware chain injects and rewrites a lot of context — date
reminders, recalled memory, compaction summaries, durable-context data,
image payloads, activated skill bodies. Downstream, none of it is
attributable: at the model-call boundary an injected HumanMessage is
indistinguishable from the user's own, and anything wanting to tell them
apart has to pattern-match prompt wording, which breaks on the next copy
edit.
* Two runs of "the same agent" are only comparable if the chain enforced the
same limits, prompts and thresholds. Recovering that from outside means
reading private attributes and guessing which of them change behaviour — a
guess that rots silently as middlewares gain fields.
* The lead-agent factory resolves a model after runtime overrides, renders a
prompt, filters tools through authorization and composes a stack, all
inside one synchronous call, and none of it survives: a middleware sees its
neighbours but not the prompt, the run worker sees a graph but not what
went into it.
* Summarization is destructive by design. N messages leave the context and
one summary enters it; afterwards only the summary exists, so "which
messages became this?" is not reconstructible.
This adds seven neutral facilities so those facts are recorded where they are
still true, and releases the contract package as 0.2.0.
Message provenance
Producers stamp `deerflow_content_kind` / `deerflow_producer_kind` onto the
messages they inject or rewrite. Stamping is unconditional — a fact whose
presence depends on whether an observer is installed is not a fact — and the
keys are server-owned, so provenance cannot be forged from a request.
Middleware self-description
Twelve middlewares declare their own behaviour-affecting parameters through
a duck-typed `release_policy_parameters()`. Long text is hashed rather than
embedded: a declaration is an identity, not a copy of the prompt.
Agent assembly descriptor
`assemble_lead_agent()` returns the graph plus a descriptor whose fingerprint
answers "did anything about this agent change between these two runs?".
`make_lead_agent()` keeps its graph-only signature — it is the LangGraph
Server ABI declared in langgraph.json. Tools and skills are sorted before
hashing because their assembly order is incidental; middlewares are not,
because stack order decides what wraps what. Host build identity is reported
but excluded from the fingerprint, so a redeploy does not invalidate every
agent's identity.
Context compaction observation
Summarization emits the content hashes of the messages it is about to remove
joined to the summary that replaced them. Content is the only identity
available at that seam: the summary does not become a message, and what later
projects it into a request renders it bounded and escaped rather than
verbatim.
Neutral policy, transform and MCP-source facts
Guardrail decisions are published to runtime context under a `__`-prefixed
key; result-rewriting middlewares append a declared, ordered transform trail;
MCP tools carry their credential-free logical origin.
Extension route identity
Contributed routes are session-authenticated and cannot opt out, but
"logged in" and "administrator" are different questions. Extensions get a
neutral projection of the caller rather than the host's auth context, and
`require_admin` fails closed when identity cannot be determined.
Extension-owned tables
An extension that persists data owns its own MetaData and migration chain, so
its tables are absent from Base.metadata and `alembic revision --autogenerate`
proposes dropping them. Extensions declare a table prefix, which is rejected
at registration if it would shadow a host table.
The contract package stays dependency-free and imports no host code; every new
Protocol method has a default so later additions remain additive. The loader's
pre-1.0 rule requires an exact major.minor match, so extensions written against
0.1 are now refused at startup with an actionable install hint rather than
loading into a host that implements a different surface.
uv.lock records the contract package's new version, so `uv sync --locked` still
resolves on a fresh checkout.
* fix(backend): sort gateway service imports
|
||
|
Daoyuan Li
|
1fe71110af
|
test(tool-output): stop expressing "unwritable path" as a magic absolute path (#4722)
* test(tool-output): stop expressing "unwritable path" as a magic absolute path `test_returns_none_on_invalid_path` and `test_fallback_when_disk_write_fails` both need an `outputs_path` that `os.makedirs` refuses to create, so they can reach `_externalize`'s `except OSError: return None` branch. They spell that as the literal path `/dev/null/cannot-mkdir-here`, which only works where `/dev/null` is a character device. On Windows it is an ordinary relative path, so `os.makedirs` succeeds, both tests fail, and the suite writes real files to `C:\dev\null\cannot-mkdir-here\ .tool-results\` -- outside any temporary directory, at the drive root. Running the backend suite a few times leaves dozens of stray files behind. The comment above the first test records that this is the second time the same assumption has broken: `/nonexistent/...` was silently created by `mkdir -p` when CI ran as root in a container, and `/dev/null/...` was the fix. Both encode a guess about the environment rather than the condition under test. Use a regular file as the parent component instead. Creating a directory below a file fails with an `OSError` subclass on every platform -- `NotADirectoryError` (errno 20) on POSIX, `FileNotFoundError` (errno 2) on Windows -- so the branch is reached deterministically, and the path lives inside the test's own `TemporaryDirectory`, so nothing is written outside it. Verified both spellings on Linux (WSL Ubuntu, non-root) and Windows; only the file-as-parent form fails on both. The two tests still have teeth: dropping `_externalize`'s `except OSError` guard makes both fail rather than pass. Tests only -- no production code or documented behaviour changes. * test(tool-output): touch the blocker file instead of writing content Only its existence as a regular file matters for os.makedirs to fail below it, so touch() states that directly. |
||
|
lllyfff
|
99c926b7bb
|
fix(mcp): bring-up has no timeout and externalized tool outputs are counted as undelivered artifacts (#4657)
* fix: bound MCP server bring-up timeouts and exclude externalized tool outputs from delivery verification Two related robustness fixes: 1. MCP server bring-up was unbounded. tool_call_timeout only covered session.call_tool(); tool discovery (subprocess spawn + initialize + tools/list) and persistent stdio session initialization could hang forever, blocking agent construction (and on the Gateway event loop, the whole process). Add a per-server session_init_timeout (default DEFAULT_MCP_SESSION_INIT_TIMEOUT = 60s, null disables) that bounds both discovery and pooled-session initialization. The session pool's existing cancellation handling tears down a session stuck mid-creation in its own task. 2. ToolOutputBudgetMiddleware externalizes oversized tool outputs into outputs/.tool-results/ (configurable tool_output.storage_subdir). The workspace-change scanner and run delivery verification counted those files as produced artifacts, so any run that externalized a tool output without also presenting a real artifact failed with "Artifact delivery incomplete". Exclude TOOL_RESULTS_DIRNAME via a shared constant (mirroring BROWSER_FRAMES_DIRNAME) and thread the configured storage_subdir through snapshot capture so both workspace-changes events and delivery verification stay clean. * review: enforce single-segment tool_output.storage_subdir; document discovery-timeout cleanup Address review feedback: 1. A custom tool_output.storage_subdir with a path separator (e.g. cache/tool-results) silently no-oped the workspace-scanner exclusion: os.walk yields one-segment dirnames, so a nested value never matched and its files were counted as produced artifacts again. ToolOutputConfig now validates storage_subdir as a single directory name (rejects separators, .., absolute, empty) with tests, so the exclusion is always sound. 2. The discovery-timeout path now documents why cancellation is safe, mirroring the session-init note: discovery runs inside the adapter's nested async context managers, and stdio_client's finally terminates the process tree (SIGTERM->SIGKILL on POSIX, process-tree on Windows), so a timed-out npx subprocess and its children are reaped rather than accumulating. * review: log session-init timeouts and align API response model default with runtime config Address second-round review feedback: 1. A session-init timeout raised TimeoutError without any log, unlike the discovery timeout which logs a WARNING. Wrap the bounded get_session in a try/except that logs the timeout (server name + seconds) and re-raises, so operators can diagnose tool-call failures caused by hung MCP sessions. 2. McpServerConfigResponse.session_init_timeout defaulted to None while McpServerConfig defaults to 60s: a server created via PUT /api/mcp/config without the field was persisted with null (no timeout) while the same server created in the config file got 60s. Align the response-model default to DEFAULT_MCP_SESSION_INIT_TIMEOUT so API-created and file-created servers behave the same; an explicit null still opts out. * review: narrow the discovery-timeout handler to the bounded wait_for path The except TimeoutError clause covered both the bounded wait_for branch and the bare discovery branch. With session_init_timeout opted out (None), a TimeoutError raised by discovery itself would hit the %.1f format with None: logging raises TypeError internally, the WARNING is silently dropped, and a --- Logging error --- traceback goes to stderr. Narrow the handler to wrap only the wait_for call, where the branch condition guarantees the timeout value is not None. A discovery-internal TimeoutError on the opted-out path now falls through to the generic failure handler and is reported as 'tool discovery failed' with exc_info. Covered by a regression test that asserts the skip is reported without any broken format. |
||
|
HaotianChen616
|
756eac0d1a
|
feat(tool):Add structured synopsis for oversized tool output previews (#3377)
* Improve tool output preview synopsis * Add JSON path anchors to tool output synopsis * Fix JSON synopsis line anchors * fix(synopsis): tighten detectors and fix CSV first-row join Address review feedback from @willem-bd on PR #3377. Detectors: - _looks_yaml now requires >=3 key-shaped lines and refuses bare uppercase-tag lines ('INFO: ...', 'ERROR: ...') that look like log lines and would round-trip into a flat string dict via safe_load. Previously a 200-line log file was classified as 'YAML object with 3 top-level keys' and lost every line, count, and middle signal. - _try_yaml refuses payloads that safe_load collapses to a dict of all strings (the shape tracebacks and log lines collapse into). - _try_table applies the header-must-look-like-identifiers and minimum-row-count guards only to TSV, since the same safeguards would reject legitimate small CSVs. Refuses tab-indented bash output, ls -l listings, and tree dumps. Rendering: - CSV first data row is now rendered as a key=value list joined by ' | ' (e.g. 'name=Ada | description="a fine, brilliant logician" | score=98'). The previous delimiter.join(rows[1]) silently re-split cells that contained the delimiter inside a quoted cell, which made the synopsis report a 3-column table as 5 columns and misled the model about column count and content. Text summary: - _summarize_text now omits the closing excerpt entirely when the input is shorter than 2 * _TEXT_EXCERPT_CHARS, since the previous opener/closer slices overlapped and duplicated text for short inputs (build_tool_output_synopsis is reachable directly from tests and other callers that pass small inputs). Tests: - Update test_table_preview_extracts_columns to assert the new key=value list format. * fix(synopsis): drop JSON path line/byte offset hints The path-location hint was computed by string-searching for the quoted key in the original content and reporting its byte offset and line number. This anchors at the first textual occurrence of the key string, which is wrong when the key also appears as a value earlier in the document, or when the same key recurs at multiple depths. With nested paths the anchor drifts further on every step because the search cursor is advanced past each previous match. Concrete cases: content = '{"label": "items", "items": {"id": 1}}' _json_path_location(content, ['items']) -> ' (line 1, byte offset 10)' # the value, not the key content = '{"data": {"info": 1, "data": {"info": 2}}}' _json_path_location(content, ['data','data','info']) -> ' (line 1, byte offset 30)' # the inner first 'info', not the second The synopsis instructed the model to 'Start near the line hints above when present', so a wrong anchor would send read_file into the wrong region of the persisted .tool-results file. Drop the hint entirely. The path itself ('$.data.items') is already useful navigation; the agent uses read_file with start_line based on its own judgement of where the relevant slice is. Tests: - Update test_json_preview_reports_nested_paths to assert no 'line ' or 'byte offset ' appears in the body before the Access section. - Rename test_json_line_hints_use_original_content_offsets to test_json_paths_are_emitted_without_line_hints and invert the assertions to check the hints are absent. * fix(synopsis): bound _scalar_examples recursion depth Mirror the _JSON_STRUCTURE_DEPTH cap used by _json_container_paths and _json_shape so that deeply nested JSON cannot trigger RecursionError inside build_tool_output_synopsis. In ToolOutputBudgetMiddleware.awrap_tool_call the synopsis is built inside asyncio.to_thread(_patch_result, ...); a RecursionError would surface as a tool-call failure and the user would lose the entire output. 300-level nested JSON is well inside what an attacker-controlled MCP tool, a JSON-RPC-over-JSON-RPC chain, or a buggy serializer can produce. * feat(synopsis): restore inline raw head/tail sample The synopsis-only preview silently dropped the raw head/tail bytes that preview_head_chars / preview_tail_chars used to inline. For text/code/log outputs the agent lost first/last KB of the actual content and had to issue a follow-up read_file round-trip to see the trailing region (last paragraph of a fetched article, final error line in a traceback, closing diagnostics of a bash run). Restore an inline 'Raw sample (head + tail)' section in the preview. The section is composed by slicing head_chars from the start and tail_chars from the end of the content (with a '...' separator between them, and the tail suppressed when it would overlap the head). For binary-like output, the synopsis's own sample is reused unchanged. This makes preview_head_chars / preview_tail_chars operational again for every kind except binary, which already had a sample channel. Tests: - Rename test_json_preview_extracts_structure_instead_of_head_tail to test_json_preview_includes_structure_and_raw_sample and assert the raw sample section is present and the payload is reachable in the head slice. * test(synopsis): add regression tests for willem-bd review findings Add 8 regression tests under TestToolOutputSynopsis, one per finding in @willem-bd's review of PR #3377: - test_review_5_log_lines_are_not_misclassified_as_yaml Pins the YAML detector to refuse 'LEVEL: message' log lines. - test_review_6_json_paths_are_emitted_without_byte_offset Pins the removal of byte/line hint from JSON path descriptions. - test_review_7_scalar_examples_respects_depth_cap Pins that 500-deep nested JSON does not raise. - test_review_8_csv_first_row_quoted_cells_round_trip Pins the new key=value list format for CSV first-row rendering and asserts that quoted cells with embedded delimiters survive. - test_review_9_tsv_detector_rejects_tab_indented_bash Pins that tab-indented bash output is not classified as TSV. - test_review_10_preview_includes_raw_head_and_tail_sample Pins the restored inline 'Raw sample (head + tail)' section. - test_review_11_short_text_does_not_duplicate_excerpts Pins that closer is suppressed for inputs shorter than 2 * _TEXT_EXCERPT_CHARS. - test_review_12_preview_head_tail_chars_are_operational Pins that head_chars / tail_chars are wired into the rendered preview and not silently dropped. Also removes the now-stale 'byte offsets are approximate anchors' sentence from render_tool_output_preview's Access block; the synopsis no longer emits byte/line hints, so the guidance to 'start near the line hints' was misleading. * fix(synopsis): resolve lint errors on tool output budget tests Local 'make lint' on feat/tool-output-synopsis-preview (after fast-forward to current main) failed with three errors in tests added by PR #3377: - E501: 307-char bash_out literal in test_review_9_tsv_detector_rejects_tab_indented_bash - E741: ambiguous single-letter 'l' in test_review_11_short_text_does_not_duplicate_excerpts - E741: same ambiguous 'l' on the closing assert Replace the long literal with a join of per-row entries, rename the loop variable from 'l' to 'ln', and run ruff format on the two touched files to absorb the formatting drift introduced by the merge with main. Verification: - make lint -> All checks passed; 643 files already formatted - pytest tests/test_tool_output_budget_middleware.py -> 110 passed * fix: address willem-bd review findings (code/csv misclassification, text duplication, line snapping, dead constant, xml hardening, depth consistency) - _CODE_HINTS: require stronger signals for use/fn (trailing ; or parenthesised) - _try_table: apply _TABLE_MIN_DATA_ROWS gate to CSV too (not just TSV) - config.example.yaml: correct misleading comment about preview_head/tail_chars - _summarize_text: skip opener/closer excerpts when raw sample will be appended - _build_raw_sample: snap to line boundaries for clean truncation - Remove dead constant _TABLE_FIRST_ROW_CHARS - Prefer defusedxml for XML parsing (billion-laughs protection), fallback to stdlib - Replace _json_shape magic number 2 with named _JSON_SHAPE_MAX_DEPTH constant - Update tests to match new CSV gate (>=5 rows) and line-snapped sample counts * style: ruff format fix for tool_output_synopsis.py and test_tool_output_budget_middleware.py * fix(tool-output): address 4 review comments - DoS hardening + size cap 1. XML entity-expansion DoS: skip _try_xml when defusedxml is not available (SafeET is None), falling through to text + raw sample. (cid=3587721336) 2. YAML alias-bomb DoS: refuse to parse YAML content > 500 KB. (cid=3587721340) 3. Unbounded content parse: add _MAX_SYNOPSIS_INPUT_BYTES=5MB cap; oversized output falls back to raw head/tail sample instead of full parse. (cid=3587721346) 4. Scalar examples surface mid-document values: add docstring note that the synopsis is a structural summary, not a confidentiality filter. (cid=3587721353) * fix(tool-output): ruff format the synopsis string to one line --------- Co-authored-by: qinchenghan <qinchenghan@huawei.com> |
||
|
Aari
|
3be3969f8f
|
fix(agent): snap tool-output tail forward so fallback truncation respects max_chars (#4017)
* fix(agent): snap tool-output tail forward so fallback truncation respects max_chars _snap_to_line_boundary() moves an offset backwards to the preceding newline. That is correct for the head's end offset, but _build_fallback() also applied it to the tail's start offset, where moving backwards lengthens the tail. On output whose last newline sits in the second half (log lines followed by one long unbroken line: minified JSON, base64 artifacts), the returned string exceeded max_chars by up to 17x, defeating the guard that exists to stop a single large tool result from blowing the model context. Add _snap_start_to_line_boundary(), the forward-snapping mirror, and use it for the tail offset. The existing head_end guard becomes redundant because a forward snap can only move the tail start away from the head. test_result_never_exceeds_max_chars already asserted this invariant but passed newline-free content, so the snapping branch was never exercised. * test(agent): pin the forward-snap direction of the fallback tail The bound test only asserts that the result fits max_chars. Its content has no newline inside the tail's snap window, so _snap_start_to_line_boundary returns pos unchanged and the test stays green even with the snap removed. Place a newline in the window so the snap has to fire: the tail must begin after it, which the pre-fix backward snap does not do. |
||
|
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. |
||
|
Zhipeng Zheng
|
e7b88a97ed
|
fix(security): add input sanitization middleware for prompt-injection defense (#3630) (#3662)
InputSanitizationMiddleware: escapes blocked XML tags in user messages (<system> -> <system>) — de-identify, don't reject (AWS PII-style). Wraps user input in plain-text boundary markers (--- BEGIN/END USER INPUT ---) per OWASP structured-prompt guidance. System-Context Confidentiality clause prevents LLM from revealing internal instructions. Boundary-marker layer hardened against delimiter injection (self-suppression + break-out). Multimodal content blocks preserved during sanitization. Addresses review feedback from @fancyboi999 and @WillemJiang: 1. Boundary-marker injection: strict startswith+endswith idempotency, neutralize user-supplied BEGIN/END tokens, forged-idempotency bypass fixed 2. Multimodal data loss: _rebuild_content preserves interleaved non-text blocks 3. CI fixes: replay_provider strips boundary markers before hashing, middleware chain assertions updated 4. Lint: ruff format applied on Linux (slice spacing + f-string collapse) Test: 71 input_sanitization + 1 replay_golden + 14 tool_error + 93 tool_output_budget = 179/179 passed |
||
|
Huixin615
|
64d923b0fd
|
fix(middleware): externalize oversized tool output into sandbox for non-mounted sandboxes (#3417)
* fix(middleware): externalize oversized tool output into sandbox for non-mounted sandboxes
ToolOutputBudgetMiddleware persisted oversized tool results to the host
filesystem and returned a /mnt/user-data/outputs virtual path. For sandboxes
that do not use thread-data mounts (e.g. remote AIO sandbox), that virtual
path does not exist inside the sandbox, so the model's read_file tool could
not read it back and reported 'file not found'.
Branch on SandboxProvider.uses_thread_data_mounts:
- Mounted sandboxes (local Docker, AIO + LocalContainerBackend) keep the
original host-disk path; the host outputs dir is bind-mounted to the same
virtual path inside the sandbox, so behavior is unchanged.
- Non-mounted (remote) sandboxes externalize into the sandbox itself via
execute_command('mkdir -p ...') + write_file + 'test -s' validation. The
validation step is required because AIO sandbox execute_command returns
'Error: ...' as a string on failure instead of raising, so a silent mkdir
failure would otherwise leak through.
Any failure (rejected subdir, mkdir/write/validate error) falls back to the
existing inline head+tail truncation, so an unreadable path is never returned
to the model.
The sandbox resolver reads the sandbox_id that SandboxMiddleware already
writes into runtime.state['sandbox']; it never calls provider.acquire(),
keeping the tool-call hot path free of blocking I/O. Tools that do not use a
sandbox (web_search, MCP, ...) resolve to None and fall through to inline
truncation, which is the safe behavior for them.
Fixes #3416
* fix(middleware): address Copilot review feedback on sandbox externalization
- Make get_sandbox_provider() lookup best-effort in _budget_content: only
query when outputs_path or sandbox is available, and fall back to inline
truncation if provider initialization raises rather than propagating
the error. A resolved sandbox instance is sufficient on its own to take
the non-mounted externalization branch.
- Strict-match the sandbox post-write validation echo
(check.strip() == 'OK') to avoid false positives if execute_command
ever surfaces unrelated stdout/stderr containing 'OK' as a substring.
Refs: #3417
* test: fix flaky tests relying on /nonexistent/... path under container root
Two tests in this module (test_returns_none_on_invalid_path and
test_fallback_when_disk_write_fails) used paths like
'/nonexistent/impossible/path' to trigger _externalize's OSError
fallback. These paths are creatable when the test process runs as root
inside the CI container: os.makedirs(..., exist_ok=True) successfully
creates the entire chain under /, so the OSError branch is never hit
and the tests fail. Reproducible on main independently of this PR.
Switch to '/dev/null/cannot-mkdir-here'. /dev/null is a character
device on both Linux and macOS, so os.makedirs always fails with
NotADirectoryError regardless of privileges, reliably exercising the
OSError fallback.
* fix(tool-output-budget): only consult sandbox provider when a sandbox is resolved
The previous revision called get_sandbox_provider() whenever externalization
was triggered, including on the legacy host-disk path. Environments without
a configured sandbox -- in particular CI runners without a config.yaml --
would raise FileNotFoundError there, get caught, and silently fall back to
inline truncation. That defeated the host-disk externalization path that
predates this PR and was the root cause of the regressing legacy tests.
Restructure the branching so the provider is only consulted when a sandbox
has actually been resolved for the current tool call:
- sandbox resolved + provider.uses_thread_data_mounts: host-disk write
(bind-mounted into the sandbox, equivalent to a sandbox-side write).
- sandbox resolved + non-mounted provider: sandbox write (#3416).
- no sandbox + outputs_path: host-disk write
(legacy / non-sandbox tools, no provider call at all).
- otherwise: inline fallback.
No test changes; the legacy externalization tests are provider-agnostic by
construction and now pass without monkeypatching.
Refs: #3416
* test(tool-output-budget): assert legacy path does not call sandbox provider
Lock in the contract introduced by d6e2d25b: when no sandbox is resolved
for a tool call, _budget_content must externalize to the host outputs
directory without consulting get_sandbox_provider(). Regressing this would
re-break legacy / non-sandbox tools in environments without a configured
sandbox (e.g. CI without config.yaml), which is the failure mode #3416's
fix avoids.
The test injects a get_sandbox_provider that raises on call, so any
future refactor that moves the provider lookup out of the sandbox-only
branch will fail loudly.
Refs: #3416
|
||
|
Xinmin Zeng
|
ca487578a4
|
feat(agent): add ToolOutputBudgetMiddleware for oversized tool output protection (#3303)
* feat(agent): add ToolOutputBudgetMiddleware for oversized tool output protection Closes #3289. Adds a unified middleware that enforces per-result budgets on ALL tool outputs (MCP, sandbox, community, custom), preventing oversized external tool results from blowing the model context window. Design informed by claude-code (persistToolResult), hermes-agent (tool_result_storage), and pi (OutputAccumulator) — the three most mature implementations in production coding-agent frameworks. Key features: - Disk externalization: oversized outputs written to thread-local .tool-results/ directory, replaced with compact preview + file reference. Model can read full output via read_file with offset/limit. - Fallback truncation: head+tail truncation when disk is unavailable (no thread_data, write failure), ensuring the context is always protected. - read_file exemption: prevents persist-read-persist infinite loops (independently discovered by claude-code, hermes-agent, and pi). - Per-tool threshold overrides via config. - Line-boundary-aware truncation (no partial lines in previews). - Multimodal content passthrough (images/structured blocks skip budget). - Historical ToolMessage patching in wrap_model_call for checkpoint recovery scenarios. Related: #3222 (design RFC), #1844 (comprehensive context management), #3137 (write_file args compaction), #1677 (sandbox tool truncation). * test: add MCP content_and_artifact format coverage Add 5 tests for MCP tool output format (list of content blocks): - text content blocks are extracted and budgeted - multiple text blocks are joined and budgeted - image content blocks are skipped (multimodal passthrough) - mixed text+image blocks are skipped - small text blocks pass through unchanged Total test count: 59 (was 54). * fix(agent): address Codex review findings for ToolOutputBudgetMiddleware Three issues identified by Codex code review, all fixed: 1. `enabled` config field was unused — middleware now checks `config.enabled` and skips all processing when disabled. 2. `_build_fallback` could exceed `fallback_max_chars` — the marker text itself (~139 chars) was not deducted from the budget. Now pre-computes marker overhead and falls back to hard slice when max_chars is smaller than the marker. 3. Sync file I/O in async path — `awrap_tool_call` now delegates `_patch_result` to `asyncio.to_thread` to avoid blocking the event loop during disk writes. Tests updated to use realistic fallback_max_chars values (500+) that can accommodate the marker overhead, plus two new tests: - `test_result_never_exceeds_max_chars` (parametric across sizes) - `test_very_small_max_chars_does_not_crash` * fix(agent): address Copilot review — path traversal, async perf, shared config 1. Path traversal defense: sanitize tool_name via _sanitize_tool_name() (strips separators, .., absolute paths), validate storage_subdir is relative, and verify resolved filepath stays inside storage_dir. 2. Async hot-path optimization: add _needs_budget() cheap check before asyncio.to_thread offload — small outputs (99% of calls) skip the thread overhead entirely. 3. Replace shared module-level _DEFAULT_CONFIG with _default_config() factory to prevent cross-instance mutation of mutable fields. 12 new tests: TestSanitizeToolName (5), TestExternalizePathTraversal (3), TestNeedsBudget (4). * fix(agent): correct preview hint to match read_file actual API read_file uses start_line/end_line (1-indexed line numbers), not offset/limit. The previous wording was copied from hermes-agent which has a different read_file interface. * perf(agent): hoist hot-path imports, add model-call pre-scan (review #3303) Address maintainer review feedback: 1. Hoist inline imports to module level — `import asyncio` (was in awrap_tool_call hot path) and `from dataclasses import replace` (was in _patch_result) now live at module top. 2. Add a cheap pre-scan to _patch_model_messages so the historical message list is not rebuilt on every model call when nothing is oversized (the common case once results are budgeted at tool-call time). Also adds the same _needs_budget gate to the sync wrap_tool_call for symmetry with awrap_tool_call. The pre-scan is refactored into per-tool-aware helpers (_effective_trigger / _tool_message_over_budget) that mirror the exact trigger conditions in _budget_content — including tool_overrides — so the fast-path can never produce a false negative (silently skipping budgeting for a tool with a low per-tool threshold). 7 new regression tests lock the per-tool-override-through-pre-scan path and the model-call early return. --------- Co-authored-by: Willem Jiang <willem.jiang@gmail.com> |