mirror of
https://github.com/bytedance/deer-flow.git
synced 2026-07-21 02:05:45 +00:00
main
998
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 Daoyuan LiandGitHub
|
8153e68eb8 |
fix(channels): escape Slack reserved chars before mrkdwn conversion (#4197)
* fix(channels): escape &/</> in Slack outbound text before mrkdwn conversion Slack requires callers to replace &, <, and > with their HTML entity equivalents before sending message text, since an unescaped <...> triggers Slack's own mention/link syntax (e.g. <@USERID>, <http://url|label>). SlackChannel.send() ran msg.text through the markdown-to-mrkdwn converter and sent the result straight to chat_postMessage with no escaping step, so technical/code content like "if a < b && b > c:" would arrive as literal Slack markup and render broken or misinterpreted. Escaping must happen before the mrkdwn conversion, not after: the converter emits its own <url|label> syntax for real markdown links ([text](url)), and that generated syntax must reach Slack unescaped. Escaping raw input first (via html.escape(text, quote=False), which replaces & before </>) and leaving the converter's output alone satisfies both requirements. * fix(channels): preserve a line-leading > as Slack's blockquote marker _escape_slack_text's html.escape(text, quote=False) replaces every ">" with ">", including a ">" at the start of a line -- Slack's own mrkdwn blockquote marker, which the converter otherwise passes through unchanged. Agent output using markdown blockquotes for quoting/callouts lost its blockquote styling and arrived as a literal "> " prefix instead of a rendered blockquote. Only "&" and "<" neutralize Slack's "<...>" mention/link syntax; ">" is special to Slack only at the start of a line. Restore a line-leading ">" to a literal character after escaping (re.sub with MULTILINE), so the mrkdwn converter still renders it as a blockquote; a "<"/"&" anywhere, and a ">" that is not at a line start, still escape. New tests cover the line-start preservation, that the exemption does not widen into "never escape '>'" (a mid-line/non-leading ">" still escapes), and that restoration applies per line in multiline text; all three revert cleanly against the unconditional html.escape() to reproduce the blockquote-corruption bug. Full test_channels.py (259 tests) plus ruff check/format are clean. |
||
|
ChenglongZandGitHub
|
ca16b64b26 |
feat(agent): support config-declared lead middlewares (#3964)
* feat(agent): support config-declared lead middlewares * fix(agent): preserve configured extension middlewares * fix(agent): address middleware extension review * fix(agent): tighten middleware extension docs and tests |
||
|
Tianye SongandGitHub
|
8511fa6aa3 |
fix(memory): consolidated facts inherit expected_valid_days from sources (#4225)
* fix(memory): consolidated facts inherit expected_valid_days from sources Consolidation (#3996) and per-fact expected_valid_days (#4143) were both authored by the same contributor but never connected: the consolidated new_fact carried the newest source's createdAt but no expected_valid_days, so _effective_fact_staleness_age fell back to the global staleness_age_days. A merge of several 200-day-old stable facts (each evd=3650) would land with no evd, read as a 90-day window, and re-enter the staleness candidate set on the very next cycle - the merge discarded the lifetime signal of the underlying information and contradicted consolidation's premise (these are stable, related facts worth synthesising). Fix: the merged fact inherits expected_valid_days set so it is re-reviewed at the EARLIEST source review deadline (min(createdAt + expected_valid_days) across sources, relative to the merged fact's createdAt = the newest source's). A merge combines details from every source, so a volatile sub-detail (evd=7) must not inherit a stable source's 3650-day window and escape staleness review for years - staleness KEEP/REMOVE is the only path that re-validates a merged fact, so biasing toward the soonest deadline keeps uncertain merges re-checked sooner. A source already past its deadline yields a minimal positive window (review next cycle) rather than the global fallback, which would defer an overdue review. Capped at the creation-time staleness_max_lifetime_multiplier like any new fact. Omitted when no source carries a valid evd (legacy facts fall back to the global age at read time, matching pre-feature behaviour). DRY: extract _read_expected_valid_days(fact) -> int | None, the shared type rule (int/float, reject bool, coerce to int BEFORE the > 0 guard) previously inlined in four places - _normalize_memory_update_fact, _effective_fact_staleness_age, the newFact creation cap, and consolidation inheritance. All four call the single helper. Coercing before the guard matters for values in (0, 1): 0.5 passes a raw > 0 check but truncates to 0, which would violate the helper's "positive int or None" contract; the order now matches the original _normalize_memory_update_fact rule. No prompt/schema change: consolidation's prompt does not surface source evd to the LLM, so asking the model to assign a merged lifetime would be guessing without signal. Source inheritance is deterministic and always available. Tests: consolidation evd cases now use time-stable createdAt (relative to now via a _days_ago helper) covering - earliest-deadline selection, creation-cap clamp, omit when no source evd, volatile source governs the deadline (and re-enters staleness next cycle), overdue source clamps to a minimal window, float coercion. Plus TestReadExpectedValidDays / TestEffectiveFactStalenessAge regression cases for the (0, 1) coercion-order fix. * fix(memory): reject non-finite expected_valid_days before int coercion The shared _read_expected_valid_days helper (introduced when consolidating the evd type rule across four call sites) coerces with int(raw) before the > 0 guard - reversing the original _normalize_memory_update_fact order so that a fractional 0.5 does not leak as 0. But int(raw) raises for non-finite floats: int(nan) raises ValueError and int(inf)/int(-inf) raise OverflowError. Python's JSON decoder accepts NaN / Infinity as floats by default, so a single malformed expected_valid_days in a hand-edited memory.json would abort staleness selection or consolidation instead of falling back to the global lifetime. On main, _effective_fact_staleness_age checked raw > 0 first, so NaN fell back safely (nan > 0 is false) - but inf did NOT (inf > 0 is true, so main also crashed on inf). This helper is now the persisted-fact read path for both staleness and consolidation, so the regression (and the pre-existing inf crash) must be closed here. Fix: require math.isfinite(float(raw)) before the int() coercion, then keep the existing positivity check and fallback. NaN / +/-inf all return None, so callers fall back to the global staleness_age_days. Normal int/float values (including large ones) are unaffected - isfinite is a no-op for them. Tests: - TestReadExpectedValidDays.test_rejects_non_finite_values - NaN, inf, -inf return None (not raise). - TestEffectiveFactStalenessAge.test_falls_back_for_non_finite_values - the persisted-fact read path returns the global age for each, no raise. - test_consolidation_with_non_finite_source_evd_does_not_raise - end-to-end: a NaN-evd source merged with a stable source does not abort consolidation; the NaN source's effective lifetime falls back to the global 90 and its deadline participates in the earliest-deadline computation. * test(memory): hoist _select_stale_candidates import + tidy deadline docstring Two review nits from the latest pass: - `_select_stale_candidates` was imported inline inside three test methods; hoisted to the module-level import block so the dependency is declared once. - `test_consolidated_evd_volatile_source_with_equal_created_at_future_deadline` had an abandoned calculation in its comment ("3 + 7 = 10 ... minus 3 already elapsed = 7? No:") that could mislead future readers into thinking elapsed-since-creation factors into the inherited window. Collapsed to a single clear line stating the window is relative to the merged createdAt, regardless of the source's current age. * fix(memory): reject huge-int expected_valid_days above timedelta.max.days _read_expected_valid_days routed every numeric value through float(raw) for the math.isfinite guard, but Python's JSON decoder parses an integer literal with no decimal point as an arbitrary-precision int (unlike 1e400, which decodes to float inf). So a hand-edited memory.json carrying "expected_valid_days": 10**400 raised OverflowError in float(raw) before math.isfinite was ever called - exactly the malformed-field-aborts-everything scenario the helper's docstring claims to prevent. The earlier non-finite fix only closed the float cases (NaN / +/-inf / 1e400). A huge int below the float limit but above timedelta.max.days (e.g. 10**12) would pass the helper and raise OverflowError downstream in timedelta(days=evd) during staleness selection or consolidation - the same crash fancyboi999 flagged for extend_by_days, just reached via a stored evd. Fix: branch on type so an int never passes through float() (matching the reviewer's suggestion), AND cap the returned int at timedelta.max.days (999999999) so the downstream timedelta(days=evd) call cannot overflow either. The float branch keeps the isfinite + int() coercion. Both branches share the 0 < evd <= timedelta.max.days positivity/range check. Normal values are unaffected - any legitimate expected_valid_days is far below the cap (the config ceiling staleness_max_extension_days tops out at 36500). Tests (all three layers): - TestReadExpectedValidDays.test_rejects_huge_int_above_timedelta_max - 10**400, 10**12, 10**9, timedelta.max.days+1 return None; timedelta.max.days itself is accepted. - TestEffectiveFactStalenessAge.test_falls_back_for_huge_int_above_timedelta_max - the persisted-fact read path returns the global age, no raise. - test_consolidation_with_huge_int_source_evd_does_not_raise[1e400|1e12|1e9] - parametrized end-to-end: a huge-int-evd source merged with a stable source does not abort consolidation; the bad source falls back to the global 90. * fix(memory): guard datetime arithmetic, not just timedelta construction The huge-int fix capped _read_expected_valid_days at timedelta.max.days, but that only proves timedelta(days=evd) can be constructed - adding it to a real fact timestamp still overflows datetime.max. @fancyboi999 reproduced it: a source with expected_valid_days=timedelta.max.days raises "OverflowError: date value out of range" at dt + timedelta(...) in the new consolidation deadline calculation. capping at timedelta.max.days was another patch chasing the next overflow boundary, not a real close. Root cause: the helper was doing datetime-range validation, but the safe bound depends on the datetime the evd is added to, not on the evd alone. So the responsibility moves to the arithmetic site, with try/except as the terminal guard - no concrete upper bound to be wrong about. Changes: - _read_expected_valid_days returns any positive int (huge ints included, not routed through float). Its job is type/positivity validation only. - New _safe_add_days(dt, days) -> datetime | None wraps dt + timedelta(days), returning None on OverflowError/ValueError. This is the terminal guard - there is no further boundary to overflow because try/except catches any datetime-range failure regardless of magnitude. - _select_stale_candidates uses _safe_add_days(now, -effective_age); a None result means the window is unrepresentably large, so the fact cannot yet be stale and is skipped (not selected). - Consolidation computes each source's deadline via _safe_add_days; a source whose deadline overflows falls back to the global staleness_age_days deadline (same treatment as a legacy no-evd source), so one malformed field cannot abort the merge. Normal values are unaffected - any legitimate expected_valid_days is far below the overflow boundary (the config ceiling staleness_max_extension_days tops out at 36500). Tests: - TestSafeAddDays: normal/negative shifts; 10**400/10**12/10**9 return None; timedelta.max.days (the exact reproduced value) returns None, not raises. - TestSelectStaleCandidates.test_huge_evd_does_not_abort_selection: a fact with a huge evd is skipped, not selected, and selection does not raise. - test_consolidation_with_huge_int_source_evd_does_not_raise now parametrized over [1e400, 1e12, 1e9, timedelta.max.days] - the last is the value that constructs a valid timedelta but overflows datetime arithmetic. - Helper/read-path tests updated to assert huge ints are returned as-is (the overflow guard is no longer in the helper). |
||
|
Willem JiangandGitHub
|
1073393e07 |
Revert "fix(memory): follow config reload in get_memory_config (#4216)" (#4320)
This reverts commit
|
||
|
Yufeng HeandGitHub
|
cd0f1e2212 |
fix(memory): follow config reload in get_memory_config (#4216)
* fix(memory): follow config reload in get_memory_config get_memory_config() returned the _memory_config singleton directly, and that singleton is only refreshed as a side effect of get_app_config() reloading (via _apply_singleton_configs -> load_memory_config_from_dict). A reader that reaches memory config without going through get_app_config() first -- e.g. the agent factory deciding whether to bind the memory tools via `feat.memory_config or get_memory_config()` -- therefore saw a stale memory.mode after a config.yaml edit, even though memory.* is documented as hot-reloadable. The agent kept the old mode until some unrelated get_app_config() call happened to refresh the singleton. Trigger the same signature-checked reload inside get_memory_config() so the singleton follows the config file, mirroring how the other config getters already resolve through get_app_config(). When no config file is on disk (tests, or a minimal deployment running purely on defaults / set_memory_config) get_app_config() raises FileNotFoundError; swallow it and fall back to the in-memory singleton so the accessor is never more fragile than before. Signed-off-by: Yufeng He <40085740+he-yufeng@users.noreply.github.com> * test(memory-config): pin malformed-config behavior for get_memory_config Address review feedback: the narrow except FileNotFoundError means a config file that exists but fails validation surfaces through get_memory_config() rather than being swallowed, consistent with every other unguarded get_app_config() caller. Add a regression test pinning that a malformed edit raises ValidationError and leaves the last-good singleton intact (not half-applied), and document the contract on the guard. Signed-off-by: Yufeng He <40085740+he-yufeng@users.noreply.github.com> --------- Signed-off-by: Yufeng He <40085740+he-yufeng@users.noreply.github.com> |
||
|
hataaandGitHub
|
92c8f2f03b |
feat(authz): add built-in RBAC provider and provider factory (#4260)
* feat(authz): add built-in RBAC provider and provider factory (Phase 1A-2, #4063) Phase 1A-2: RBAC provider + provider factory. No runtime behavior change. New authz/rbac.py — RbacAuthorizationProvider: - allow: '*' / True / list / [] / missing → deny-wins semantics - deny always overrides all allow forms - resource name explicit mapping (tool→tools, model→models, etc.) - unknown/missing role raises ValueError (never silent allow) - config compiled to immutable frozensets at construction - filter_resources preserves order, no mutation, consistent with authorize - sync == async decisions New authz/runtime.py — resolve_authorization_provider: - disabled → None (no import attempted) - enabled + no provider → ValueError - invalid class path / construction failure → ValueError with path - isinstance Protocol check post-construction - no caching, no fail_closed/default_role injection 48 tests (37 RBAC + 11 factory). No config schema change, no config_version bump. Per RFC #4063 Phase 1A-2. Layer 1/Layer 2 wiring deferred to Phase 1B. * fix(authz): reject unknown RBAC provider config Fail fast on misspelled top-level RBAC settings, cover factory error propagation, and record Phase 1B policy and audit caveats. * fix(authz): reject unreachable resource aliases Fail fast when RBAC config uses reserved request-side aliases, preserve same-name and custom resources, and add regression coverage for every mapped alias. * fix(authz): validate RBAC request identifiers |
||
|
|
5eb59cb130 |
fix(sandbox): stop multi-worker orphan reconcile from killing peer sandboxes (#4221)
* fix(sandbox): stop multi-worker orphan reconcile from killing peer sandboxes Docker sandboxes are shared across gateway workers, but each worker kept its own in-memory warm pool. Startup reconciliation adopted every running container, so a peer idle reaper could destroy sandboxes another worker still owned and tool calls hit 502 / Connection refused. Add file-based ownership leases under sandbox-leases/, only adopt true orphans, refuse idle/replica/shutdown destroy while a foreign lease is live, and renew the lease on create/get/release/reclaim. Fixes #4206 * fix(sandbox): close lease fail-open, hot-path IO, and check→destroy race Address review of the multi-worker orphan lease (#4206): - read_lease returns None only for a genuinely-absent lease and raises (CorruptLeaseError/OSError) when a lease is unreadable or corrupt, so the ownership check fails closed instead of mistaking an unprovable peer lease for a free container. clear_lease still removes a stuck/corrupt file. - get() no longer renews the lease (blocking mkdir/fsync/os.replace on the event loop path used by ensure_sandbox_initialized_async); active leases are renewed off the event loop from the idle checker (_renew_active_leases). - The ownership check and container stop run under a per-sandbox flock guard (lease_ownership_guard); every lease write takes the same guard so a peer's touch cannot interleave with a destroy. Same-host multi-worker scope, not a multi-pod distributed lock. Also fixes the ruff format lint on the branch. Adds regression tests: corrupt and unreadable lease fail closed, a tests/blocking_io anchor keeping get() non-blocking on the event loop, and a peer-touch/destroy interleave test. * fix(sandbox): share container ownership across gateway instances Rework of the #4206 fix per review: ownership state is shared through a third-party service instead of being maintained per gateway instance, following the stream_bridge precedent (sandbox.ownership.type: memory | redis). The file lease and its same-host flock guard are deleted, not ported — they only covered workers on one host, while the deployment that hits #4206 is a load-balanced multi-instance gateway. A lease answers "who reaps this container", not "who may use it". Containers are deterministic per (user, thread), so consecutive turns legitimately land on different instances: take() transfers ownership on acquire, while claim() gates every adopt/reap path. Leases carry a state — own: or del: — so a takeover is refused against a teardown in progress. Without it an unconditional take() would overwrite a destroyer's claim and the peer's container stop would land on a sandbox the new owner had already handed to an agent. renew() distinguishes a lapsed lease from one a peer took; only the latter drops the sandbox. Collapsing them meant a Redis restart evicted every in-flight sandbox on every instance at once. Renewal runs on its own thread with a TTL derived from its interval, never from idle_timeout: renewal used to ride the idle checker, which does not start at idle_timeout: 0, so leases silently lapsed on a supported config. Ownership establishment is fail-closed: a sandbox whose ownership cannot be published is never handed out, and a just-created container is destroyed rather than leaked as an adoptable orphan. Every destroy path claims before untracking. The memory store is single-instance only and says so; the resolver reads app_config.stream_bridge and the env var in the bridge's own order, so deployments already using Redis get a redis ownership store without extra config. * fix(sandbox): wait out a recovery grace before adopting a keyless container An absent ownership lease meant two opposite things on two paths. Renewal reads it as LAPSED and re-establishes it: nobody took the lease, so the container is still ours. Reconciliation read the same absent key as "orphan" and adopted on sight. After the store loses its keys (a Redis restart without persistence, or eviction under maxmemory) every owner is alive and merely pre-renewal-tick. Whichever instance reconciled first therefore adopted every live container; each real owner's next renewal reported LOST and dropped a sandbox it was serving mid-turn, leaving it for the adopter to idle-destroy — #4206 through the back door, in the very case the LAPSED handling was added to make safe. Not limited to startup: an already-running instance hits the same window from the idle checker's periodic reconcile. _adoptable_after_grace requires an untracked container to be seen unowned across a full lease TTL before it can be adopted. That rebuilds the delay the state loss erased: a live owner republishes within one renewal interval, shorter than the TTL by construction, while a crashed owner never does, so its containers are still adopted one grace later rather than leaking. A republished lease resets the grace; a pausing-only timer would still expire over a live owner's lease. The peek is read-only — the atomic claim still gates adoption. The grace is skipped when the store cannot coordinate across processes: no peer can hold a lease such a store would show us, so single-instance deployments keep instant orphan cleanup, and a grace could not help a multi-worker gateway on memory anyway. * fix(sandbox): hold the teardown lease for as long as the container stop runs claim(..., for_destroy=True) wrote the del: marker with the ordinary lease TTL and nothing refreshed it. renew() extends only own: and deliberately reports a teardown as LOST, and the destroy paths drop the sandbox from the maps the renewal loop iterates — so a container stop that outlived the TTL let the marker lapse, a peer's take() succeeded against the still-running container, and the stop then landed on the turn that had just been handed it. That is the exact window the del: state exists to close, reopened by its own expiry. The two lease states alone never made the per-sandbox flock redundant, as I claimed when deleting it: a held lock cannot expire, a lease can. The exclusion has to be held deliberately rather than assumed to outlast the work it guards. _held_teardown_lease wraps both _backend.destroy() call sites and re-claims the marker every renewal_interval_seconds until the stop returns. No store change is needed: claim(for_destroy=True) already refreshes an existing del: marker on both backends. Reachable without an abnormal backend. The schema bounds only renewal_interval_seconds (> 0) and ttl_multiplier (>= 2), so a legal config puts the TTL below a normal container stop; and LocalContainerBackend._stop_container passes no timeout to subprocess.run, so a wedged daemon blocks unbounded even at the default 120s TTL. The TTL stays finite on purpose: the heartbeat dies with the process, so a destroyer that crashes mid-stop still releases the container one TTL later instead of marking it undestroyable forever. * fix(sandbox): hold the teardown lease on every del: stop, and pin the claims that had no test |
||
|
|
3949340610 |
fix: harden Postgres async engine with pool_recycle and command_timeout to stop stale-connection 504s (#4230)
* fix: harden Postgres async engine with pool_recycle and command_timeout to stop stale-connection 504s * fix(persistence): make postgres command timeout a configurable database setting Default the app-ORM command timeout to 30s (below nginx's 60s proxy deadline), expose it as database.command_timeout with null to disable, and add regression coverage. Addresses P1 review feedback. Signed-off-by: Matt Van Horn <455140+mvanhorn@users.noreply.github.com> * fix(persistence): decouple DB command timeout from nonexistent proxy deadline The command timeout bounds stalled ORM queries independently; drop the incorrect coupling to a 60s nginx deadline (actual proxy timeout is 600s). * feat(config): make pool_recycle configurable Expose pool_recycle alongside command_timeout and pool_size in config, example config, and the helm chart, keeping the 300s default, per review. --------- Signed-off-by: Matt Van Horn <455140+mvanhorn@users.noreply.github.com> Co-authored-by: Matt Van Horn <455140+mvanhorn@users.noreply.github.com> Co-authored-by: Willem Jiang <willem.jiang@gmail.com> |
||
|
VectorPeakGitHubchatgpt-codex-connector[bot] <199175422+chatgpt-codex-connector[bot]@users.noreply.github.com> |
16a77cb780 |
fix(serper): ignore malformed image URLs (#4319)
Co-authored-by: chatgpt-codex-connector[bot] <199175422+chatgpt-codex-connector[bot]@users.noreply.github.com> |
||
|
Willem JiangGitHubClaudeCopilot Autofix powered by AI <223894421+github-code-quality[bot]@users.noreply.github.com> |
ac5fd46281 |
feat(backend): bound LLM call concurrency and shed burst-rate (429) retries (#4294)
* Create a feature of Process-global LLM concurrency cap
* Added configuration of llm_call of max_concurrent_calls
* Classify limit_burst_rate and expose retry params via config.yaml
* refactor(middleware): encapsulate LLM concurrency state in a dataclass
Address PR #4294 review feedback (github-code-quality bot): the bare
module-level globals _GLOBAL_CONCURRENCY_LOOP / _GLOBAL_CONCURRENCY_LIMIT
were flagged as unused - a false positive, since both are read on the
recreate condition, but the `global`-declaration pattern tripped the
analyzer.
Replace the three globals + `global` declaration with a single
_ConcurrencyState dataclass singleton mutated in place. Behavior is
unchanged (lazy recreate when the running loop or configured limit
changes); the state is now co-located and no longer relies on bare
globals. dataclasses is already an established harness convention.
Co-Authored-By: Claude <noreply@anthropic.com>
* fix(middleware): make LLM concurrency limiter process-wide + jitter burst retry
Addresses PR #4294 review (fancyboi999, CHANGES_REQUESTED) - two P1 issues.
P1 #1: the asyncio.Semaphore limiter was loop-bound, so it recreated per
event loop and the cap was NOT process-wide: lead-agent calls (main loop)
and subagent calls (the isolated persistent loop in subagents/executor.py)
each got their own semaphore, and the sync graph path (wrap_model_call)
bypassed the cap entirely. Recreating on loop/limit change also abandoned
permits held by the prior instance.
Replace it with a _ProcessWideLimiter built on threading primitives (not
loop-bound): one limiter shared across every event loop and both sync/async
wrappers. The cap is mutable via set_limit (never recreates, so in-flight
permits are never abandoned); permits release in finally and async waiters
unregister on cancellation, so cancellation never leaks capacity. Wire it
into wrap_model_call (sync) too - previously a direct handler() call.
P1 #2: the first (and only) burst-rate retry was deterministic at 5000ms.
prev_delay_ms was seeded from the 1000ms normal base, so for burst the
window collapsed to randint(5000, max(5000, 1000*3)) = randint(5000, 5000) -
a fleet that failed together realigned on the same 5s tick. Seed the first
retry from the reason-specific base (prev_delay_ms=None on loop init) so
the burst window is [burst_base, cap] = [5000, 8000], non-degenerate.
Retry-After is still honored verbatim.
Tests: rename semaphore tests -> limiter; add an autouse fixture resetting
the process singleton; add regressions the reviewer asked for - cross-loop
(lead + isolated-loop subagent), two concurrent sync calls, limit-change
while a permit is held (same instance, permit preserved), cancellation
no-leak, and burst first-retry non-degeneracy with default config (real
and seeded RNG) plus a concurrent de-synchronization case. Verified the
burst guard goes red on the old logic ({5000}) and green on the new.
Co-Authored-By: Claude <noreply@anthropic.com>
* Apply suggestions from code review
Co-authored-by: Copilot Autofix powered by AI <223894421+github-code-quality[bot]@users.noreply.github.com>
* Potential fix for pull request finding 'Statement has no effect'
Co-authored-by: Copilot Autofix powered by AI <223894421+github-code-quality[bot]@users.noreply.github.com>
* fix(middleware): lossless limiter handoff, generation-aware cap, burst-rate CB gate
Address the open P1/P2 review findings on #4294:
- P1 #1 (cancellation handoff): reserve the permit for a specific waiter at
dequeue time (grant-at-dequeue, _AsyncWaiter.granted) so a waiter cancelled
in the post-dequeue / pre-reacquire window hands its reservation to the next
waiter (_handoff_granted_permit_locked) instead of stranding it. No
cancellation window remains.
- P1 #2 (hot-reload generation): move cap updates out of the per-attempt path;
give the limiter one generation-aware owner (set_limit_if_newer with a
monotonic instance seq proxy for config freshness) so a stale in-flight run
cannot rewrite a freshly-lowered cap. max_concurrent_calls is now genuinely
hot-reloadable, resolving the reload-boundary inconsistency by option (b) -
no STARTUP_ONLY_FIELDS change (retry params truly hot-reload).
- P2 (circuit breaker): gate _record_failure on reason != "burst_rate" so
burst-rate (limit_burst_rate) exhaustion - a transient slope-throttle, not
"provider down" - does not trip the CB and fast-fail ALL calls.
- P3: clamp the jitter window to the cap before drawing (uniform spread
instead of piling at the cap); document the per-process / GATEWAY_WORKERS
cap semantics in config + the field description.
Tests: add the reviewer-requested regressions (cancel-after-dequeue handoff;
stale-instance-doesn't-overwrite-lowered-cap across sync + isolated-loop async;
burst_rate-exhaustion-doesn't-trip-CB sync + async). Each is red on the prior
buggy logic and green on the fix. _build_middleware now routes llm_call knobs
through AppConfig so __init__ applies the cap. 71 middleware tests pass; 212
across the blast radius (1 pre-existing skip).
Co-Authored-By: Claude <noreply@anthropic.com>
* fix(middleware): startup-only LLM concurrency cap; report effective retry budget
Addresses review feedback on #4294 (fancyboi999 CHANGES_REQUESTED on
|
||
|
AariandGitHub
|
e66f455d51 |
fix(skills): don't treat a lazily evaluated PEP 695 type alias as a network sink (#4315)
* fix(skills): don't treat a lazily evaluated PEP 695 type alias as a network sink * test(skills): cover type alias parameter bounds |
||
|
Ryker_FengandGitHub
|
24a45a4e68 | feat(tui): add clear command (#4306) | ||
|
Daoyuan LiandGitHub
|
6544d96cc4 |
fix(skills): close AST literal-only shell=True bypass in SkillScan (#4057)
* fix(skills): close AST literal-only shell=True bypass in SkillScan SkillScan's Python analyzer only classified a subprocess call as the CRITICAL, hard-blocked python-shell-exec rule when its shell= keyword was the literal AST constant True. Any non-literal value with the same runtime effect - a variable (shell=shell_flag) or an expression (shell=bool(1)) - fell through to the HIGH, non-blocking python-subprocess classification instead, silently bypassing enforce_static_scan's deterministic CRITICAL gate despite behaving identically to shell=True at runtime. _call_has_shell_true is renamed to _call_shell_may_be_true and now fails closed on ambiguity: any shell= value that is not a literal, statically-provable False is treated as CRITICAL, matching the literal shell=True case. A call with no shell= keyword at all is unaffected (subprocess already defaults to shell=False). Adds regression tests for the variable and expression bypass shapes, plus a boundary test locking in that literal shell=False remains a non-blocking warning. * fix(skills): fail closed on **-unpacked shell= in SkillScan _call_shell_may_be_true only checked keyword.arg == "shell", so a subprocess.* call that supplies shell via **-unpacking (a keyword node with arg is None) fell through to the non-blocking python-subprocess classification instead of the CRITICAL python-shell-exec path. Treat any **-unpacked keyword as shell-ambiguous and fail closed, same as the existing shell=variable/shell=expression handling. This intentionally over-blocks a **-unpack that carries no shell key, since a mapping's contents are not knowable by static analysis; that tradeoff is documented inline and covered by a dedicated test. |
||
|
AariandGitHub
|
09e25b8a32 |
fix(auth): let deployments close local self-registration (#4311)
* fix(auth): let deployments close local self-registration The OIDC provisioning policy (allowed_email_domains, require_verified_email, auto_create_users) is enforced only in the SSO callback via get_or_provision_oidc_user. POST /api/v1/auth/register creates a local account without consulting any of it, and nothing can turn that path off, so a deployment declaring an email-domain allowlist can still be joined by any address through local registration. Add auth.local.allow_registration (default true, so existing deployments are unchanged) and gate /register on it before the account is created. Report the flag from /setup-status so the login page stops offering a signup entry the Gateway will reject. /initialize is deliberately not gated: it is the bootstrap path, guarded by admin_count == 0, and closing it would leave a fresh install unable to create its first admin. An unreadable config.yaml falls back to the pre-gate default (open) rather than making these two endpoints a hard dependency on the file. * docs(auth): align registration-gate fallback wording with the FileNotFoundError catch |
||
|
AariandGitHub
|
1a1c5def0d |
fix(agents): classify web_fetch error pages as errors, not successful evidence (#4314)
* fix(agents): classify web_fetch error pages as errors, not successful evidence
* fix(agents): classify 502/503/504 error shells as transient, not internal
Per review: a gateway error page is the try-a-different-source case, so
502/503/504 reason phrases now map to transient (warn, then escalate)
while 500/501 stay internal (stop). This mirrors _ERROR_RULES' own split
and removes the two phrases that classified differently through the
shell table than through _classify_error_text ("service temporarily
unavailable", "gateway timeout") — now pinned by a cross-path
consistency test over the live table plus a chain-level test that a 503
page warns and escalates instead of blocking web_fetch on first sight.
Also per review, _classify_error_shell returns a copy of the category
attrs instead of the rules-table dict, matching _classify_error_text.
Dropped "gone" from the phrase table: a single-word reason phrase
collides with legitimate one-word document titles ("# Gone"), and 410
error pages are rare while such titles are not; negative controls record
the decision.
* test(agents): pin crawl4ai's recorded renderer shape; note web_capture asymmetry
Per review: the title rule assumes every web_fetch provider renders
title-first. Eight of the nine providers hold by construction
(f"# {title}" or the shared Article.to_markdown()); crawl4ai renders
server-side, so its generator output (crawl4ai 0.9.2, fit and raw) was
measured over the same corpus and recorded as fixtures driven through
the real tool and middleware chain. A comment on _PAGE_CONTENT_TOOL_NAMES
records that web_capture's absence is intentional: its dead-target signal
is the provider warning suffix, which belongs to the provider boundary.
|
||
|
AariandGitHub
|
b02308bb1d | fix(dingtalk): drop inbound messages that carry no conversation identity (#4316) | ||
|
|
cd34a1a504 |
fix(skills): don't attach model tracing to the in-graph skill security scan (#4252)
* fix(skills): don't attach model tracing to the in-graph skill security scan * fix(skills): pass attach_tracing explicitly from the in-graph scan call site Follow the tracing INVARIANT's own convention rather than detecting the call context: scan_skill_content takes an attach_tracing flag, and _scan_or_raise -- the single in-graph choke point -- passes False. Standalone callers (Gateway skill routes, installer) keep the default True. The INVARIANT list named four sites and asks that new in-graph calls be added to it; record this fifth one so a future audit of that list finds it. --------- Co-authored-by: Willem Jiang <willem.jiang@gmail.com> |
||
|
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. |
||
|
|
a9a57fb711 |
fix(gateway): handle null config.configurable in _resolve_thread_id (#4301)
* fix(gateway): handle null config.configurable in _resolve_thread_id
`_resolve_thread_id` read the thread id with
`(body.config or {}).get("configurable", {}).get("thread_id")`. The
`.get("configurable", {})` fallback only applies when the key is absent.
`RunCreateRequest.config` is typed `dict[str, Any] | None`, so a client
can send `{"config": {"configurable": null}}`; the key is then present
with value `None`, and `None.get("thread_id")` raises `AttributeError` —
an unhandled HTTP 500 on `POST /api/runs/stream` and `/api/runs/wait`.
Coalesce the inner value with `or {}` so a null `configurable` yields a
freshly generated thread id, matching the docstring ("or generate a new
one") and the behaviour of `config: null` / `configurable: {}`. The
sibling `_checkpoint_configurable` in thread_runs.py already guards the
same field defensively. Behaviour is byte-for-byte identical for every
input that worked before.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* style: ruff format the new test (lint-backend runs ruff format --check)
Collapse the multi-line assert ruff format flags; behaviour unchanged.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* Also coerce null configurable in build_run_config, not just _resolve_thread_id
---------
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
||
|
|
de024646a7 |
feat(memory): memory template externalization (#4286)
* feat(memory): template externalization — externalized prompts + signal patterns Externalize two categories of hardcoded content in the DeerMem memory backend so operators can customise them without touching Python code. Prompt externalization (plugin 06): - 4 memory extraction prompts moved from Python string constants to YAML files under core/prompts/ (consolidation / fact_extraction / memory_update.chat / staleness_review). - load_prompt(name) and load_prompt_messages(name, variables) loaders. - memory_update uses the chat format (system / user message split) for prompt-caching-friendly system prefixes; other prompts stay text. - The extraction callback + per-agent prompt directories + Jinja2 dependency are intentionally not included (minimal surface). - Compatible with the upstream prompt additions from #4143 (expected_valid_days, staleFactsToExtend, KEEP/REMOVE/EXTEND in staleness review). Signal-pattern externalization (plugin 07, regex only): - Correction and reinforcement detection patterns externalised to core/message_patterns/{correction,reinforcement}.yaml. - load_patterns(name, patterns_dir) loader with bundled defaults. - detect_correction / detect_reinforcement accept a keyword-only patterns= parameter; signatures are backward-compatible. - patterns_dir config field added to DeerMemConfig. - Importance scorer (importance.py, build_importance_scorer, _prepare_update changes) is NOT included — deferred to a later PR. Co-Authored-By: Claude <noreply@anthropic.com> * fix(memory): fail loudly on invalid yaml in prompt/pattern loaders Replace silent error handling in load_prompt / load_prompt_messages / load_patterns so malformed yaml or missing required keys raise ValueError with the file path rather than a raw YAMLError traceback or silent empty-string / empty-list return. Changes: - load_prompt: YAMLError -> ValueError(path); missing/empty 'template' key -> ValueError - load_prompt_messages: YAMLError -> ValueError(path); missing/empty 'messages' key -> ValueError; KeyError from .format (placeholder mismatch) -> ValueError - load_patterns: YAMLError -> ValueError (was silent warn+return []). OSError still degrades (permission, not format). Not-a-list yaml -> ValueError (was silent warn+return []). Co-Authored-By: Claude <noreply@anthropic.com> * fix(memory): wire prompts_dir through updater, fail-loud patterns_dir, bump config_version Addresses PR feedback on the template-externalization PR: P1 — Wire prompts_dir into the DeerMem update path: - Add prompts_dir field to DeerMemConfig (default None = bundled defaults). - Thread it through DeerMem -> MemoryUpdater.__init__(prompts_dir=). - _build_staleness_section / _build_consolidation_section accept prompts_dir= keyword and call load_prompt() instead of relying on module-level shim constants. - _prepare_update_prompt passes prompts_dir to load_prompt_messages and to both section builders. - Add _PROMPT_CACHE to load_prompt so repeated lookups (once per memory-update cycle) do not re-read yaml from disk. P2 — Explicit patterns_dir must find its files: - When patterns_dir is explicitly set and a YAML file is missing, load_patterns() raises FileNotFoundError instead of silently caching an empty list. Bundled defaults (patterns_dir=None) still log a WARNING and return [] for a missing bundled file (packaging bug). P3 — Bump config_version and sync Helm: - config.example.yaml: 26 -> 27 - deploy/helm/deer-flow/values.yaml: 26 -> 27 - deploy/helm/deer-flow/README.md: 26 -> 27 - scripts/check_config_version.sh confirms parity. Tests: 224 passed (218 memory + 6 config_version). Lint: ruff check + ruff format --check pass. Co-Authored-By: Claude <noreply@anthropic.com> * fix(memory): cache load_prompt_messages, validate format fields, fail-loud load_patterns Review feedback from willem-bd: Cache for load_prompt_messages: Add _CHAT_TEMPLATE_CACHE + _render_messages() helper so the parsed chat templates are cached per (name, agent, prompts_dir). On cache hit only .format() rendering runs; the yaml file is read once per key. Validate format field in both loaders: load_prompt rejects format='chat' (redirects to load_prompt_messages); load_prompt_messages rejects format='text' (redirects to load_prompt). Prevents operators from loading a chat yaml via the text loader (or vice versa) without a clear error. Load_patterns fail-loud for explicit directories: - OSError (permission, etc.) now raises for explicit patterns_dir instead of silently disabling detection. - Malformed entries (missing/empty pattern key, wrong type) are skipped with a WARNING instead of silent skip. - Unknown flag names are warned instead of silently dropped. - re.error from compile() raises ValueError with file path and entry index instead of a bare re.error traceback. Co-Authored-By: Claude <noreply@anthropic.com> * fix(memory): thread agent_name through section builders, validate explicit prompts at construction, bump config_version to 28 P1 — Thread agent_name through staleness/consolidation section builders: _build_staleness_section and _build_consolidation_section now accept agent_name= and pass it to load_prompt(), so per-agent prompt overrides work for section templates too (not just memory_update). Previously only prompts_dir was threaded; agent_name was ignored for section builders. P1 — Validate explicit prompts at DeerMem construction: When DeerMemConfig.prompts_dir is explicitly set, DeerMem.__init__ now pre-loads all four prompt templates (staleness_review, consolidation, fact_extraction, memory_update with dummy variables) at construction time. A missing file, malformed YAML, or invalid placeholder raises immediately at startup instead of being caught by the updater's generic error handler and silently dropped as a failed update. P2 — Advance config_version to 28: Upstream main already consumed the previous 26→27 bump with a different schema change. Bump config.example.yaml, Helm values.yaml, and Helm README.md to 28 to version this PR's patterns_dir and prompts_dir additions. Co-Authored-By: Claude <noreply@anthropic.com> * fix(memory): render text templates at construction, propagate PromptConfigurationError Keep prompt-configuration failures out of the recoverable update catch: - Define PromptConfigurationError(ValueError) in prompt.py. Raised by load_prompt, load_prompt_messages, and _render_messages for bad yaml, missing keys, and invalid placeholders. - _do_update_memory_sync_impl, update_memory's executor path, and _process_queue all re-raise (PromptConfigurationError, FileNotFoundError, OSError) before the generic except Exception, so a bad explicit prompt is never silently returned as False. - DeerMem.__init__ prompt validation now renders text templates with dummy variables (.format(stale_facts="") etc.) so an unknown placeholder in staleness_review.yaml, consolidation.yaml, or fact_extraction.yaml is caught at construction. Co-Authored-By: Claude <noreply@anthropic.com> * fix(memory): drop re-raise guards, soften validation comment, exclude dormant fact_extraction - Remove (PromptConfigurationError, FileNotFoundError, OSError) re-raise guards from _do_update_memory_sync_impl, update_memory executor path, and _process_queue. The existing except Exception: logger.exception(...) already logs prompt-config errors at ERROR with full traceback; the re-raise was killing the entire batch and only surfacing via stderr. Per-agent override errors now log at ERROR per-item without aborting co-tenant updates. - Soften the construction validation comment to clarify it only covers global templates. Per-agent overrides are validated lazily at first use and logged at ERROR by the updater's exception handler. - Drop fact_extraction from construction validation (dormant prompt with no runtime caller; the shim + yaml remain for backward compat). Co-Authored-By: Claude <noreply@anthropic.com> --------- Co-authored-by: Claude <noreply@anthropic.com> |
||
|
AariandGitHub
|
a8bf54cbbb |
feat(skillscan): detect exfil through instance/dataflow network clients (#4265)
* feat(skillscan): detect exfil through instance/dataflow network clients * fix(skillscan): resolve client handles with Python lexical scopes _walk_client_nested_scope copied every live handle into a nested scope after excluding only parameters. That is not Python's name resolution: a class namespace is not a closure scope for its methods, a function-local binding shadows an enclosing name across the whole body, and comprehensions bind their targets in a scope of their own. Benign skills were therefore blocked as python-env-dump-exfil (CRITICAL) even where the outbound-looking call provably cannot run on the tracked client. Derive each scope's bindings lexically instead: - a class body reads its enclosing scope, but the names it binds are not passed into the methods defined in it; - comprehensions are their own scope, so only the outermost iterable is evaluated outside and every for-target shadows first; - a function-local prepass excludes names the body binds anywhere, with global/nonlocal opting back out. Detection is unchanged where the client really is reachable: a comprehension calling an unshadowed handle, a method closing over an enclosing function's handle, and a global-declared module handle all still block. * fix(skillscan): apply target rebinding in Python evaluation order The generic ast.iter_child_nodes() fallback yields fields in declaration order, which for `for`/`async for`, `:=` and `+=` puts the binding target ahead of the expression Python evaluates to produce it. Visiting the target first dropped the handle before the call that actually runs on it, so `for s in s.post(host, ...)` reported nothing even though Python calls post() on the client before binding the loop target. Match captures bind through a plain `name` string rather than a Name node, so the Store branch never saw them and a rebound name kept a stale handle. Assignment expressions inside a comprehension were applied only to the comprehension-local map, leaving the containing scope stale as well. Give every bind-after-evaluate construct its own branch: - `for`/`async for` walk the iterable against the pre-loop binding, then rebind the target, then walk the body; - `:=` walks its value first and binds into the containing scope too, since PEP 572 puts a comprehension's walrus target there; - `+=` walks its value before dropping the target; - match captures drop the handle, matching how a rebind under `if`/`try` that may equally not execute is already treated. The bypasses this closes are the reason detection widens here; the comprehension walrus and match cases narrow it back where the client provably cannot be the receiver. * fix(skillscan): preserve scoped client handle semantics * fix(skillscan): scan sinks inside assignment target expressions The Assign/AnnAssign, AugAssign, For/AsyncFor, with, and comprehension branches evaluated only the value and rebound the target, so a client call placed in an attribute receiver or a subscript value/index -- both evaluated at bind time -- was never scanned. os.environ could exfil through a tracked client while python-env-dump-exfil reported nothing. Add _walk_client_target_exprs to walk the executable parts of a binding target (attribute receiver, subscript value+slice, recursing through tuple/list/starred) in Python evaluation order, without treating Store name leaves as reads. The AnnAssign annotation expression is walked too. Name-leaf invalidation is unchanged. * fix(skillscan): apply assignment targets and annotations in runtime order The round of assignment-target scanning walked every target then rebound the names in one batch, and always walked an AnnAssign annotation before the target. Python instead binds chained and destructured targets left to right (so `session = out[session.post(...)] = cfg` runs the subscript on the already-rebound name), evaluates a variable annotation only in module or class scope (never in a function, never under `from __future__ import annotations`), and evaluates an executed annotation after the target. The scanner therefore hard-blocked benign skills. Bind each target left to right via _bind_client_targets (walk its executable sub-expressions against the current bindings, then rebind before the next target), and walk an annotation only for the nodes _evaluated_annotation_nodes marks as actually evaluated, after the target. Target-expression scanning and name-leaf invalidation are otherwise unchanged. * fix(skillscan): scan client sinks in evaluated function-signature annotations A function's parameter and return annotations are evaluated at def time in the enclosing scope, like its decorators and defaults, so a tracked client sink placed in one is a real egress. _client_scope_prelude walked the decorators and defaults but not the annotations, so os.environ could exfil through `def f() -> session.post(host, json=dict(os.environ))` while python-env-dump-exfil reported nothing. Record function/async-function defs in _evaluated_annotation_nodes (their signatures evaluate at def time unless from __future__ import annotations postpones them) and, for those nodes, add the parameter and return annotation expressions to the enclosing-scope prelude walk. * fix(skillscan): match runtime evaluation order for annotations and except handlers * fix(skillscan): scope try/match branches to their own selection state * fix(skillscan): propagate branch bindings to everything that observes them A branch's net effect has to be visible exactly where Python makes it visible. The walker isolated `except`/`else`/`match` bodies into scope copies and then discarded them, so a client created on the branch was invisible to `finally`, to the code after the statement, and to anything defined inside the branch, while a name the branch replaced stayed a sink receiver. - `except*` clauses are sequential, not alternatives: thread one scope through body, clause types and clause bodies in source order instead of reusing the mutually exclusive copies ordinary `except` needs. - Fold each `except`/`else`/`match` branch's net effect back into the scope that `finally` and the following code read, and make the branch scope what nested definitions close over. - Keep a fallthrough scope across `match` guards, so a guard that returned false still hands its side effects to the next case, while pattern captures stay isolated to their own case. - Keep an `as` target path-local: Python unbinds it only on the path that ran, so dropping it for every path erased a live handle where no such handler executed. Adds a 14-case runtime-oracle regression covering both directions at every branch site; each of the ten guards was deleted on its own to confirm the test that pins it goes red. * fix(skillscan): join alternative branches instead of overwriting one with another Only one of a statement's alternative branches runs, but the walker folded each one into a single destructive binding map. Whichever branch was visited last therefore decided the state: a handler that rebinds the name erased a sibling that leaves the client in place (missing a client Python really calls), and a handler that builds one was credited on paths where it never ran (inventing a CRITICAL sink). Alias targets had the mirror problem, since only key presence was compared, so replacing `import x as name` on a live branch was ignored. - Join alternatives into a may-state: a name stays a sink receiver when any feasible branch leaves it a client, and stops being one only when every feasible branch replaced it. Alias targets join toward the target that can still name a constructor. - Treat each `except*` clause as optional rather than threading every clause body unconditionally, so a clause whose type never matched cannot erase a handle the next clause calls. - Keep the fall-through state (no exception raised, no case matched) as one more alternative, and drop it only where the source decides the outcome: a literal always-raising body selecting one handler, a literal exception group choosing `except*` clauses, a wildcard or literal-equal `match` case. Also corrects an existing match-capture test that asserted the non-exhaustive case is benign: the runtime oracle shows the original client still takes the call on the path where nothing matched. Adds runtime-oracle regressions for both directions at every alternative site; each of the nine guards was deleted on its own to confirm the test pinning it goes red. * fix(skillscan): model feasible conditional client flow * fix(skillscan): preserve feasible control-flow outcomes * fix(skillscan): model expression evaluation paths * fix(skillscan): narrow instance-client detection to lexical statement order The construction-to-use signal had grown into a path-aware interpreter: exception selection, except* subgroup consumption, match capture timing, finally override, comprehension laziness, annotation evaluation order, and may-state joins over feasible branches. That is the heavyweight analysis RFC #2634 rules out of Phase 5, and because the signal feeds a CRITICAL rule it hard-blocks skill installation, so every ambiguity it resolved by over-reporting cost a benign skill instead of a human review. Replace it with ordinary statement order over a one-level handle map: a known constructor bound to a simple name (including `with ... as`), a direct outbound method call on that name in the same lexical scope, rebinding invalidation, and name-to-name alias propagation so `s = session` does not shed the handle. A sink is recorded at the call, so a rebind after the call cannot retract it. Compound statements are not interpreted. Every name an if/try/except*/loop/ match may bind is dropped before its bodies are walked, and each body is walked from an isolated copy. Dropping before rather than after is what keeps a `finally` that runs after a handler rebound the name, a later except* clause, and a second loop iteration from reporting a client the runtime never calls. Bodies are still walked, or wrapping any construction in `if True:` would be a universal bypass. Lexical scoping is unchanged: class namespaces are not closures for their methods, comprehension targets and function-local bindings shadow, and alias visibility stays per scope. The cases this gives up are false negatives by construction and are recorded in #4296, pinned by a test that asserts the runtime really calls the client while the scanner stays silent. Verified: 102 SkillScan tests; full suite 8 failed / 7851 passed, the same network-dependent web-fetch tests that fail on clean main; per-clause red check 12/12 guards red; 925 repo-owned files scanned branch vs main with 0 new and 0 lost CRITICAL findings; #4158 bypass BLOCKED and #4153 false positive allowed with 0 findings through the real enforce_static_scan gate. * test(skillscan): pin closure boundary * refactor(skillscan): narrow client handle analysis * fix(skillscan): close client handle correctness gaps * fix(skillscan): require proven client imports |
||
|
Daoyuan 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. |
||
|
|
5b65d543b1 |
fix(channels): key inbound dedupe on chat-scoped workspaces (#4287)
* fix(channels): key inbound dedupe on chat-scoped workspaces * fix(channels): release the inbound dedupe key on swallowed transient failures _release_inbound_dedupe_key runs from _handle_message's generic exception handler, but three sites handle their error in place and never re-raise, so the key recorded on receipt survives the full dedupe TTL and the provider's redelivery -- the retry that would have recovered the failure -- is dropped: - _handle_streaming_chat swallows every stream error into its finally block - the fire-and-forget runs.create busy branch - the non-streaming runs.wait busy branch Keying chat-scoped workspaces gives telegram/feishu/wechat/dingtalk a dedupe key for the first time, which newly exposes them to this. The mechanism is pre-existing, not introduced here: WeCom already had a key (streaming plus aibotid) and shows the same black hole on main. Also parametrize the dispatch dedupe test over all three chat-scoped providers so the streaming path is covered end-to-end, and restate the workspace-fallback comment in terms of what the chain actually guarantees -- both fallbacks are appended last and gated on every earlier source being absent, so they can only turn "no key" into a key, never change one. * fix(channels): publish final reply before dedupe release --------- Co-authored-by: Willem Jiang <willem.jiang@gmail.com> |
||
|
Hanchen QiuandGitHub
|
d2f8f61e3a |
fix(skills): add security_fail_closed option for moderation model outages (#4297)
* fix(skills): add security_fail_closed option for moderation model outages When the skill security moderation model call fails, scan_skill_content previously blocked ALL content (executable and non-executable), which turns a moderation-model outage into a denial of service for skill writes. Add a skill_evolution.security_fail_closed option (default True, preserving current behavior). When set to False, non-executable content is allowed with a warn decision during an outage while executable content is still blocked. Closes #3021 * fix(config): bump config_version to 27 and format skill_evolution config Address review feedback on #4297: - Bump config_version 26 -> 27 so existing installs are flagged outdated and pick up skill_evolution.security_fail_closed via make config-upgrade. - Apply ruff format to skill_evolution_config.py to satisfy the backend formatting gate. - Add config-version/upgrade regression tests covering the v26 outdated warning and merging security_fail_closed without changing user values. * fix(helm): bump chart config_version to 27 to match config.example.yaml Keeps deploy/helm/deer-flow/values.yaml and its README example in sync with the config schema bump, satisfying scripts/check_config_version.sh (validate-chart CI). * fix(skills): surface fail-open security scan in logs Address @willem-bd review feedback on #4297: - Log an operator-visible warning when the moderation model is unavailable and fail-open lets non-executable skill content through as a warn, so a skipped scan is no longer silent. - Reword the model-call-failed log so it stays accurate under both fail-closed and fail-open policy instead of always claiming a "conservative fallback". - Add a regression test asserting the fail-open warn path emits the warning log. |
||
|
luo jiyinandGitHub
|
271a921baf |
refactor(sandbox): reuse E2B kill helper during eviction (#4298)
* refactor(sandbox): reuse E2B kill helper during eviction * test(sandbox): preserve close on kill lookup failure |
||
|
|
867235389d |
Add trace-based behavioral tests with Monocle Test Tools (#4025)
* Add Monocle behavioral test suite Trace-based tests under tests/monocle/ asserting against the agent's Monocle execution traces: 4 offline tests load a recorded trace by file (with_trace_source), one per curated question, plus 1 live end-to-end test. Fluent structural asserts (agent, tools, input/output, token/duration budget); additive only, no app-code changes. * Address review: restructure suite, move under backend/tests/monocle/ Responds to the review on this PR. Behavioural coverage is now the live tests, not frozen fixtures. Keep one offline test as a worked example of the fluent assertion API (loads a recorded trace by file, no keys), and add two live tests that drive the agent end-to-end through DeerFlowClient and assert on the trace the real run emits (web-research and sandbox paths). The live tests skip without OPENAI_API_KEY or the app. Other review fixes: - Move the suite under backend/tests/monocle/ so backend pytest collects it. - Split helpers into _helpers.py; conftest is fixtures-only (run_agent), with Monocle setup owned by the validator and .env load scoped to the live path. - Resolve the model from config.yaml instead of hardcoding gpt-4o; skip the live tests when config.yaml is absent. - Keep one trace with a stable name (web_research_ev_battery.json); drop the other three (removes ~2,200 lines of fixture blobs). - Drop the flaky wall-clock duration bound on live runs. - Keep monocle_test_tools in a standalone requirements.txt rather than the backend dev group: it hard-depends on the ML eval stack (torch, transformers, sentence-transformers, ~48 packages, +950 lines in uv.lock), so isolating it keeps the app's locked deps clean. importorskip skips the suite when absent. * docs(monocle tests): explain the golden-trace workflow Add a "How this is meant to be used" section: capture a run you are happy with as a golden, labelled trace, turn it into assertions (the offline example), then point the same assertions at the live agent so every later run has to reproduce that behaviour. * Address review: fix docstring pytest paths, state the suite is not run in CI The docstring commands now use the backend/tests/monocle/ form (matching the README, which also gains the backend-dir uv variant), and the README states explicitly that the suite is skipped in CI and run on demand. * Address review: document the committed trace, pin assertions context - README: new section on the committed trace. It is a full, unmodified real-run recording (system prompt of the recording date + fetched web content, no credentials), committed whole so the offline example parses a genuine trace. The offline assertions are pinned to this trace and the monocle_apptrace 0.8.8 span shapes; re-record when prompt, tools, or model change. - README: note that the monocle_trace_asserter fixture comes from monocle_test_tools' auto-registered pytest plugin (pytest11 entry point). - test_deerflow.py: comment why web_fetch asserts min_count=2 rather than the recorded exact count of 5 (fetch counts vary run to run; keep it a floor). - requirements.txt: loose pin python-dotenv>=1.0. * Address review: make live tests explicit opt-in, drop OpenAI-only gate Two execution-gating fixes from review: - Live tests are now opt-in via MONOCLE_LIVE_TESTS=1 (default off). Previously the documented offline command collected the live tests too, and on a configured checkout (.env + config.yaml present) they would run for real, spending model tokens and hitting the network. Now the plain `pytest backend/tests/monocle/` run cannot go live regardless of what credentials are present; test_live_gate_defaults_off pins the gate. - The run_agent fixture no longer requires OPENAI_API_KEY. config.yaml resolves the model, which may be any provider (Anthropic, Gemini, Volcengine, ...), so a hard-coded OpenAI gate skipped valid configurations and passed invalid ones. Credentials are validated by the configured model itself. README and docstrings updated to match: offline command is offline by construction, live is MONOCLE_LIVE_TESTS=1, credentials described as the configured model's rather than OpenAI's. Verified both modes: default run is 2 passed 2 skipped with no network; opted in, all 4 pass with real end-to-end runs. --------- Co-authored-by: Willem Jiang <willem.jiang@gmail.com> |
||
|
AariandGitHub
|
bc9c027a54 |
fix(uploads): claim the converted markdown filename before writing it (#4288)
* fix(uploads): claim the converted markdown filename before writing it * doc(changelog): record the converted-markdown filename collision fix Covers both surfaces that report markdown_file: the gateway uploads route and DeerFlowClient.upload_files. * fix(uploads): release the claimed markdown name when conversion fails Claiming the companion .md name before conversion means a conversion that writes nothing leaves the name reserved for the rest of the request, so a later same-stem upload is renamed against a name nothing occupies. Uploading notes.docx (conversion fails) + notes.md stored the user's file as notes_1.md with no notes.md on disk, where main kept notes.md. Discard the claim when conversion returns None, at both call sites that pre-claim it. Covers both victims: a later same-stem .md upload, and the next convertible's companion. |
||
|
|
4746a2579b |
fix(loop-detection): clear the per-tool frequency counter on evict/reset (#4295)
`_evict_if_needed` and `reset` dropped `_tool_name_history` (the windowed
deque) but left `_tool_name_counter` (the Counter that mirrors it) in place.
After a thread id was LRU-evicted and later reused, its frequency count
resumed from the stale value instead of zero, so the first fresh tool call
was force-stopped ("Tool X called N times") as if the evicted calls had
never rotated out. `reset()` had the same gap.
Drop the counter alongside the deque at all three sites (evict, per-thread
reset, full reset). The window deque and its mirror Counter now stay in sync.
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
||
|
|
dd2f73c1a1 |
fix(config): normalize the postgres:// short scheme for the async ORM engine (#4293)
`app_sqlalchemy_url` rewrites `postgresql://` to `postgresql+asyncpg://` but
leaves libpq's `postgres://` short scheme untouched, so it reaches
`create_async_engine` verbatim and raises
`NoSuchModuleError: Can't load plugin: sqlalchemy.dialects:postgres`.
The two consumers of the same `database.postgres_url` disagree about that
scheme, which is what makes this a partial, backend-only failure rather than a
clean config error:
- the checkpointer and store pass the raw URL to psycopg
(`runtime/checkpointer/provider.py`, `runtime/store/provider.py`), and
psycopg's `conninfo_to_dict` accepts `postgres://` and `postgresql://`
identically;
- the application ORM engine goes through `app_sqlalchemy_url`
(`persistence/engine.py:181`), and SQLAlchemy dropped the `postgres`
dialect alias in 2.0.
So a `postgres://` DSN brings the checkpointer up and takes the ORM engine
down. The `postgres_url` field docstring already promises the opposite --
"the +asyncpg driver suffix is added automatically where needed".
`postgres://` is a legal libpq URI scheme, not a typo, and is the form
`$DATABASE_URL` commonly takes on managed Postgres providers -- which is
exactly what the module docstring recommends configuring
(`postgres_url: $DATABASE_URL`).
Normalize it alongside `postgresql://`. The existing tests covered
`postgresql://` and `postgresql+asyncpg://` but not the short scheme; the new
case fails on main with the `NoSuchModuleError` above.
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
||
|
luo jiyinandGitHub
|
90d511f3d2 |
refactor(sandbox): consolidate E2B client lifecycle helpers (#4262)
* refactor(sandbox): consolidate E2B client lifecycle * test(sandbox): cover E2B lifecycle cleanup |
||
|
yym36991andGitHub
|
0f088033fe |
fix(gateway): prefer X-Trace-Id over metadata.deerflow_trace_id when header is set (#4283)
When TraceMiddleware binds a valid inbound X-Trace-Id, worker resolution prefers that id over config.metadata.deerflow_trace_id so logs, response headers, Langfuse, and runtime context stay aligned. Also add trace-context marker reset coverage for the inbound-header flag. |
||
|
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. |
||
|
Ryker_FengandGitHub
|
a028dfd5fb |
feat(auth): add keep me signed in login option (#4255)
* feat(auth): add keep me signed in login option * fix(auth): address remember-login review feedback |
||
|
Ryker_FengandGitHub
|
e89edb39b1 | fix(gateway): reject non-positive read limits (#4284) | ||
|
Ryker_FengandGitHub
|
f8bef42a04 | fix(wechat): validate timing configuration (#4280) | ||
|
Zhengcy05andGitHub
|
5994fdf38c |
fix: avoid forcing frontend nginx upgrades (#4250)
* fix: avoid forcing frontend nginx upgrades * fix: scope nginx upgrade assertion to frontend location |
||
|
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. |
||
|
AariandGitHub
|
a0e1d82ef4 |
fix(workspace-changes): offload blocking filesystem IO in text-cache lifecycle (#4268)
* fix(workspace-changes): offload blocking filesystem IO in text-cache lifecycle capture_workspace_snapshot and record_workspace_changes offload their scans via asyncio.to_thread, but ran the snapshot text cache's whole lifecycle on the event loop: roots resolution (os.path.abspath), tempfile.mkdtemp, and shutil.rmtree on both the capture-failure branch and record_workspace_changes' finally. That finally runs on every agent run, including abort paths, so each run removed up to max_files cached texts on the loop. Offload the roots + mkdtemp prep through one _prepare_capture worker hop, and route both rmtree call sites through _remove_text_cache_dir. Cleanup stays best-effort: it swallows and logs, so a failing cleanup cannot replace the exception or result already in flight. asyncio.shield is deliberately not used -- to_thread submits to the pool immediately, so cancelling the future does not stop the running thread and the cache is still removed under single and repeated cancellation. Externally observable behavior is unchanged. Found via `make detect-blocking-io`: these were the last 2 HIGH findings in the repo, which now reports none. The roots resolution is invisible to that scanner (sync helper, cross-file call) but blocks the same async path, and the anchor cannot reach the rmtree without it. Add tests/blocking_io/test_workspace_changes_recorder.py, driving the capture-failure branch and the record finally. Teeth verified per clause under the strict Blockbuster gate: reverting each offload alone reddens its own call (os.path.samestat for rmtree, os.path.abspath for roots/mkdtemp). * fix(workspace-changes): make text-cache prepare handoff cancellation-safe _prepare_capture creates the mkdtemp cache dir inside the to_thread worker, so a run cancelled after mkdtemp but before the coroutine receives the path orphaned the dir. Shield the prepare future and, on cancellation, reclaim its result to remove the dir before re-raising. mkdtemp stays offloaded (the blocking-io gate flags os.mkdir from deerflow code). Adds a deterministic cancellation regression. * fix(workspace-changes): drain repeated cancellation in text-cache reclaim The mkdtemp handoff guard reclaimed the shielded worker's result on the first CancelledError, but the reclaim await was itself cancellable: a second cancel landed there, slipped past `except Exception` (CancelledError is BaseException), and skipped the reclaim while the shielded worker still finished — orphaning the deerflow-workspace-changes-* dir. Repeated cancelled runs accumulate leaks. Move reclaim+remove into a task the caller cannot abandon and drain repeated cancellation until it completes, then restore the cancellation. A repeat cancel interrupts the await, not the task, so the dir is never abandoned; the loop exits only once cleanup has finished, leaving no pending task. Non-cancel paths are unchanged. Adds test_capture_workspace_snapshot_repeated_cancellation_leaks_no_text_cache (double-cancel regression). make test-blocking-io: 43 passed. |
||
|
Huixin615andGitHub
|
c9b6131f8f |
fix(skills): reload mounted skills without restarting Gateway (#4264)
* fix(skills): add admin-only reload endpoint * fix(skills): preserve cache when reload fails |
||
|
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. |
||
|
AariandGitHub
|
5a5c661e9f |
fix(middleware): recover malformed tool-call ids in dangling repair (#4246)
* fix(middleware): recover malformed tool-call ids in dangling repair DanglingToolCallMiddleware normalizes malformed tool-call names (#4008) and arguments (#4193) so strict OpenAI-compatible providers do not reject the next request. The id is the third field of that same recovery contract and was left alone. A provider that emits an empty id -- or omits it -- parses into a well-formed tool_calls entry, so it reaches the middleware through the normal path. The empty id never enters the pairing set, so the orphan pass drops the call's already-produced ToolMessage and the placeholder pass skips the call. The request then goes out carrying an empty id and with the real tool result gone. Normalize ids up front and re-point each already-paired ToolMessage at its call's new id, so the existing pairing/orphan/placeholder logic no longer sees a malformed id. Only the view that is actually read and serialized is relabelled; a valid id is left byte-for-byte alone, since it is matched verbatim against ToolMessage.tool_call_id. * fix(middleware): scope malformed-id result pairing to its own turn Malformed tool-call ids are all equally empty, so pairing recovered results by the original id alone was a global FIFO over the whole transcript. An earlier dangling call then consumed a later turn's result: the real result was served to the wrong call while the call that actually ran got the interrupted placeholder. An orphan result whose originating AIMessage was already gone could likewise be adopted by a later malformed call, resurrecting it instead of being dropped as the orphan pass intends. Walk the messages once in document order so only the most recent AIMessage's unanswered calls are claimable, which keeps a result answering the turn that issued it, and rule out the wrong parallel sibling within a turn by tool name. A result whose name matches no open call is left malformed for the existing orphan pass to drop rather than repurposed as some other call's answer. * fix(middleware): keep the shadowed raw view out of id recovery * fix(middleware): only claim a malformed result when the pairing is forced * docs(middleware): cite ToolNode's ordering guarantee for positional pairing |
||
|
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. |
||
|
|
7156e745e0 |
fix(channels): don't treat a bare "connect" as a bind command (#4251)
`extract_connect_code` matched `command in {"/connect", "connect"}`, so any
message whose first word is the ordinary English word "connect" was parsed as
`/connect <code>` and its next word swallowed as a one-time bind code. For
example `extract_connect_code("connect the database to the api")` returned
`"the"` instead of `None`, meaning a normal chat message never reached the
agent and instead attempted a channel bind.
Every other channel control command requires a leading slash:
`is_known_channel_command` only matches `/`-prefixed tokens and
`KNOWN_CHANNEL_COMMANDS` holds only slash forms (`/goal`, `/new`, ...). The
docstrings and AGENTS.md advertise only `/connect <code>`. The bare alias was
inconsistent with all of that and produced the false-positive bind.
Match only `/connect`. The `.lower()` keeps it case-insensitive (`/Connect`)
and the mention-prefix handling from #4222 is unaffected, so
`@bot /connect <code>` still binds.
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
||
|
hataaandGitHub
|
10890e10a8 | feat(authz): propagate trusted authorization principal context (#4203) | ||
|
Huixin615andGitHub
|
f9340c1f08 |
test(mcp): cover passive skill tool visibility (#4247)
* test(mcp): cover passive skill tool visibility * test(mcp): tighten deferred discovery coverage |