mirror of
https://github.com/bytedance/deer-flow.git
synced 2026-07-20 09:45:47 +00:00
main
34
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 Daoyuan LiandGitHub
|
474a0fd6b7 |
fix(gateway): correct GitHub auto-retry claim in webhook route docs (#4289)
While investigating a review on PR #4274 (which corrected the same misconception in the channel manager's dedupe-TTL comment), I found a more elaborate version of the same factual error in this route, predating that PR: the docstring claimed "GitHub retries 5xx deliveries with exponential backoff (up to ~5 attempts over ~8 hours)". No such behavior exists. Verified directly against GitHub's documentation: failed deliveries (5xx, timeout, connection error alike) are simply recorded as failed and never automatically redelivered, for both repository and GitHub App webhooks (https://docs.github.com/en/webhooks/using-webhooks/handling-failed-webhook-deliveries). Redelivery is always explicit: the "Redeliver" button, the REST API, or an operator's own recovery script (GitHub's own recommended pattern for the latter runs on a 6-hour cron, illustrating this is opt-in operator tooling, not a GitHub-side mechanism). The 503-vs-200 status code choice itself was already correct and is unchanged: 503 keeps a transient failure recorded as failed so it can be recovered via one of the explicit paths above, while 200 correctly marks permanent/non-retryable conditions (disabled channel, unknown event, missing service) as done so no pointless redelivery is invited. Only the rationale text describing *why* was wrong. - github_webhooks.py: reworded the route docstring and four inline comments/log messages that repeated "GitHub retries" / "so GitHub retries" framing, citing the actual documented behavior. - test_github_webhooks.py: renamed test_dispatch_failure_returns_503_so_github_retries to test_dispatch_failure_returns_503_not_200 and reworded its docstring plus two nearby docstrings/comments with the same framing. No assertions changed - same status codes and response bodies checked. - AGENTS.md: corrected the GitHub Webhooks router table row to match. All 44 tests in test_github_webhooks.py pass, plus the full test_github_dispatcher.py / test_github_channel.py / test_github_registry.py / test_channels.py suites (313 total). ruff check and ruff format --check are clean on both touched Python files. |
||
|
Daoyuan LiandGitHub
|
0cd55067f3 |
fix(skills): reject colon in zip member names to close NTFS ADS smuggling gap (#4236)
Neither is_unsafe_zip_member (installer.py) nor its duplicated check in skillscan/orchestrator.py rejected a colon in a zip member name. On Windows/NTFS, a name like scripts/run.sh:hidden.txt addresses an Alternate Data Stream on run.sh instead of creating a new file, so the hidden content is invisible to Path.rglob()/os.walk()-based scanning in both the deterministic static scanner and the extracted-file content scanner, while still landing genuinely on disk. Reject any colon in a zip member's relative path outright in both files; a colon has no legitimate use there since zip entries use forward slashes and a real Windows drive prefix is already caught by the existing absolute-path check. |
||
|
Daoyuan LiandGitHub
|
3ed2e1f1d9 |
fix(config): close out #4124 review follow-ups (shared signature helper, resolve_config_path None contract) (#4275)
* fix(config): close out #4124 review follow-ups Extracts the (mtime, size, sha256) content-signature helper that was duplicated between config/app_config.py and mcp/cache.py into a new config/file_signature.py, and fixes ExtensionsConfig.resolve_config_path() to return None instead of raising FileNotFoundError when an explicit config_path argument or DEER_FLOW_EXTENSIONS_CONFIG_PATH points at a file that has since been deleted -- the exact resolution mode Docker dev/prod uses per AGENTS.md, so the MCP tools-cache staleness check could raise instead of degrading to "not stale". Both were flagged by willem-bd in review on #4124 and explicitly deferred there ("flagging for visibility", "leaving the extraction as the follow-up you suggested"). * fix(config): keep explicit extensions-config paths fail-loud resolve_config_path() previously turned every missing-file case into a clean None, including an explicit config_path argument or DEER_FLOW_EXTENSIONS_CONFIG_PATH (the exact mode Docker dev/prod uses). That silently downgrades a bad Docker mount, typo, or deleted production config to "no extensions" instead of surfacing the misconfiguration, per fancyboi999's review [P1] and willem-bd's follow-up notes on this PR. Restores FileNotFoundError for the two explicit modes (config_path argument, DEER_FLOW_EXTENSIONS_CONFIG_PATH); only the fallback search mode (no explicit path/env var, nothing found in the usual locations) still returns None, since that is the legitimate "extensions were never configured" case. The one caller that needs the old fail-soft behavior -- the MCP tools-cache staleness check, which re-resolves the path on every get_cached_mcp_tools() call -- gets a narrow, local catch instead (deerflow.mcp.cache._resolve_config_path) so a config file going missing mid-run still degrades the cache to "not stale" rather than crashing a hot per-request path. Also hoists the double os.getenv() read in the env-var branch into a local, per willem-bd's nit. Adds resolver-level tests for both explicit-path and env-var raises, a dedicated search-mode None regression test, and updates the existing MCP-cache docstrings to describe the corrected split. |
||
|
Daoyuan LiandGitHub
|
9a5d701355 |
fix(channels): document GitHub dedupe TTL, tighten redelivery tests (#4274)
* fix(channels): document GitHub dedupe TTL, tighten redelivery tests Follow-up to PR #4104's review, whose non-blocking notes were left unaddressed at merge time: - Document the previously-undocumented inbound-dedupe TTL (INBOUND_DEDUPE_TTL_SECONDS, 10 min, in-memory only) directly above the constant in ChannelManager: what it is, why the window, and what happens at the boundary (a manual "Redeliver" clicked after the TTL has elapsed, or any redelivery after a Gateway restart, is not deduped, since _recent_inbound_events is never persisted to ChannelStore). Cross-reference it from the GitHub dispatcher's fan-out comment, which previously implied unconditional redelivery coverage. - test_channels.py::test_github_redelivery_is_deduped_like_other_channels: the _gh helper stamped a 2-part delivery:agent id while production stamps a 3-part delivery:user:agent id. Align the helper with the real shape and add a cross-user assertion the old 2-part shape could not even express. - test_github_dispatcher.py::test_missing_delivery_header_leaves_dedupe_open: previously only asserted the dispatcher-layer id was None. Add the manager-level _is_duplicate_inbound assertion across two header-less deliveries so a future regression that treats a missing id as a real key is caught here too, not only at the dispatcher layer. All four affected test files (test_github_dispatcher.py, test_channels.py, test_github_channel.py, test_github_registry.py) pass; ruff check and ruff format --check are clean. Verified the two tightened tests actually discriminate by temporarily reintroducing each bug they target (2-part id shape; missing-id treated as a real key) and confirming the new assertions fail, then reverting. * fix(channels): correct GitHub redelivery claim in dedupe comments fancyboi999's review on this PR flagged that the previous commit's new comments (manager.py, dispatcher.py) justified the 10-minute dedupe TTL by claiming GitHub automatically retries a failed delivery after its 10-second timeout. GitHub's own documentation says the opposite: failed deliveries (non-2xx, timeout, connection error) are recorded as failed and never automatically redelivered, for both repository and GitHub App webhooks alike. Every redelivery is explicit: the "Redeliver" button, the REST API, or an operator's own recovery script. - manager.py: rework the INBOUND_DEDUPE_TTL_SECONDS comment to state GitHub's actual behavior (no auto-retry), cite the authoritative doc (docs.github.com/en/webhooks/using-webhooks/handling-failed-webhook-deliveries), and justify the 10-minute window as a bound on explicit near-term replays rather than an automatic-retry window. No longer asserts unverified retry behavior for the other channels either. - dispatcher.py: same correction for the dedupe_message_id comment, which previously implied "retried after a timeout" as a distinct, automatic case alongside the manual "Redeliver" button. - test_channels.py / test_github_dispatcher.py: reworded the two docstrings and one section comment that repeated the same "retry-on-timeout" framing, so the corrected mental model doesn't sit next to contradicting prose nearby. No assertions changed. Verified directly against GitHub's current documentation (fetched, not paraphrased from memory): "Handling failed webhook deliveries" states plainly that GitHub does not auto-redeliver; "Automatically redelivering failed deliveries for a GitHub App webhook" / "...for a repository webhook" are both guides for a user-written recovery script (GitHub's own example runs on a 6-hour cron), not a native GitHub feature. No difference in this behavior between GitHub Apps and repository webhooks. All four affected test files pass; the two `test_bot_login_whitespace_only` / `test_trigger_mention_login_whitespace_only` failures in test_github_dispatcher.py are pre-existing on this branch's unmodified HEAD (confirmed against a clean checkout of the same commit) and are unrelated to this change. ruff check and ruff format --check are clean on all four touched files. |
||
|
Daoyuan LiandGitHub
|
d075be0277 |
fix(browserless): surface target-page error status in web_fetch_tool (#4239)
* fix(browserless): surface target-page error status in web_fetch_tool Browserless returns HTTP 200 for the render request itself even when the target page responded with a 4xx/5xx or served an anti-bot block page, tagging the real outcome on X-Response-Code/X-Response-Status headers. capture_screenshot/web_capture_tool already reads these headers and surfaces a warning via _target_status_warning. fetch_html only logged them at debug level and web_fetch_tool returned the block/error page's raw text as if it were a normal successful fetch, with no indication anything was wrong. fetch_html now returns a BrowserlessFetchResult carrying the rendered HTML plus the target-status headers (mirroring BrowserlessScreenshotResult), and web_fetch_tool appends the same _target_status_warning used by web_capture_tool when the target page errored. Legitimate 200-target fetches are unaffected. * fix(browserless): keep fetch_html() returning a plain string BrowserlessClient is re-exported from deerflow.community.browserless.__all__, so fetch_html() is public harness API with an established str-only contract: the rendered HTML on success, or an "Error: ..." string on failure. Changing its return type to BrowserlessFetchResult broke that contract for any caller that treats the result as a string (.lower(), concatenation, passing to a parser), even when the fetch itself succeeded. fetch_html() is now a thin wrapper that always unwraps back to the original str contract. The richer, status-aware result (needed to tell a genuine 200 apart from a render-succeeded-but-target-errored response) moves to a new fetch_html_with_status() method, which web_fetch_tool now calls instead so it keeps surfacing the target-page-error warning. Tests: retarget the tool-level mocks onto fetch_html_with_status, add a direct regression asserting fetch_html() returns str on success - including when the target page itself errored under a 200 render response - and keep the status-aware coverage on the new method. |
||
|
Daoyuan LiandGitHub
|
283cea567e |
fix(scripts): broaden support bundle secret-key redaction denylist (#4242)
* fix(scripts): broaden support bundle secret-key redaction denylist SECRET_KEY_RE only matched a fixed keyword allowlist, so a secret stored under an unanticipated key name inside an open-ended config dict (e.g. guardrails.provider.config, an arbitrary provider-kwargs dict) was emitted verbatim into config-summary.json even though manifest.json claims redacted_secret_fields=true. This gap was flagged on PR #3886's review before merge but not fully addressed. Broaden the key-name match to mirror env_policy.py's wildcard denylist (*KEY*/*SECRET*/*TOKEN*/*PASS*/*CREDENTIAL*/*DSN*) already used for sandbox env-scrubbing, plus its no-flag credential exact names (GH_PAT/GITHUB_PAT/ REDIS_AUTH/REDISCLI_AUTH/PGSERVICEFILE). The new bare key/pass/dsn alternatives are boundary-guarded so they match only their own delimited token, not an unrelated word that starts with the same letters (routing "keywords", guardrails "passport"). * fix(scripts): stop the pass token boundary from missing passphrase/passcode SECRET_KEY_RE's bare "pass" alternative, (?<![a-zA-Z])pass(?![a-zA-Z]), excludes any key where "pass" is followed by another letter. That correctly keeps "passport" out of the redaction set, but it also excludes genuine secret-bearing key names like "passphrase" and "passcode" -- both of which env_policy.py's *PASS* substring denylist does catch, so a secret stored under either name in an open-ended config dict (e.g. guardrails.provider.config) would still leak into config-summary.json. Narrow the lookahead to only exclude a trailing "port" -- pass(?!port) -- so passphrase/passcode/pass/db_pass all match while passport stays excluded; compass/bypass stay excluded via the existing leading-letter lookbehind, independent of the lookahead. Added a regression test covering both the newly-caught names and the still-excluded ones in one place. Reverting to the old lookahead reproduces the exact leak (passphrase left unredacted); with the fix, tests/test_support_bundle.py (30 tests) is green, and ruff check/format are clean. |
||
|
Daoyuan LiandGitHub
|
75fa028e89 |
fix(artifacts): serve inline binary artifacts via FileResponse for Range support (#4281)
* fix(artifacts): serve inline binary artifacts via FileResponse for Range support Audio/video/image artifacts that are previewed inline (not downloaded, not active content) were served by reading the whole file into memory and returning it through a plain Response. That response never sets Accept-Ranges and ignores any Range header the browser sends, so seeking an <audio>/<video> element backed by this endpoint always gets the full 200 response back instead of a 206 partial response for the requested byte range -- which is why dragging the seek bar on an audio artifact restarts playback from the beginning instead of jumping to the new position. Route this case through FileResponse instead (as the active-content/ download branch already does), which handles Range/If-Range natively. Verified with a real Range request against the endpoint: initial load now reports Accept-Ranges: bytes, and a ranged GET returns 206 with the correct Content-Range and only the requested slice of bytes. Fixes #3240 * fix(artifacts): address review nits on the inline FileResponse branch Two non-blocking nits from review: - Drop the redundant filename= kwarg on the inline_file FileResponse call. Content-Disposition is already set explicitly on this branch, and FileResponse only uses filename to setdefault that same header -- which is a no-op once it's already present. Harmless today, but removes the latent risk that a future Starlette version turning that setdefault into a hard set would silently flip inline preview to attachment. - Reword the blocking-IO regression-anchor docstring: it claimed awaiting get_artifact for a binary artifact does "zero filesystem IO", but _read_artifact_payload still runs exists/is_file/ mimetypes.guess_type/is_text_file_by_content (an 8 KB read) for binary files too, just offloaded via asyncio.to_thread like the text branch. The gate has nothing to catch because that IO is off the event loop, not because there's none -- reworded to say no full-file read happens, matching the accurate framing already used one paragraph up. tests/test_artifacts_router.py, tests/blocking_io/test_artifacts_router.py (19 tests) are green, and ruff check/format are clean on both changed files. |
||
|
Daoyuan LiandGitHub
|
7757e38b7f |
fix(nginx): allow long chat prompts through /api/langgraph/ without a raw 500 (#4277)
nginx's default client_max_body_size (1m) and proxy_request_buffering (on) were never overridden for the chat/LangGraph route, only for the upload route. A long pasted prompt either exceeded the default size limit (413) or got spooled to a temp file before reaching Gateway, which can fail with a raw nginx 500 on a non-root local run if that temp directory isn't writable -- reproduced independently (real nginx 1.28.3, byte-identical error page and matching error-log Permission denied line) while confirming fancyboi999's diagnosis in issue #3952. Mirrors the upload route's existing client_max_body_size / proxy_request_ buffering fix onto /api/langgraph/ in all three places this nginx config is maintained (Docker prod, local make dev, and the Kubernetes/Helm ConfigMap), sized smaller (20M) since this route only ever carries JSON chat text, never binary file uploads. |
||
|
Daoyuan LiandGitHub
|
1ae02913ea |
fix(skills): cap archive entry count in safe_extract_skill_archive (#4241)
safe_extract_skill_archive() capped total uncompressed bytes (zip bomb defence) but had no limit on member count, so a small archive with tens of thousands of tiny/empty entries extracted with no error. The same entry-count cap already existed in scan_archive_preflight() (skillscan orchestrator, 4096 members) with the comment "a huge member count is a bounded DoS vector even when the total size is small" -- but that scan only runs when the optional skill_scan.enabled kill switch is on (default true, but operator-configurable), so disabling it silently dropped this specific protection while config.example.yaml's comment implied safe archive extraction alone still covered it. Move the same 4096 cap into safe_extract_skill_archive itself as an early-abort before any per-member work, so it applies unconditionally regardless of skill_scan.enabled. Leaving scan_archive_preflight's own cap in place as defense in depth (it fires earlier, on preflight, with a structured finding for reporting). Related: #2618 requested exactly this hardening; #2619 (closed, unmerged) implemented a broader version of it, including this same entry-count cap directly in the extractor. |
||
|
Daoyuan LiandGitHub
|
9a4c72db99 |
fix(channels): isolate per-message failures in WeChat poll loop (#4231)
_poll_loop persisted the long-poll cursor (get_updates_buf) for the whole getupdates batch immediately on receipt, then iterated data["msgs"] with no per-message error handling. If any single message's processing raised (e.g. an attachment that fails to decrypt), the for loop aborted and every message after the failing one in that batch was never handled -- but since the cursor had already advanced past the whole batch, the next poll would never re-fetch them. The loss was silent and permanent, not a delay. Fix is two parts: - Wrap each _handle_update call in its own try/except (re-raising CancelledError) so one bad message is logged and skipped instead of aborting the rest of the batch. - Move the cursor advance/persist to after the per-message loop instead of before it, so a hard crash mid-batch leaves the cursor unmoved. Worst case becomes re-fetching and re-processing the batch on restart, not silently skipping messages that were never actually handled. Regression test drives the real _handle_update over a 3-message batch where the middle message is a WeChat image item with deliberately non-block-aligned "encrypted" bytes, so decryption raises a genuine cryptography ValueError (the same failure shape a corrupt real attachment would produce). Confirms the first and third messages both still reach the bus, the failure is logged with the message id, and the persisted cursor reflects the fully-attempted batch. |
||
|
Daoyuan LiandGitHub
|
13afef6278 |
fix(tui): interrupt an active run before /quit exits (#4235)
_handle_builtin's "quit" branch called self.exit() unconditionally, unlike action_interrupt (Ctrl+C), which checks self._streaming and interrupts before any teardown. During an active run, /quit tore the app down while the worker thread was still live; the next call_from_thread call from that orphaned thread then failed silently (the app's loop is gone), quietly abandoning the in-flight turn and any post-run persistence such as the thread title. Mirror action_interrupt's check: if self._streaming, run the same interrupt/cleanup path before calling self.exit(). /quit still always exits -- it now just does so safely. Adds a regression test using a fake client that blocks mid-stream (a real worker thread genuinely stuck, not a hand-flipped flag) to catch /quit during a run, plus a Ctrl+C contrast test under the same setup. |
||
|
Daoyuan LiandGitHub
|
45865e9f3f |
fix(tracing): attach Langfuse trace metadata to the goal evaluator (#4202)
The goal evaluator (runtime/goal.py) runs from runtime/runs/worker.py after
the main graph run has already completed, so there is no graph root for it
to inherit tracing from. create_goal_evaluator_model was built with
attach_tracing=False, and evaluate_goal_completion invoked the model with a
bare config={"run_name": "goal_evaluator"} — no tracing callbacks, no
Langfuse session/user attribution. Every goal-evaluator LLM call went
untraced.
Same class of gap fixed by #2944 for the main agent graph and by #3902 for
memory_agent/suggest_agent: a standalone call site that invokes a model
directly instead of through a traced graph root must attach its own tracing
callbacks and inject Langfuse trace-attribute metadata itself.
- create_goal_evaluator_model: attach_tracing=False -> True, matching the
other standalone non-graph callers (oneshot_llm.run_oneshot_llm,
MemoryUpdater).
- evaluate_goal_completion: accept optional thread_id/user_id/
deerflow_trace_id and inject Langfuse trace metadata onto the ainvoke
config via the shared inject_langfuse_metadata() helper, mirroring
oneshot_llm.py's pattern.
- worker.py: thread user_id (resolve_runtime_user_id(runtime)) and
deerflow_trace_id through _prepare_goal_continuation_input into
evaluate_goal_completion so the evaluator's trace groups under the
triggering run's thread/session.
Updates the existing test that pinned attach_tracing=False as expected
behavior, and adds a regression test asserting the ainvoke config carries
Langfuse trace metadata when enabled.
|
||
|
Daoyuan LiandGitHub
|
6ef0aa2aea |
fix(tui): stop empty-id assistant deltas from merging across turns (#4201)
_apply_assistant_delta matched AssistantDelta.id anywhere in the transcript, which is correct for a genuine per-message id but not for an empty one. runtime._as_str() coerces a missing/None chunk id to "", and that value is shared by every id-less chunk from every turn, not just the current one. Once one assistant row had id="", every later, unrelated AssistantDelta that also carried id="" (e.g. from a provider that never stamps per-chunk ids) matched that same stale row instead of starting a fresh one, silently folding a second turn's answer backward into the first turn's bubble. Route empty-id deltas to a dedicated path that tracks the current turn's row by position (streaming_anonymous_row_index, reset on RunStarted/ RunEnded/ClearRows) instead of by id, mirroring the existing empty-id guards in _apply_tool_started/_apply_tool_result but adapted for assistant text: unlike a tool call, an id-less assistant delta still needs to be displayed, so it starts a new row rather than being dropped. Multiple id-less chunks legitimately arrive within one turn (per-token streaming), so they keep coalescing into that row -- but only while it is still the transcript tail; once a tool card is appended after it (the same way a genuine id naturally changes across a tool round-trip), the next empty-id delta starts fresh instead of reaching backward past the tool card. Add regression coverage for the cross-turn merge, same-turn coalescing, the tool-call-interleaved edge case, and non-interference with the existing id-keyed path. |
||
|
Daoyuan LiandGitHub
|
fa419b3e4c |
fix(goal): stop continuation_count double-bump in thread_changed_before_continuation stand-down (#4199)
_prepare_goal_continuation_input calls the same _persist closure twice with the identical next_count in one evaluation cycle: once to commit the real continuation, and again to record a thread_changed_before_continuation stand-down if a race is detected right after that commit. The second call re-passed continuation_count=next_count, so #4088's defensive max(continuation_count, current_count + 1) guard saw the first call's own write as a "current_count" bump and added another +1 on top of it - silently consuming 2 units of the continuation budget for a cycle that delivered zero actual continuations. #4088's guard is correct for the independent-concurrent-continuations race it targets; this is a separate call site incorrectly re-triggering that same guard against itself. The second _persist call no longer passes continuation_count, matching every other stand-down call site in this function - the count was already correctly committed by the first call. Adds a regression test mirroring the existing thread_changed_after_evaluation race test's checkpointer-wrapper technique, since this sibling branch had zero prior coverage. |
||
|
Daoyuan LiandGitHub
|
2a7469cdbc |
fix(channels): dedupe GitHub webhook redeliveries (#4104)
* fix(channels): dedupe GitHub webhook redeliveries The inbound dedupe added for the IM channels in #3584 keys ChannelManager._is_duplicate_inbound on a top-level metadata["message_id"] plus a workspace id. The GitHub channel added later in #3754 never populated either: the X-GitHub-Delivery GUID was buried in metadata["github"]["delivery_id"] and no workspace id was set, so every GitHub delivery produced a None dedupe key and was never deduped. GitHub reuses the same X-GitHub-Delivery GUID when a delivery is retried after a timeout or replayed via the repo/App "Redeliver" button, so a redelivery re-ran the agent with real side effects (e.g. a duplicate PR comment) while the other channels absorbed an identical redelivery. fanout_event now stamps the dedupe identity the manager already consumes: - workspace_id = repo (globally unique, always present; mirrors Telegram/WeChat keying the workspace on the chat id) - metadata["message_id"] = f"{delivery_id}:{agent.name}" A single delivery fans out to N agents, so the id is scoped to (delivery, agent): an identical redelivery reproduces the same pairs (deduped) while two agents matching the same delivery keep distinct ids and both still fire. When the delivery header is absent the id is left None, so the manager fails open exactly as before. Tests: dispatcher-level coverage that the identity is stamped, stable across redelivery, and distinct per agent/delivery; a ChannelManager regression that an identical GitHub redelivery dispatches once while a new delivery and a second agent on the same delivery still fire. * fix(channels): scope GitHub dedupe id by owning user _inbound_dedupe_key indexes on (channel, workspace_id, chat_id, message_id). For GitHub, workspace_id and chat_id are both the repo, so the owning user was never represented anywhere in the key - fanout_event stamped the id as f"{delivery_id}:{agent.name}". Two different users each binding an agent of the same name (e.g. "reviewer") to the same repo+event therefore produced an identical dedupe id for both fan-out messages. ChannelManager._is_duplicate_inbound treated the second as a replay of the first and silently dropped it, even though GitHub delivered the webhook once and both users' agents legitimately matched. Fold match.user_id into the id: f"{delivery_id}:{match.user_id}:{agent.name}". Genuine redeliveries (same user, same agent, same delivery) still produce the same id and are deduped; two agents - same-named or not, same user or not - on one delivery now always keep distinct ids. Tests: new cross-user regression pins that two users' same-named agents both dispatch instead of the second being deduped against the first; the existing per-(delivery, agent) test and the literal id assertion in test_delivery_id_populates_inbound_dedupe_identity are updated for the new id shape. * fix(channels): point cross-user dedupe comment at its regression test Replace the inline reviewer-handle attribution with a pointer to test_dedupe_identity_distinguishes_same_agent_name_across_users, which ages better than a person's name in committed code. |
||
|
Daoyuan LiandGitHub
|
0dd90ccfde |
fix(agents): require config.yaml in update_agent's legacy-agent guard (#4166)
update_agent (the harness tool) and PUT /api/agents/{name} (the same
operation over HTTP) share an identical guard meant to block updates to
an agent that only exists in the legacy shared layout. The guard checked
bare directory existence:
if not agent_dir.exists() and paths.agent_dir(name).exists():
When memory is enabled, the first time a user chats with a legacy shared
agent, the memory writer creates a per-user directory containing only
memory.json (no config.yaml). agent_dir.exists() is then true, so the
guard never fires: the tool falls through to load_agent_config, which
resolves through to the legacy shared config via the already-hardened
resolve_agent_dir, and silently writes a brand-new config.yaml/SOUL.md
into the memory-only directory. That forks the agent for just this user;
every other user keeps reading the original shared config forever, with
no error or warning.
resolve_agent_dir itself was already hardened against exactly this
failure mode: it requires config.yaml to exist, not just the directory.
Mirror that condition at both call sites here.
|
||
|
Daoyuan LiandGitHub
|
656f6b364c |
fix(skills): recognize fully deleted public skill packages in review CI (#4169)
select_skill_packages() resolved every changed non-SKILL.md path to its owning package via an unconditional depth-3 fallback, then queued that path for review. When a PR deletes an entire public skill package (not just SKILL.md, but its scripts/assets/etc. too), the fallback still returned the package directory even though it no longer exists on disk post-deletion. The review CLI then reported a false structure.missing-skill-md blocker for a path that isn't there, failing CI on a routine, correct package removal. Skip a resolved package only when every changed file under it was a deletion and the package directory itself is gone from disk - i.e. the whole package was intentionally removed. A package left in a broken/partial state (e.g. SKILL.md deleted while sibling files remain) still resolves to an existing directory, so it is unaffected and continues to be queued and flagged. |
||
|
Daoyuan LiandGitHub
|
51bb19fa1b |
fix(channels): drop redundant GitHub review-comment webhook fan-out (#4131)
* fix(github): drop redundant pull_request_review_comment fan-out noise
GitHub fires one pull_request_review_comment webhook per inline comment
attached to a pull_request_review submission, in addition to the single
pull_request_review event for the review itself. A bot reviewer like
CodeRabbit commonly leaves 20-30 inline comments per review, flooding
the webhook with near-duplicate deliveries that carry nothing an agent
doesn't already have -- it fetches every inline comment itself via
`gh api` when it processes the parent pull_request_review event.
Filter these out in fanout_event() before the registry lookup / per-
agent loop, since the redundancy is a property of the event itself, not
of any specific agent binding. A companion comment is identified by
pull_request_review_id being set (it belongs to a review) and
in_reply_to_id being absent (it is not itself a reply within an
existing thread -- that case is a genuine new interaction and must
still fire).
* fix(channels): scope review-comment suppression to bindings that also see the review
The redundant pull_request_review_comment filter suppressed every
companion comment unconditionally, before the registry lookup even ran.
That premise only holds for a binding that also subscribes to
pull_request_review on the same repo -- events are opt-in per binding,
so a binding registered for pull_request_review_comment alone never
receives the parent review event and the companion comments were its
only delivery of the review's inline content. Suppressing those too was
a silent, total loss for that binding, not noise reduction.
Move the check into the per-agent loop so it only suppresses a matched
binding's companion comment when that same binding also has an active
pull_request_review trigger on this repo, reusing the existing registry
lookup rather than re-walking bindings by hand. Bindings subscribed to
pull_request_review_comment alone now always fire, matching the
opt-in-per-binding contract documented in triggers.py.
* fix(channels): close require_mention gap in review-comment redundancy gate
willem-bd's second review round found that the per-binding redundancy gate
(commit
|
||
|
Daoyuan LiandGitHub
|
cdefd4a8aa |
fix(mcp): invalidate tools cache on config content + path, not just newer mtime (#4124)
* fix(mcp): invalidate tools cache on config content + path, not just newer mtime The MCP tools cache invalidated only on a strict extensions-config mtime `>` comparison and tracked no resolved config path, so `_is_cache_stale()` missed: - content changes with an unchanged mtime (same-second edits; object-store / network mounts that do not bump mtime); - content changes with a backward mtime (git checkout, cp -p / backup restore, tar / rsync preserving timestamps); - a resolved-path switch to a different config file with mtime <= the recorded value (structurally invisible — no path was tracked at all). On multi-worker (uvicorn/gunicorn) or stale-mtime deployments this leaves the LangGraph-embedded runtime and every non-writer worker serving stale MCP tools after `PUT /api/mcp/config`, breaking the module's documented promise that changes made through the Gateway API are reflected in the embedded runtime. Record the resolved config path and a `(mtime, size, sha256)` content signature at initialization and invalidate when the path OR the signature differs (`!=`), mirroring `config/app_config.py::get_app_config()` so the two runtime-editable config files share one content-based staleness signal. The per-call stat was already paid, and the small-file sha256 matches the cost app_config pays per request. The "config missing / not yet initialized" no-op behavior and the cache reset endpoint are preserved. Adds backend/tests/test_mcp_cache.py covering all three failure modes plus unchanged-file and forward-edit sanity cases. Also folds in an incidental backend/AGENTS.md doc-sync: the restart-required field list was missing `scheduler` and `run_ownership`, both present in reload_boundary.py::STARTUP_ONLY_FIELDS. * test(mcp): pin cache staleness contracts raised in review Two review observations on the content-signature cache fix, both raised as non-blocking design questions rather than bugs: - Whether relying on mtime+size alone (skipping the sha256) could ever be "optimized" back in, reopening the narrow same-second / identical-length swap gap the signature was built to close. - Whether the extensions config being deleted entirely after a successful init leaves the cache in a defined state, since current_signature flips to None and _is_cache_stale() returns False. Neither is a behavior change: both were already the intended contract, preserved verbatim from the pre-fix mtime-only code (which also returned False once the file could no longer be stat-ed). Record the reasoning inline and add regression tests that pin each contract so a future change cannot alter either silently: - test_same_mtime_same_size_swap_is_stale: a same-length server-name swap that leaves mtime AND size unchanged (the precise scenario from review, sharper than the existing same-mtime test, which also changes size) is still caught only because the sha256 is computed unconditionally. - test_config_deleted_after_init_is_not_stale: deleting the config file after init keeps the cache serving its last-known-good MCP tools instead of invalidating into an unconfigured state. Both new tests were confirmed to fail against a deliberately reintroduced version of the regression they guard (hash short-circuit / removed None-guard), then confirmed to pass against the real code. |
||
|
Daoyuan LiandGitHub
|
3e7baba39a |
fix(models): apply stream_chunk_timeout default to all BaseChatOpenAI subclasses (#4102)
* fix(models): apply stream_chunk_timeout default to all BaseChatOpenAI subclasses The 240s stream_chunk_timeout default (issue #3189, PR #3195) was scoped to a class-path allowlist of only ChatOpenAI and PatchedChatOpenAI. Every other OpenAI-compatible provider that subclasses BaseChatOpenAI — VllmChatModel, MindIEChatModel, PatchedChatDeepSeek, PatchedChatMiMo, PatchedChatStepFun and PatchedChatMiniMax — was excluded, so they kept langchain-openai's aggressive 120s built-in chunk-gap timeout and, worse, silently discarded a user's explicit stream_chunk_timeout override from config.yaml. Issue #3189 was itself reported on mimo-v2.5 (PatchedChatMiMo), the exact class the original fix left out. Gate the injection on issubclass(model_class, BaseChatOpenAI) instead of the string allowlist, so any OpenAI-compatible subclass inherits the default and honors an explicit override. Genuinely non-OpenAI clients (e.g. ChatAnthropic) stay excluded and still have the kwarg dropped before it reaches a constructor that would divert it into model_kwargs and fail at request time. * fix(models): address review nits on stream_chunk_timeout default Correct the module-level comment above _DEFAULT_STREAM_CHUNK_TIMEOUT_SECONDS: langchain-openai's built-in stream_chunk_timeout default is 120s, not 60s (BaseChatOpenAI.stream_chunk_timeout's default_factory reads LANGCHAIN_OPENAI_STREAM_CHUNK_TIMEOUT_S with a 120.0 fallback). Simplify the BaseChatOpenAI gate in _apply_stream_chunk_timeout_default from `isinstance(model_class, type) and issubclass(model_class, BaseChatOpenAI)` to just `issubclass(...)`. The sole caller passes model_class from resolve_class(), which already raises before returning anything that isn't a type, so the isinstance half can never be False there. Also soften the docstring's non-OpenAI-client bullet: ChatAnthropic declares extra="ignore" and silently drops an unrecognized kwarg rather than diverting it into model_kwargs and failing at request time (that failure mode is specific to other OpenAI-style clients). |
||
|
Daoyuan LiandGitHub
|
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. |
||
|
Daoyuan LiandGitHub
|
2bd0f56a0f |
fix(subagents): classify recursion-capped LLM error fallbacks as failed (#4056)
The GraphRecursionError except-block in SubagentExecutor._aexecute derives usable_partial from the last AIMessage's raw non-empty text, without checking _extract_llm_error_fallback (#4042) first. A handled provider failure (LLMErrorHandlingMiddleware's deerflow_error_fallback marker) always carries non-empty user-facing text, so when it lands on the same turn that trips max_turns, it is indistinguishable from genuine partial output and gets misclassified as a completed task instead of the failed provider error it is. Consult _extract_llm_error_fallback in this except-block too, same as the normal-completion path above it, and classify FAILED with stop_reason=turn_capped when it detects the marker. |
||
|
Daoyuan LiandGitHub
|
08fd218b83 |
fix(sandbox): use os.sep in reverse-resolve containment check on Windows (#4058)
* fix(sandbox): use os.sep in reverse-resolve containment check on Windows Path.resolve() always renders with the native separator (backslash on Windows), but _reverse_resolve_path's containment check hardcoded a "/" suffix when testing whether a resolved path is nested under a mapping's local root. Only the exact-root case (no separator needed) ever matched; every nested path fell through to the "no mapping found" branch and returned the raw host path -- leaking the real username and full directory tree into list_dir/glob/grep results and bash output masking instead of the virtual /mnt/user-data/... path. _is_read_only_path already does the equivalent check correctly via os.sep, so this aligns _reverse_resolve_path with that pattern: the containment check now compares with os.sep, and the extracted relative portion is normalized to forward slashes before being spliced into the (always POSIX-style) container path. Also fixes a same-file cosmetic bug in list_dir's virtual sub-directory overlay: it compared a bare child name (e.g. "workspace") against a set of full container paths, so the already-listed guard never matched and a mount whose subdirectory the underlying scan already found (the common case for /mnt/user-data/workspace, uploads, outputs) was appended a second time. Continues the same separator-bug class already fixed in this file by #3869 (forward-direction command resolution) and #4035 (reverse regex-boundary matching); neither touched this containment check. * test(sandbox): add host-OS-independent regression test for the os.sep containment fix _reverse_resolve_path's os.sep containment check (and the paired lstrip(os.sep).replace(os.sep, "/") extraction) has no test that would fail if reverted: backend CI runs only on ubuntu-latest, where os.sep == "/" makes the pre-fix hardcoded "/" and the current os.sep form observationally identical, so a plain POSIX-path test can't discriminate between them. Add a test that forces the Windows code path independent of host OS by monkeypatching os.sep to "\" and stubbing both the module's Path name and the sandbox's cached _resolved_local_paths to return backslash-joined strings, mirroring what real WindowsPath.resolve() produces -- without touching the filesystem or requiring an actual Windows host. Verified this fails with the raw host path leaking through when the os.sep fix is reverted to the hardcoded "/" form, and passes with the fix in place. |
||
|
Daoyuan LiandGitHub
|
c4b651fb9d |
Fix lost loop_capped stop reason when a subagent's run_id is None (#4059)
LoopDetectionMiddleware._get_run_id used a truthiness check that collapsed a present-but-None run_id to the same "default" key as a totally absent one. SubagentExecutor sets context["run_id"] = self.run_id unconditionally, so run_id is genuinely None for an embedded/TUI-dispatched subagent, and later reads the stop reason back with that same raw attribute via consume_stop_reason(self.run_id). The write (keyed "default") and the read (keyed None) disagreed, so a genuine loop-detection hard-stop's loop_capped reason was silently dropped instead of reaching the lead. Align _get_run_id with TokenBudgetMiddleware's key-presence-based version, which does not have this bug: return the context value as-is when the key is present (None included), and fall back to a per-runtime-unique key only when the key is absent. |
||
|
Daoyuan LiandGitHub
|
74392e1470 |
Fix require_mention gating on whitespace-only bot_login/mention_login (#4055)
github.bot_login and trigger.mention_login were read raw in the require_mention precedence chain, so a whitespace-only value (e.g. " ") never fell through to the working fallback the chain documents - Python truthiness lets " " win an `or` chain over agent.name or the operator default. The third link (channels.github.default_mention_login) was already correctly normalized and pinned by test_operator_default_blank_string_treated_as_none; this closes the gap for the other two by normalizing GitHubTriggerConfig.mention_login and GitHubAgentConfig.bot_login once, at the config layer, via a field_validator mirroring the pattern already used elsewhere in the config package. |
||
|
Daoyuan LiandGitHub
|
5edc7a889e |
fix(security): neutralize prompt-injection tags in web_capture results (#4099)
The remote-content allowlist in ToolResultSanitizationMiddleware (`_REMOTE_CONTENT_TOOL_NAMES`) covered web_fetch / web_search / image_search but not web_capture, which was added later. The Browserless web_capture tool embeds the target site's `X-Response-Status` reason phrase — free-form text controlled by whatever server is being captured (RFC 7230 §3.1.2) — into its result message via `_target_status_warning`. A malicious page could therefore forge a `<system-reminder>` block (or a `--- END USER INPUT ---` boundary marker) through web_capture that would be escaped for web_fetch, letting attacker-influenced remote content reach the model as authoritative framework context. Add "web_capture" to the allowlist so its result is structurally neutralized for parity with the other remote-content tools. This extends the same defense introduced in #4002 to the one built-in remote-content tool it did not yet cover. Add regression tests that build the web_capture result the way community/browserless/tools.py does (real `_target_status_warning` + `BrowserlessScreenshotResult`) and assert the forged tags/boundary markers are escaped, while a benign status warning is preserved unchanged. |
||
|
Daoyuan LiandGitHub
|
88b0484898 |
fix(channels): validate channel provider before resolving its config (#4100)
`_provider_config` resolved a request-supplied provider name with an
unallowlisted `getattr(config, provider, None)`. `ChannelConnectionsConfig`
carries non-provider attributes -- the `enabled` and `require_bound_identity`
bool fields plus the `provider_status` method -- so a name matching one of
them returned that attribute instead of falling through to the intended 404.
Callers (e.g. `POST /api/channels/{provider}/connect`, reachable by any
authenticated user) then dereferenced the bool/method as a provider config,
crashing with `AttributeError` -> HTTP 500.
Validate `provider` against the `_PROVIDER_META` allowlist before the lookup,
matching how `_credential_fields` / `_connect_instruction` / `_connect_url`
already gate provider names, so unknown providers get a clean 404.
Add a parametrized router regression test covering `enabled`,
`require_bound_identity`, `provider_status`, and an unknown name.
|
||
|
Daoyuan LiandGitHub
|
c143c0415b |
fix(config): Make the sync checkpointer honor the unified database config (#3994)
* Make the sync checkpointer honor the unified database config
The sync checkpointer factory (`get_checkpointer` and `checkpointer_context`)
read only the legacy `checkpointer:` config section and fell back to
`InMemorySaver` when it was absent — it never consulted `database:`. The async
`make_checkpointer` factory and both sync/async Store providers already resolve
the unified `database:` section (legacy `checkpointer:` takes precedence,
otherwise `database:` drives the backend), so the sync checkpointer was the
lone outlier.
Consequence: with `database: {backend: sqlite|postgres}` and no legacy
`checkpointer:` section, the sync checkpointer silently returned `InMemorySaver`
while the Store — same process, same config — correctly persisted to
sqlite/postgres. Embedded callers (`DeerFlowClient`) and the TUI hit this: e.g.
the TUI writes `threads_meta` rows to sqlite (thread appears in the Web UI) but
its checkpoints went to memory and were lost on exit. This also contradicts
backend/AGENTS.md ("the unified `database` section selects the Gateway's
LangGraph checkpointer, LangGraph Store, and DeerFlow SQL repositories").
Mirror the sync Store provider: add `_resolve_checkpointer_config` /
`_get_checkpointer_config` (legacy precedence, else translate `database:` into
a CheckpointerConfig) and route both the singleton and the context manager
through them. The Gateway is unaffected — it uses the async path.
Adds a TestCheckpointerDatabaseConfig suite mirroring the Store's database
tests (singleton + context-manager honor `database:`, legacy precedence,
explicit-memory, missing-config fallback). The two `uses_database_config`
tests fail (return InMemorySaver) before the fix and pass after.
* Handle database=None in sync checkpointer resolution
The unified-config change made `checkpointer_context` / `get_checkpointer`
consult `app_config.database` when no legacy `checkpointer:` section is set.
`AppConfig.database` is always a `DatabaseConfig` in production, but the
existing regression test for issue #1016
(`test_checkpointer_none_fix.py::test_sync_checkpointer_context_returns_in_memory_saver_when_not_configured`)
mocks the app config with `database` left unset, exercising a `database=None`
path that now raised `ValueError: Unknown database backend`.
Mirror the async `make_checkpointer` factory, which already tolerates
`database=None` (falls back to memory), by treating a `None` database as the
memory backend in `_resolve_checkpointer_config`. Update the sync test to set
`mock_config.database = None`, matching its async sibling in the same file.
* Address review: mirror None-guard in store resolver, add sqlite coverage
|
||
|
Daoyuan LiandGitHub
|
79611673d7 |
Fix circuit breaker wedging after a non-retriable half-open probe (#3991)
* Fix circuit breaker wedging after a non-retriable half-open probe When the circuit breaker is half-open it admits a single probe call by setting `_circuit_probe_in_flight = True`. If that probe raised a *non-retriable* error (e.g. quota/auth), the except block skipped both `_record_failure()` (correct - business errors must not trip the breaker) and any probe reset, so the circuit stayed `half_open` with `_circuit_probe_in_flight = True` permanently. Every later call then fast-failed in `_check_circuit()` forever, because no call could run the handler to reach `_record_success` / `_record_failure`. Release the probe on the non-retriable path (mirroring the existing GraphBubbleUp handler) so the next call admits a fresh probe. The breaker still never trips on non-retriable errors. Applied to both the sync and async paths. Adds sync + async regression tests asserting the probe is released and the next `_check_circuit()` re-admits a probe. * Address review: extract _release_half_open_probe helper |
||
|
Daoyuan LiandGitHub
|
a2a949b178 |
Fix UnboundLocalError in memory injection when facts are empty (#3992)
`format_memory_for_injection` bound `facts_header` / `all_fact_lines` only inside the `if isinstance(facts_data, list) and facts_data:` block, but the structure-aware overflow-truncation path at the end of the function references both unconditionally. When a user's memory has sizeable user-context / history (so `sections` is non-empty and the assembled output exceeds `max_tokens`) but an empty or missing `facts` list, that block is skipped, so the truncation branch hits `UnboundLocalError: cannot access local variable 'all_fact_lines'` and aborts memory injection entirely. Hoist the two initializers to function scope, alongside the existing `guaranteed_line_tokens = 0`, so they are always bound. Behaviour is unchanged when facts are present. Adds a regression test (empty facts + oversized user context) that fails with UnboundLocalError before the fix and truncates gracefully after. |
||
|
Daoyuan LiandGitHub
|
1fa91fa39d |
Fix AttributeError in ThreadDataMiddleware when runtime.context is None (#3989)
`before_agent` guards `context = runtime.context or {}` at the top, but the
`run_id` stamp on a trailing HumanMessage still read the raw `runtime.context`,
so a None context (thread_id resolved from `config.configurable`) plus a
HumanMessage last message raised `AttributeError: 'NoneType' object has no
attribute 'get'`. Use the guarded local `context` instead.
Adds a regression test.
|
||
|
Daoyuan LiandGitHub
|
c0b917cce2 |
Fix KeyError in staleness review when a fact has no id (#3993)
The apply-time staleness guardrail built ``candidate_ids`` with a direct
``f["id"]`` access over ``_select_stale_candidates`` output:
candidate_ids = {f["id"] for f in _select_stale_candidates(current_memory, config)}
Every other fact access in ``updater.py`` uses ``f.get("id")``; this was the
lone direct-subscript outlier. An aged, non-protected fact that lacks an
``id`` key — common in legacy / hand-edited / migrated ``memory.json`` — is a
valid staleness candidate, so it reached ``f["id"]`` and raised
``KeyError: 'id'``, aborting the entire background memory-update cycle for
that user. The guardrail runs unconditionally (independent of the
``staleness_review_enabled`` flag), so any id-less aged fact triggers it as
soon as the LLM returns a non-empty ``staleFactsToRemove``.
Skip id-less candidates when building the intersection set. They can never
be targeted by the id-based removal set anyway, so behaviour is otherwise
unchanged.
Adds a regression test with an aged, id-less fact that raises KeyError
before the fix and applies cleanly after.
|
||
|
Daoyuan LiandGitHub
|
d300e3597f |
Correct memory-config documentation to match the code (#3995)
Two memory-config docs disagreed with the actual defaults/behavior in `config/memory_config.py` and `agents/memory/storage.py`: - backend/AGENTS.md: `staleness_age_days` was documented as "default: 180; range: 1-3650" but the field is `default=90, ge=30, le=365`; `staleness_max_removals_per_cycle` was documented as "default: 5; range: 1-20" but the field is `default=10, ge=1, le=50`. - config.example.yaml: the `storage_path` comment said "Path relative to backend directory", but a relative `storage_path` resolves against the data `base_dir` (`get_paths().base_dir / p`), not the backend working directory, and an absolute path is what opts out of per-user isolation (matching the field's own description in `memory_config.py`). Doc-only change; no behavior change. |
||
|
Daoyuan LiandGitHub
|
eb8eec21ce |
Return the full-resolution image URL from image_search (#3990)
`image_search` set both `image_url` and `thumbnail_url` to the DDGS result's `thumbnail` field, so the tool never returned the full-resolution image even though its docstring/usage_hint promise reference-quality images (and a result with no `thumbnail` returned an empty `image_url`). DDGS `.images()` exposes the full-res source under the separate `image` key — read that for `image_url`, matching the serper/brave providers which keep image vs thumbnail distinct. Adds a regression test. |