mirror of
https://github.com/bytedance/deer-flow.git
synced 2026-07-28 00:48:07 +00:00
644 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
Daoyuan Li
|
44990ff194
|
fix(mcp): use threading.Lock for OAuth token refresh to avoid cross-thread deadlock (#4240)
* fix(mcp): use threading.Lock for OAuth token refresh to avoid cross-thread deadlock
OAuthTokenManager created one asyncio.Lock per server for the process
lifetime. The embedded/TUI sync tool-call path (DeerFlowClient.stream()
-> LangGraph's ToolNode._func -> a ThreadPoolExecutor ->
make_sync_tool_wrapper's per-call asyncio.run()) invokes
get_authorization_header from a fresh event loop on a fresh OS thread
for every concurrent tool call. asyncio.Lock binds to whichever loop
first contends on it; when a caller on a different loop later releases
or wakes a waiter, it does so without call_soon_threadsafe, so the
waiting loop's selector is never woken and that caller hangs forever
with no exception. A third concurrent caller instead raises a
synchronous RuntimeError ("bound to a different event loop"). Either
way, two concurrent OAuth-protected tool calls (including the very
first cold-start token fetch) can freeze the entire agent turn.
Gateway's async path (ToolNode._afunc) is unaffected.
Replace the asyncio.Lock with a plain threading.Lock, acquired via
asyncio.to_thread so the blocking wait never blocks the event loop,
and released synchronously in a finally block. This keeps the
single-fetch de-duplication the lock provided while making it safe
across however many event loops/threads call into the same server's
lock.
Adds a regression test that runs three threads, each with its own
event loop, calling get_authorization_header concurrently for the same
server, and asserts (with a bounded join timeout so a regression fails
fast instead of hanging the suite) that none hang or raise, and that
only one real token fetch happens.
* fix(mcp): make OAuth lock acquisition cancellation-safe
get_authorization_header acquired the per-server threading.Lock via a
bare `await asyncio.to_thread(lock.acquire)`, with the try/finally that
guarantees release only starting after that await returned. Once the
executor thread had actually started running lock.acquire(), cancelling
the awaiting caller only stopped the caller -- Python cannot interrupt a
running OS thread. CancelledError was still delivered to the caller
immediately, but the thread kept blocking until the current holder
released, then silently acquired the lock with nobody left to call
release() for it. The lock stayed locked forever and every later OAuth
token refresh for that server blocked permanently at the same line --
the exact cross-thread deadlock this lock was introduced to prevent,
reintroduced via a different path under cancellation (e.g. a caller
wrapped in asyncio.wait_for/asyncio.timeout, or task-group cancellation).
Run the acquisition as an explicit asyncio.create_task, awaited via
asyncio.shield() so cancelling the caller no longer cancels the
underlying acquisition task. If the caller is cancelled, keep
(re-)waiting on the still-shielded acquisition task -- tolerating
further cancellation during this cleanup by simply retrying -- until it
actually finishes, release the lock immediately, and only then
re-raise. This guarantees the lock is released regardless of when or
how many times the caller is cancelled: before the acquisition is even
scheduled, while queued, or after it has already been silently granted.
Adds a regression test that holds the per-server lock, starts a second
caller that has to wait for it, cancels that caller while it is
genuinely blocked in its executor thread, releases the original holder,
and asserts a third caller completes within a bounded asyncio.wait_for
and still performs exactly one token fetch. Every potentially-hanging
await is bounded so a regression fails the test quickly instead of
hanging the suite.
|
||
|
March7
|
8c78d1f41f
|
fix(subagents): load user-scoped skills (#4356) | ||
|
Daoyuan Li
|
09d9cf53d2
|
fix(harness): add timeout to invoke_acp_agent to prevent indefinite hangs (#4238)
invoke_acp_agent had no timeout anywhere in its call path, and ACPAgentConfig had no timeout field. If the ACP agent subprocess answers initialize/new_session correctly but then hangs inside prompt(), the tool call - and therefore the whole agent turn - blocks indefinitely, with the child process left running. MCP stdio servers already guard against this class of hang via tool_call_timeout; ACP agent invocations had no equivalent. Add ACPAgentConfig.timeout_seconds (default 1800, ge=1), mirroring the shape/default of subagents.timeout_seconds, and wrap the conn.prompt() call in asyncio.wait_for(). On TimeoutError, return a clear error instead of hanging; exiting the spawn_agent_process context block triggers the ACP library's own graceful-then-forceful subprocess cleanup, so the hung process is actually terminated. |
||
|
lllyfff
|
01a89f2379
|
[feat] memory: pluggable MemoryManager interface for backend onboarding (#4326)
* refactor(memory): pluggable MemoryManager interface for backend onboarding
Optimize the MemoryManager interface layer so new backends (mem0/openviking)
onboard with less code and the contract stays stable as capabilities are
added. A minimal backend now implements only from_config + add + get_context
(verified by test_memory_manager_interface.py::_MinimalBackend onboarding via
the factory); the factory no longer knows a backend's private hooks.
- MemoryManager: ABC -> pydantic BaseModel; three-tier methods (tier-1
add/get_context abstract; tier-2 management defaults; tier-3 optional hooks
warm/reload/fact + on_pre_compress/on_turn_start). Dropped 3 self-serving
hooks. 6 hasattr probe sites -> direct call + try/except NotImplementedError.
- from_config classmethod: factory thins to resolve + inject storage_path +
collect host hooks + call from_config; DeerMem-specific hook consumption
moved from factory to DeerMem.from_config.
- Invariants: @model_validator (mode='tool' requires search via supports_search
ClassVar); DeerMemConfig storage_path-is-file check moved here from factory.
- Async: aadd/aget_context/asearch default to the sync path (speculative).
- Callbacks: MemoryCallbacks + LangfuseMemoryCallbacks; on_memory_llm_call
subsumes tracing_callback (same signature/timing/mutation); deleted the
tracing_callback field. DeerMem decoupled from langfuse (portability).
- noop keeps read-op empty overrides (avoids router 500s on the
disable-memory-via-noop path); only delete/export inherit the base raise.
Behavior preserved: 661 passed / 13 skipped. Docs: backends/README.md rewritten
(three-tier + from_config + callbacks); samples README updated; removed stale
private doc paths.
Co-Authored-By: Claude <noreply@anthropic.com>
* fix(memory): 501 on unsupported read/manage endpoints + accurate warm log
Review follow-up on the three-tier MemoryManager refactor.
- Read/manage endpoints (GET /memory, /memory/export, /memory/status,
DELETE /memory, POST /memory/import) and the /memory/reload fallback now
catch NotImplementedError -> 501, matching the fact-CRUD endpoints. The
hasattr->try/except migration had skipped these: they were @abstractmethod
before (every backend implemented them, so they never raised), so once they
became tier-2 default-raise a minimal backend (only add + get_context) hit a
raw 500 -- there is no global NotImplementedError handler. get_memory is
shared via _get_memory_or_501 (covers /memory, export, status, reload
fallback). noop is unchanged: its read-op empty overrides never raise.
- warm() base default returns None (tri-state: True=warmed, False=failed,
None=nothing to warm) so the Gateway lifespan logs "skipping" for a
non-DeerMem backend (e.g. noop) instead of the inaccurate "warmed
successfully" it never earned. DeerMem.warm keeps True/False.
- Tests: 6 router 501 tests (read/manage + reload fallback) + 2 lifespan
warm-log tests (None->skipping, False->warning); conformance/pluggable
assert warm() is None.
705 passed / 13 skipped; lint clean.
Co-Authored-By: Claude <noreply@anthropic.com>
* fix(memory): review follow-ups - search-flag consistency, client reload, backend_config purity
Address review feedback on the three-tier MemoryManager refactor:
- [Medium] supports_search/search drift: the invariant now requires the
supports_search ClassVar flag to MATCH whether search() is actually
overridden (type(self).search is not MemoryManager.search), so the flag
can't drift from the impl. Catches both directions at instantiation: a
backend that overrides search() but forgets supports_search=True (was a
misleading tool-mode rejection), and one that sets the flag without
overriding (was a runtime NotImplementedError on the first memory_search).
noop sets supports_search=True to match its search() override. Conformance
adds drift + consistent-backend tests.
- [Low] client.reload_memory fallback: wrap the get_memory fallback so a
minimal backend (only add + get_context) surfaces a clean NotImplementedError
("implements neither reload_memory nor get_memory") instead of an uncaught
propagation -- mirrors the router's 501. Test added.
- [Low] backend_config purity: DeerMem.from_config restores backend_config to
the pure data the host passed after model_post_init parses the injected hooks
into DeerMemConfig (self._config, PrivateAttr); the field stays serializable
(no callables/LLM) and matches the README ("host hooks NOT in backend_config").
Test asserts purity + hooks wired.
- [Low] CHANGELOG: breaking-change note that mode='tool' + non-search backend
now fails fast at startup (was silently empty) so operators recognize it on
upgrade.
- [Nit] .gitignore: drop the env-specific .tmp-pytest/ entry (--basetemp is
local-only, not make test/CI).
709 passed / 13 skipped; lint clean.
Co-Authored-By: Claude <noreply@anthropic.com>
* docs(changelog): correct memory tool-mode fail-fast note
The CHANGELOG entry said mode='tool' + a non-search backend "(e.g. noop)"
fails fast at startup, but noop overrides search() (returns []) and sets
supports_search=True (required by the consistency invariant), so noop IS
search-capable and noop+tool does NOT fail fast. The fail-fast only affects a
custom backend that onboards without overriding search(). Reworded to drop the
misleading noop example and state both shipping backends implement search().
Co-Authored-By: Claude <noreply@anthropic.com>
---------
Co-authored-by: Claude <noreply@anthropic.com>
|
||
|
Aari
|
05e4f4f6d8
|
fix(sandbox): bound E2B output synchronization resources (#4364)
* fix(sandbox): bound E2B output synchronization resources E2B release-time output sync pulled every changed file back from the remote VM with only a per-file size cap and no aggregate bound, so a pathological outputs tree (thousands of files, or many sub-cap files summing to gigabytes, or a slow VM) could make release download unboundedly on a hot path that runs at every agent turn end. Add three aggregate ceilings on top of the per-file cap — total bytes, file count, and a wall-clock deadline — enforced in the sync loop. When a ceiling is hit the pass stops early, logs what it dropped, and defers the rest to the next release. A truncated pass skips stale-manifest pruning so files it never reached are reconciled next time instead of being forgotten and re-downloaded. Closes #4340 * test(sandbox): pin multi-pass convergence of bounded output sync The four truncation tests each exercise a single capped pass. Add a two-pass test that locks in the invariant the design relies on for correctness: already-synced files are skipped before the budget check, so they never consume the cap and the deferred tail drains over successive releases instead of the leading files being re-downloaded every turn. A refactor that let a skipped file consume the cap would pass the single-pass tests but fail this one. |
||
|
Lee minjing
|
e225ad57d7
|
feat(uploads): lazy-load historical files via list_uploaded_files tool (#4174)
* feat(uploads): lazy-load historical files via list_uploaded_files tool
Replace per-turn injection of all historical upload metadata with on-demand
discovery via a new `list_uploaded_files` built-in tool, following the same
deferred-discovery pattern used by skills.
- Rename <uploaded_files> block to <current_uploads> (current-run files only)
- Add list_uploaded_files tool with include_outline: bool|list[str]
- Extract outline helpers to shared deerflow/utils/file_outline.py
- Update system prompt to reflect lazy-loading behaviour
- Historical file scan removed from UploadsMiddleware.before_agent()
Co-Authored-By: Claude <noreply@anthropic.com>
* fix(uploads): clear uploaded_files state when no new files in current turn
When before_agent() returns None on empty turns, the LastValue
uploaded_files field retains the previous turn's filenames.
list_uploaded_files then incorrectly excludes those files as
"current-run" files, making them invisible until the next upload.
Fix: return {"uploaded_files": []} instead of None to explicitly
clear state. Add two-turn regression test covering the exact
scenario from review feedback.
Co-Authored-By: Claude <noreply@anthropic.com>
* fix: resolve CI lint errors and stale test assertion from merge
- Split long prompt line to fit 240-char limit
- Add missing `Any` import in list_uploaded_files_tool
- Remove unused `re` import in file_conversion (outline code moved)
- Remove unused `os` import in middleware test
- Fix test assertion: <uploaded_files> → <current_uploads> after main merge
Co-Authored-By: Claude <noreply@anthropic.com>
* fix: resolve CI lint errors and stale test assertion from merge
- Split long prompt line to fit 240-char limit
- Add missing `Any` import in list_uploaded_files_tool
- Remove unused `re` import in file_conversion (outline code moved)
- Remove unused `os` import in middleware test
- Fix test assertion: <uploaded_files> → <current_uploads> after main merge
Co-Authored-By: Claude <noreply@anthropic.com>
* fix: add current_uploads to input sanitization exempt tags
The lazy-loading PR renamed <uploaded_files> to <current_uploads>.
The anti-drift guard scans all framework XML blocks and requires each
to be either blocked or explicitly exempted. current_uploads wraps
trusted server-generated file metadata, not user input, so it belongs
in the exempt set.
Co-Authored-By: Claude <noreply@anthropic.com>
* test: regenerate replay golden after uploaded_files state change
before_agent now returns {"uploaded_files": []} instead of None,
adding uploaded_files to SSE values events. Regenerated via
DEERFLOW_WRITE_GOLDEN=1.
Co-Authored-By: Claude <noreply@anthropic.com>
* fix: review feedback — memory pipeline, stale tags, state clearing, nits
- Match both tags in memory stripping pipeline (uploaded_files|current_uploads)
- Remove stale uploaded_files from _BLOCKED_TAG_NAMES
- Clear uploaded_files on all before_agent early-return paths
- Fix ponytail: stray word in file_conversion re-export comment
- Remove dead total_omitted branch in _format_omitted_summary
- ruff format fixes
Co-Authored-By: Claude <noreply@anthropic.com>
* fix: block current_uploads, sanitize only original user content
Per review feedback: instead of exempting <current_uploads> (which
allows user forgery), move it to _BLOCKED_TAG_NAMES and change
InputSanitizationMiddleware._process_request to scan only the
original user content (ORIGINAL_USER_CONTENT_KEY) when available.
Server-injected trusted blocks are no longer checked against the
blocked-tag denylist.
Co-Authored-By: Claude <noreply@anthropic.com>
* docs: clarify fallback reason in input sanitization comment
Co-Authored-By: Claude <noreply@anthropic.com>
* @
fix: third-round review feedback — state visibility, sanitization, regex, nits
- list_uploaded_files_tool: logger.warning instead of silent try/except
on runtime.state read failure (High)
- input_sanitization_middleware: _extract_text_from_content skips empty
text blocks to match message_content_to_text behaviour; rfind fallback
path logs warning for observability (Medium)
- memory pipeline regexes: backreference (?P<tag>)(?P=tag) in
message_processing.py and prompt.py (Low)
- file_conversion.py: re-export moved to top of file (Low)
- Tests: middleware→tool state bridge test; integrated forged-tag +
multimodal sanitization tests
PR #4174 — Follow-up issues: #4212, #4213, #4214
Co-Authored-By: Claude <noreply@anthropic.com>
@
* @
fix: 4th-round review — denylist, sanitization, scandir, nits
- Add "uploaded_files" back to _BLOCKED_TAG_NAMES (old tag still processed by
deermem; user forgery must be escaped) (consistency)
- Fix inaccurate rfind-fallback comment: UploadsMiddleware keeps string as
string, fallback is unreachable for strings (doc fix)
- Distinguish "empty string key" (upload without text) from "non-string key"
(caller forgery) so empty-text uploads never escape the server block (edge)
- Merge dual os.scandir(uploads_dir) calls into one list re-use (minor)
- Add comment on .md sibling skip known limitation: user-uploaded .md files
whose stem collides with a converted doc are hidden (boundary, no code change)
Co-Authored-By: Claude <noreply@anthropic.com>
@
* @
fix: tighten rfind-failure fallback — distinguish server blocks from user blocks
When _extract_text_from_content and message_content_to_text disagree on
multimodal list content and rfind fails, use content[0] (server-injected
<current_uploads> block) vs content[1:] (user blocks) to sanitize only
user blocks. Raw strings and non-standard dict blocks that
_extract_text_from_content misses are now also sanitized.
Non-distinguishable paths (< 2 text blocks, non-list content) still
degrade to full sanitization (safe — server block may be escaped but
user forgery never leaks). All fallback paths log via logger.warning.
Decision 18 / willem-bd 4th-round comment #3
Co-Authored-By: Claude <noreply@anthropic.com>
@
* @
fix: correct comments referencing text_blocks → content in rfind fallback
Co-Authored-By: Claude <noreply@anthropic.com>
@
* fix: 5th-round review — dead code, subagent gating, integration test, perf, consistency
- Delete unreachable ORIGINAL_USER_CONTENT_KEY guard in rfind fallback
branch (original_user_content guaranteed non-empty str at that point)
- Remove list_uploaded_files from BUILTIN_TOOLS; add include_upload_tool
param to get_available_tools(), default True; task_tool.py passes False
so subagents no longer receive a tool whose state exclusion is broken
- Add integration test exercising real create_agent graph (not mocked
runtime.state) to verify LangGraph propagates before_agent state writes
into ToolRuntime.state during same-turn tool calls
- Cache DirEntry.stat() st_size in candidates tuple to avoid second
per-file syscall in the rendering loop
- Make the upload-tag pre-check case-insensitive (content_str.lower())
to match _UPLOAD_BLOCK_RE re.IGNORECASE
PR #4174 — willem-bd 5th-round review items #1-#5
Co-Authored-By: Claude <noreply@anthropic.com>
* fix(channels): pass files metadata through _human_input_message() for IM uploads
_human_input_message() was not passing additional_kwargs.files to the
downstream message. UploadsMiddleware read no files, wrote
uploaded_files=[], and list_uploaded_files reported same-run IM
attachments as historical files (fancyboi999 repro).
Fix: add files parameter to _human_input_message(), call site passes
files=uploaded. Regression test locks the contract.
Co-Authored-By: Claude <noreply@anthropic.com>
* fix(channels): remove legacy <uploaded_files> manual prepend to fix double-injection regression
Commit 8d86dbf6 added files= pass-through to UploadsMiddleware but
left the manual _format_uploaded_files_block() prepend in place.
Every IM attachment reached the model twice — once via the legacy
<uploaded_files> block and once via <current_uploads>.
This commit removes the manual prepend and the now-dead
_format_uploaded_files_block() function. UploadsMiddleware is the
sole upload-context producer for both IM and web paths.
Reported-by: fancyboi999 (PR review)
Co-Authored-By: Claude <noreply@anthropic.com>
* docs: update #4212 issue body to reflect completed fixes and narrowed remaining scope
* chore: remove temporary scratch file
* fix(middleware): neutralize user-derived values inside <current_uploads> block
Upload-derived filenames, paths, outline titles, and preview text are
interpolated verbatim inside the trusted <current_uploads> wrapper,
which InputSanitizationMiddleware exempts from sanitization. A crafted
filename or document heading containing blocked authority tags would
bypass the guardrail and enter model context as trusted framework data.
Fix: call neutralize_untrusted_tags() on all four user-derived values
inside _format_file_entry(), preserving the outer <current_uploads>
wrapper untouched.
Reported-by: fancyboi999 (P1 security review)
Co-Authored-By: Claude <noreply@anthropic.com>
* fix(middleware): neutralize extension labels in omitted-file summary
Files exceeding the 10-item context cap bypass _format_file_entry().
Their extensions, derived from user-controlled filenames via
_extension_label(), were interpolated verbatim into the trusted
<current_uploads> wrapper — another path for blocked authority tags
to escape the guardrail.
Fix: neutralize extension values inside _extension_label(), the
single extraction point for all extension labels.
Reported-by: fancyboi999 (P1 security review)
Co-Authored-By: Claude <noreply@anthropic.com>
* fix(tools): neutralize user-derived values in list_uploaded_files tool result
Apply neutralize_untrusted_tags() to every model-visible user-derived value
returned by list_uploaded_files: filename, virtual path, extension, outline
titles, outline preview lines, and omitted-file extension summary.
This closes the last remaining injection bypass in the upload lazy-loading
path - the <current_uploads> block and its omitted summary were already
neutralized (previous commits), but the list_uploaded_files tool produced
a second exit for the same attacker-controlled metadata that
ToolResultSanitizationMiddleware did not cover.
Co-Authored-By: Claude <noreply@anthropic.com>
* fix(tests): add missing include_upload_tool=False to task_tool mock assertions
PR #4174 added include_upload_tool parameter to get_available_tools().
task_tool.py correctly passes include_upload_tool=False for subagents
but 5 existing tests' assert_called_once_with expectations were not
updated, causing CI failures.
Co-Authored-By: Claude <noreply@anthropic.com>
---------
Co-authored-by: Claude <noreply@anthropic.com>
|
||
|
Ryker_Feng
|
20debf9cc7
|
feat(agents): per-agent model and generation settings (#4347)
* feat(agents): per-agent model and generation settings Let each custom agent choose its own model and sampling settings (temperature, max_tokens) plus thinking / reasoning_effort defaults, so agents sharing a model profile are no longer stuck with one shared temperature and output length (#4336). AgentConfig gains optional model_settings / thinking_enabled / reasoning_effort (None = inherit). create_chat_model applies per-caller model_overrides on top of the profile before the thinking/Codex transforms; the lead agent resolves each knob with precedence request > agent config > profile/default. The /api/agents create/update routes persist the fields and reject an unknown model. The default lead agent path is unchanged (no agent config -> overrides None). The agent chat composer also stops force-overriding an agent's configured default model with models[0]. * fix(agents): tri-state thinking control and default-model capability gating The model-settings dialog seeded the thinking switch to false, so opening it to tweak temperature and saving silently disabled thinking (the runtime default is on) with no way back to inherit. It also hid the thinking / reasoning controls whenever the agent inherited the global default model, since `__default__` never resolved through `models.find`. Give thinking an explicit Inherit / On / Off tri-state so an untouched save is a no-op, and resolve `__default__` to the effective default (models[0]) for the capability check. Logic lives in the tested helpers module. |
||
|
Andrew Chen
|
ae510cb2e8
|
fix(sandbox): make an empty old_str a no-op in str_replace on any file (#4256)
str_replace guards the replacement with `if old_str not in content`, which
cannot reject an empty old_str -- `"" in content` is always true. So an
empty old_str reached `str.replace("", new_str)`, which inserts new_str at
every character boundary, and the tool rewrote the file while still
returning "OK":
old_str='', new_str='# H\n' -> OK, file silently prepended
old_str='', new_str='X', replace_all -> OK, 'XdXeXfX XmXaXiXnX(X)X:X\nX...'
The empty-file branch above it already handles this case (`if not content:
if not old_str: return "OK"`), and the existing test states the intent
directly: "An empty old_str is a no-op edit and remains a benign OK". That
contract just never held once the file had content.
The tool is registered by default (config.example.yaml) and its schema
declares old_str as a plain string with no minLength, so a model can emit
"" legitimately; read-before-write only compares a hash and lets it past.
Check old_str first so the no-op holds whatever the file contains. The
empty-file case folds into the same not-found branch, which keeps its
message and behaviour.
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
||
|
Daoyuan Li
|
495e90832c
|
fix(sandbox): scope e2b grep() glob filter to its directory prefix (#4168)
E2BSandbox.grep()'s glob handling reduced a directory-scoped pattern like "src/*.js" down to just "*.js" before passing it to `grep --include=`, dropping the directory-scoping prefix entirely. GNU grep's `--include` matches by basename only, at any depth, so the search silently broadened to every matching-extension file in the sandbox tree instead of just the directory the caller asked for. Keep the basename portion as a coarse `--include=` pre-filter (a superset of the true match set) and post-filter grep's raw hits through path_matches(), the same helper glob() already uses to enforce directory scoping correctly, so grep and glob agree on what a directory-scoped pattern means. |
||
|
March7
|
ce4a6d4c3d
|
fix(backend): remove transient image context after model calls (#4267)
* fix(backend): discard transient image context * fix(backend): protect client image context ids * docs(backend): clarify image checkpoint lifecycle |
||
|
Vanzeren
|
42baed8c8c
|
feat(checkpoint): dual-mode checkpoint storage with LangGraph DeltaChannel (#4292)
* feat(checkpoint): dual-mode checkpoint storage with LangGraph DeltaChannel
Add a restart-required database.checkpoint_channel_mode ("full" default,
"delta") that stores the messages channel via LangGraph 1.2 DeltaChannel,
cutting checkpoint storage from O(n^2) to O(n) for append-only history.
Existing full checkpoints seed delta state transparently; no data migration.
- config: mode schema + freeze-on-first-use with
CheckpointModeReconfigurationError; mode marker persisted in checkpoint
metadata; unsafe delta->full downgrade rejected fail-closed with
CheckpointModeMismatchError (run-level error, failed state read)
- state: delta message state schema; CheckpointStateAccessor centralizes
materialized reads for all consumers (threads API, branches,
regeneration, compaction, state updates, memory, goal workers)
- runtime: raw writers (run durations, interrupted title, thread goal)
parent their checkpoints to the checkpoint they derive from, preserving
delta ancestry; rollback forks the pre-run lineage through a state
mutation graph with Overwrite restores; InMemorySaver delta-history
override delegates to the base walk (fixes dropped first write after
migration, also present upstream)
- tests: conformance suite over {memory, sqlite, postgres} covering
migration replay, stable message IDs, storage shape and writer
preservation; conftest fixture isolates the frozen mode between tests;
stale config fakes refreshed
- ci: backend unit tests gain a postgres service
* fix(checkpoint): close materialization gaps in goal flow, guard public factory
- Route goal-continuation message reads through CheckpointStateAccessor:
raw channel_values reads see the delta sentinel in delta mode, which
disabled goal continuation (stand_down=no_durable_end_of_turn) after
durable assistant turns. Raw tuples remain for tuple-only metadata
(checkpoint id, pending_writes).
- Reject checkpoint_channel_mode='delta' + checkpointer in
create_deerflow_agent at construction: factory-built persisted graphs
bypass mode-marker injection and the fail-closed gate, reproducing
silent mixed-mode state loss. Delta without persistence stays allowed.
- Import the postgres saver lazily (pytest.importorskip in the fixture)
so the documented default install collects the suite; add a CI job
running pytest --collect-only on uv sync --group dev without extras.
- Fix test_checkpointer fallback test to patch get_app_config at its
use site (provider module), making it deterministic when a local
config.yaml selects a persistent backend.
* fix(gateway): preserve extension-owned channels in state mutations, bump config version
- build_state_mutation_graph / build_checkpoint_state_mutation_accessor
accept an explicit state_schema; branch and POST /state now compile the
mutation graph from the thread's effective schema
(graph_state_schema on the assistant graph). The base-ThreadState
fallback silently discarded channels contributed by custom
AgentMiddleware.state_schema on branch (data loss) and returned a
false-success 200 on POST /state.
- POST /state validates values keys against the mutation graph's
channels and rejects unknown fields with 422 instead of ignoring
them; reducer detection covers extension channels
(BinaryOperatorAggregate or DeltaChannel) so Overwrite replace
semantics work for middleware reducers in both modes.
- Endpoint regression: custom AgentMiddleware.state_schema value
survives branch, updates through POST /state, and an unknown field
receives 422.
- config_version 26 -> 27 for the new database.checkpoint_channel_mode
(example, Helm chart values + README, support-bundle fixture), so
existing installs get the outdated-config warning and
make config-upgrade merges the field; covered by a test driving the
real example file and the real config-upgrade script.
* fix(gateway): resolve assistant schema via one boundary, copy branch reducer values with Overwrite
GET /threads/{id}/state now resolves the thread's assistant_id through a
single reusable boundary (thread metadata -> assistant_id -> effective
graph), so channels contributed by a custom AgentMiddleware.state_schema
are materialized instead of dropped by the default lead schema. POST
/state uses the same boundary instead of resolving the schema ad hoc.
Branch writes wrap every copied reducer channel in Overwrite (derived
from the effective mutation graph: BinaryOperatorAggregate + DeltaChannel),
not just messages, so already-aggregated values are never re-merged.
Regression tests use a real AgentMiddleware.state_schema with a
non-identity reducer in both full and delta modes: GET /state returns the
extension value, POST /state replaces it, branch preserves it
byte-for-byte; the unknown-field 422 is a separate assertion.
* refactor(checkpoint): collapse read-path round-trips and ship dual-mode parity tests
Address review round 4 on PR #4292:
- Push ahistory/history limit through Pregel into checkpointer.alist
(SQL LIMIT) instead of materializing all rows and breaking in Python
- Fold the read-side mode-compat gate onto the returned snapshot's
metadata; only writes keep the pre-write tuple fetch (fail-closed)
- Cache factory-built accessor graphs per (assistant_id, mode) with
factory-identity revalidation; state reads no longer build a lead
agent per request
- get_thread: one snapshot fetch + one raw pending_writes fetch on the
resolved checkpoint (post-checkpoint __error__ writes never surface
in snapshot.tasks; verified empirically)
- DeerFlowClient.get_thread: single checkpointer.list walk collects
pending_writes per checkpoint instead of N get_tuple calls
- InMemorySaver delta-history patch: stand-down when the upstream
override disappears, try/except guard, validated-version warning,
guard tests
- make_lead_agent mode precedence: first freeze is owned by app_config
(client-supplied configurable key ignored); once frozen, injected
key/app_config must match or fail closed
- Rollback: lock in non-message channel restoration via fork
inheritance with a dedicated reducer-channel test
- Add tests/test_threads_checkpoint_mode.py and
tests/test_gateway_checkpoint_mode.py referenced by AGENTS.md and
the PR validation section: lifecycle parity (memory + sqlite),
per-step blob-count storage guard, gateway endpoint parity
Counted-saver tests pin checkpoint round-trips for aget/ahistory so
these regressions cannot silently return.
* fix(checkpoint): precise mode-mismatch HTTP mapping, gate E2E, and accessor resilience
- threads router: map CheckpointModeMismatchError to 409 (with cause and
thread id) and CheckpointModeReconfigurationError to 503 across all state
endpoints instead of swallowing both into a generic 500
- gate coverage: seed a real delta checkpoint into AsyncSqliteSaver and
assert aget/aupdate/ahistory fail closed; assert 409 at the HTTP boundary
through the real route stack
- rollback: compile the restore mutation graph with the thread's effective
state schema per the build_state_mutation_graph contract
- inheritance contract locks: rollback and manual compaction preserve
middleware-contributed channels via checkpoint fork cloning
- services: revalidate the accessor-graph cache against app_config identity
so config.yaml hot-reloads never serve a stale compiled graph
- services: degrade full-mode state reads to raw checkpointer reads when the
agent factory is unavailable (delta gate still applies; delta mode has no
fallback)
- deps: override websockets==16.0 (langgraph-sdk 0.4.2's <16 pin silently
downgraded 16.0 -> 15.0.1; pin is not grounded in any API incompatibility)
and bump the langchain lower bound to what the lockfile actually resolves
* fix(checkpoint): include anchor checkpoint in degraded history walk + cover get_thread
- _RawCheckpointReadAccessor.ahistory: alist(before=...) is exclusive while
pregel's get_state_history treats config.checkpoint_id as the inclusive
start; fetch the anchor explicitly so both read paths paginate identically
- extend the degraded-path gateway test: GET /thread returns raw values, and
POST /history with before starts at the anchor checkpoint
* fix(gateway): preserve degraded checkpoint timestamps
* fix(gateway): harden degraded checkpoint access
* fix(gateway): resolve assistants for checkpoint reads
---------
Co-authored-by: Willem Jiang <willem.jiang@gmail.com>
|
||
|
Ryker_Feng
|
8dafb667dd
|
fix(tui): derive /help text from the command registry (#4327)
The /help string was hardcoded and had drifted from BUILTIN_COMMANDS, omitting six real commands (help, switch, resume, uploads, artifacts, details) and ordering the rest differently from the picker. Generate the command line from the registry so /help can never fall out of sync again, and add parity tests that guard against future drift. |
||
|
RongfuShuiping
|
4bf028d048
|
feat(memory): add incremental agent-scoped Markdown fact storage (#4279)
* feat(memory): add Markdown fact storage repository * docs(memory): explain storage rewrite for beginners * docs(memory): fix plan markdown formatting * refactor(memory): separate global summaries from agent facts * fix(memory): make Markdown fact updates incremental and safe * Update STORAGE_REWRITE_CHANGES.md * Delete docs/plans/STORAGE_REWRITE_PLAN.md * Delete docs/plans/STORAGE_REWRITE_CHANGES.md * fix(memory): address Markdown storage review feedback * fix(memory): complete review follow-ups * fix(memory): resolve storage review findings * feat(memory): add proactive Markdown migration CLI * fix(memory): harden Markdown storage concurrency * fix(memory): harden markdown storage migration * fix(memory): close migration review gaps --------- Co-authored-by: Willem Jiang <willem.jiang@gmail.com> Co-authored-by: qin-chenghan <qinchenghan@Huawei.com> |
||
|
Aari
|
3c0a45ad77
|
fix(skills): inject Langfuse metadata into the standalone skill scan (#4321) | ||
|
luo jiyin
|
d2116d861b
|
fix(sandbox): sync same-size E2B output updates (#4329)
* fix(sandbox): detect same-size E2B output updates * test(sandbox): cover E2B output modification times * fix(sandbox): persist E2B output sync versions * test(sandbox): cover E2B output sync manifest * docs(backend): document E2B sync manifest * fix(sandbox): scope E2B sync manifests * test(sandbox): cover E2B sync manifest lifecycle * docs(backend): describe E2B manifest lifecycle * fix(sandbox): fall back from empty E2B sandbox ID * test(sandbox): cover empty E2B sandbox IDs * docs: clarify E2B output sync ownership |
||
|
luo jiyin
|
da3feb3863
|
fix(sandbox): fail E2B bootstrap safely (#4325)
* fix(sandbox): fail E2B bootstrap safely * test(sandbox): cover E2B bootstrap cleanup * docs(backend): document E2B bootstrap failure * fix(sandbox): discard E2B bootstrap failures * test(sandbox): cover E2B reconnect bootstrap failures * docs(backend): clarify E2B bootstrap recovery * fix: preserve E2B bootstrap errors * fix: reject falsey E2B bootstrap errors |
||
|
Andrew Chen
|
f113f10f36
|
fix(workspace-changes): count diff body lines starting with "-- "/"++ " (#4303)
`_count_diff_lines` skipped every line starting with "+++ "/"--- " to drop difflib's two file headers. But a deleted hunk-body line whose content begins with "-- " (e.g. a SQL comment `-- get users`) becomes the diff line `--- get users`, and an added `++ ...` line becomes `+++ ...`, so those were dropped too — undercounting the user-visible +N/-M change summary (WorkspaceChangeSummary, per-file WorkspaceFileChange, and the run-event string in recorder.py). `difflib.unified_diff` always emits the two headers first, so skip them by position (`lines[2:]`) instead. Hunk `@@` lines start with `@` and are still ignored. Numeric/plain diffs are unaffected. Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> |
||
|
Ryker_Feng
|
fa496c0c8d
|
feat(browser): add agentic browser control (#4187)
* feat(browser): add agentic browser control
* fix(frontend): format browser view changes
* fix(browser): keep browser optional and isolate sidecar layout
* fix(browser): address PR review security and IME findings
- Nginx: add a browser-stream WebSocket location before the generic
/api/threads regex so Live upgrades instead of downgrading to HTTP
(both nginx.conf and nginx.local.conf).
- Ownership: require an existing owned thread for the WS stream and REST
navigate, and tear down the browser session on thread deletion so a
later caller cannot reuse a retained page/cookies by guessing the id.
- SSRF: enforce the URL policy at the browser request boundary via a
context-level route guard covering redirects, popups, iframes, and
subresources (skipped for CDP-attached Chrome).
- IME: skip key forwarding while a composition is active so confirming a
CJK candidate with Enter no longer submits the remote page form.
Adds regression tests for the request guard, session teardown on delete,
and the composing-Enter key decision.
* fix(frontend): smooth streaming in long tool threads
* Revert "fix(frontend): smooth streaming in long tool threads"
This reverts commit f0462516eabe77f138d4027ea1c714fb226683cf.
* fix(browser): address review security and lifecycle findings
- Reject cross-origin WebSocket upgrades on the live browser stream
(Origin allow-list reuse of CORS/same-origin helpers) to close a
WS-CSRF hole, and fail closed when the ownership store is absent.
- Warn when a CDP-attached session runs with the SSRF request guard
off, and drop the unreachable CDP screencast teardown dead code.
- Read browser session launch config from a single canonical source
(browser_navigate) so it is deterministic regardless of call order.
- Bound per-thread Chromium accumulation with idle-timeout eviction
and an LRU max-sessions cap.
- Reset the Live reconnect counter on a successful open so the stream
can't permanently stall after the cumulative attempt cap.
* fix(frontend): reduce long tool thread render stalls
Reuse stable historical message groups during streaming, defer heavy Markdown and browser previews, and lazy-decode message images.
* fix(browser): keep live control responsive during continuous input
Why: Manual browser control felt laggy — a physical click ran the remote
Playwright click three times and each non-move input synchronously awaited a
JPEG screenshot, so events queued behind capture (queue wait up to ~237ms).
The first async attempt used a trailing-edge debounce, which froze the visible
page until a wheel/keyboard gesture stopped ("scroll finishes, then it jumps").
What:
- Frontend forwards one `click` per physical click instead of also emitting
`down`/`up`, so the remote page is not clicked twice per gesture.
- Backend detaches live-frame capture from input dispatch: non-move actions
start a rate-limited background refresh loop (leading frame + bounded cadence)
that keeps emitting frames while input continues and never blocks dispatch.
- Add regression tests: input dispatch no longer awaits the screenshot, rapid
inputs coalesce, and continuous input keeps refreshing before it stops.
Scenarios: Verified in the live Browser panel — a single click completes in
~57ms (was blocked behind a 171ms capture), and a 1.14s sustained wheel gesture
renders ~7 frames throughout the scroll instead of one frame after it ends.
* fix(browser): harden worker and session lifecycle
* fix(browser): address latest review feedback
* fix(frontend): preserve optimistic new-chat message
* test(e2e): preserve mocked message run ids
* fix(browser): address capability review feedback
---------
Co-authored-by: Willem Jiang <willem.jiang@gmail.com>
|
||
|
Daoyuan Li
|
b565e6c0f0
|
fix(workspace-changes): classify a symlink replacing a file distinctly from deleted (#4170)
* fix(workspace-changes): classify a symlink replacing a file distinctly from deleted scan_workspace_roots() skipped every symlinked path entirely (host_file.is_symlink() -> continue), so the path was completely absent from a snapshot instead of being recorded as a metadata-only stub the way binary/large/sensitive-looking files already are. When an agent run replaces a tracked file with a symlink (e.g. rm config.txt && ln -s /some/other/path config.txt), the after-snapshot never contained that path at all, so compare_snapshots()'s _status() saw after_file=None and reported plain "deleted" -- silently hiding that the path is still alive on disk, now as a symlink that can point anywhere on the host, including outside the workspace root. Add a "symlink" classification mirroring the existing binary/large/ sensitive pattern: scan_workspace_roots() now records a symlink as a metadata-only FileSnapshot stub (symlink=True, symlink_target from os.readlink(), lstat'd without ever following the link) instead of omitting it. _status() reports "symlink_created" whenever a symlink newly occupies a path that was not already a symlink (brand new or replacing a prior file), so the security-relevant fact surfaces distinctly instead of collapsing into "deleted". A symlink genuinely removed with nothing replacing it is unchanged: still "deleted". Verified against a real POSIX symlink (WSL; native Windows symlink creation needs elevated privilege) driving the unmodified scan_workspace_roots()/compare_snapshots() functions, and via a patch-file revert/reapply cycle on this same fix to confirm the added regression tests fail before and pass after. * fix(workspace-changes): count symlink_created in the changed-file badge getChangedFileCount summed only created + modified + deleted, so a run whose only change was a symlink replacing a file (reported as the new symlink_created status, not deleted) produced a count of 0 and WorkspaceChangeBadge hid the badge entirely -- the exact scenario this PR targets, with the opposite of the intended result. Add symlink_created to the frontend WorkspaceChangeSummary interface (already emitted by the backend) and include it in the count. The per-file StatusIcon/statusLabel "modified" fallthrough is unaffected and left as a follow-up, per review. Regression test reverts cleanly to reproduce a count of 0 pre-fix. * fix(workspace-changes): rank symlink_created in file sort and complete the type contract sortWorkspaceChanges's statusRank had no entry for the new symlink_created status, so statusRank[left.status] - statusRank[right.status] evaluated to NaN for any comparison involving a symlink-created file, violating Array#sort's ordering contract instead of producing a deterministic order. The frontend WorkspaceChangeStatus and DiffUnavailableReason unions, and the WorkspaceFileChange symlink fields, also stayed narrower than what the backend now emits, so TypeScript's satisfies Record<...> guard on statusRank could not catch the gap. Widen WorkspaceChangeStatus to include "symlink_created" and DiffUnavailableReason to include "symlink", add the matching symlink/symlink_target_before/symlink_target_after fields to WorkspaceFileChange, and give symlink_created a rank alongside modified in statusRank -- restoring the satisfies guard's ability to catch a future unranked status. unavailableLabel now has an explicit "symlink" branch (new symlinkUnavailable i18n string, en-US + zh-CN) instead of falling through to the generic label. Also fixes the failing e2e-tests CI check: the existing workspace-changes.spec.ts mock summary predates the symlink_created field, so getChangedFileCount computed 1 + 1 + 0 + undefined = NaN and the badge rendered "Edited NaN files" instead of "Edited 2 files". Added symlink_created: 0 to the mock to match the real backend contract. New sortWorkspaceChanges unit tests revert cleanly against the unranked statusRank to reproduce the NaN-driven misordering. pnpm test (626 tests), pnpm check, and pnpm format are clean, and the previously-failing e2e spec plus the full e2e suite (94 tests) pass. |
||
|
qin-chenghan
|
9eebc6a9e5
|
fix(config): sync _memory_config with AppConfig auto-reload (#4208)
* fix(config): sync _memory_config with AppConfig auto-reload get_memory_config() now calls get_app_config() before returning the cached _memory_config singleton. get_app_config() auto-reloads when the config file signature changes, and its _apply_singleton_configs() calls load_memory_config_from_dict() to populate _memory_config. Without this side effect, changing memory.mode in config.yaml (e.g. from "middleware" to "tool") would never take effect at runtime because _memory_config retained the stale value. Only call get_app_config() when _app_config has already been loaded to avoid picking up a config file as a side effect of the first access to get_memory_config(), which would break callers that expect module-level defaults (e.g. unit tests). Catch FileNotFoundError for environments without a config file. Closes #4204. * fix(memory): broaden config-reload guard and add isolated-path tests Addresses two review findings from willem-bd: 1. Broken-config crash risk: broaden the except from FileNotFoundError to Exception so transiently broken config.yaml (YAMLError, ValidationError, ValueError) falls back to the last-good singleton instead of crashing per-turn hot paths (memory_middleware, summarization_hook, lead_agent/prompt.py). 2. Missing regression test: add test_get_memory_config_self_syncs_... (calls get_memory_config() after file mutation with no intervening get_app_config() call) and test_get_memory_config_falls_back_on_... (verifies broken config does not crash, returns cached value). |
||
|
ChenglongZ
|
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 Song
|
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 Jiang
|
1073393e07
|
Revert "fix(memory): follow config reload in get_memory_config (#4216)" (#4320)
This reverts commit cd0f1e2212b0e910a5a3bd5f5e6f060b5bfe5cfa. |
||
|
Yufeng He
|
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> |
||
|
hataa
|
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 |
||
|
Aari
|
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 90936b49 said `_held_teardown_lease` wrapped "both" `_backend.destroy()` call sites. There are three. `_drop_unhealthy_sandbox` marks `del:` and then blocks on the same unbounded stop, and it untracks *before* claiming, so `_renew_owned_leases` cannot see the id either — nothing refreshed the marker. Reproduced against a real redis: the peer's `take()` succeeds 1.0s into a 2.5s stop. Same window, third path. That miss came from the habit the rest of this commit addresses: a property asserted in prose, with no test that could falsify it. Auditing every load-bearing claim in this feature — AGENTS.md, the store docstrings, the provider's design comments — against the test that would go red turned up several more, each verified by mutating the code and watching the suite stay green. Tests that could not fail: - `test_reconcile_fails_closed_when_ownership_unknown` reached the grace gate, not the claim. A bare MagicMock answers `owner()` with a truthy mock, so the container read as peer-owned and deferred; `claim()` was never called. It stayed green with `_claim_ownership` failing open. Adding the grace ahead of the claim is what hollowed it out — inserting a gate can silently disarm the tests for the gate behind it. - `test_adoption_grace_restarts_when_a_live_owner_republishes` never distinguished reset from pause. Those diverge only on a *second* lapse, which it never drove, so it passed with the reset deleted. Claims with no test at all, each now pinned (mutation → red, per test): - `destroy()`, `_evict_oldest_warm`, `_reclaim_warm_pool_sandbox`, `_register_created_sandbox` and `shutdown()`'s warm loop were each the one untested sibling of an "every path does X" enumeration. `shutdown()` was never driven with a non-empty warm pool, so a loop bypassing the ownership claim — stopping a live peer's container on our exit — went unnoticed. - Renewal's unknown-is-not-lost rule, the single deliberate exception to fail-closed. Inverting it drops every active and warm sandbox on every instance the moment the store blinks. - Both hops of the stream-bridge redis inference. Deleting either left the suite green while every config.yaml-native multi-instance deployment silently fell back to memory — #4206 reopened on exactly the deployments the inference exists for. Claims narrowed instead, because they promised more than the code delivers: - "run against both backends ... cannot drift" — CI provisions no redis, so the merge gate runs the memory tier only and the Lua never executes there. - "Every destroy path claims before untracking" — `_drop_unhealthy_sandbox` untracks first, deliberately, under its `expected_info` TOCTOU guard. - "Atomic: concurrent claims from different instances cannot both succeed" — true via Lua on redis, vacuous on the single-instance memory store, and pinned by neither, since the contract suite drives sequential calls. A concurrency test against the memory store would make the claim look covered while the mechanism that carries it still never runs in CI. * fix(sandbox): release the teardown marker when a destroy() stop fails The three `del:`-marked stop paths disagreed on failure. `_destroy_warm_entry` releases on both outcomes and says why: the stop failed, so the container is probably still up, and a marker left behind refuses its own thread's `take()` until the TTL lapses. `_drop_unhealthy_sandbox` does the same. `destroy()` had no such guard — a raising backend propagated straight past `_release_ownership`, and the thread could not re-acquire for a full TTL. Fails safe rather than fatal: a stuck marker stops peers from touching the container, it is not the cross-instance kill. But the paths must agree, and this one is the odd one out. Release, then re-raise. Swallowing would be the easier symmetry with `_destroy_warm_entry`'s `return False`, but `destroy()` has no failure return and `shutdown()` logs per sandbox off the exception, so swallowing would silently narrow what callers can see. Found by comparing the three paths after @fancyboi999 asked for release to be handled "consistently with the other destroy paths" on the unhealthy path — which 0d2377b2 already does. This is the sibling that wasn't. * fix(deploy): bump chart config_version to 27 for sandbox.ownership config.example.yaml went to 27 with the new sandbox.ownership section, but the chart embeds its own copy and stayed at 26, so validate-chart failed. A bare bump: the chart already sets stream_bridge.type=redis, which is what resolve_ownership_config infers a redis ownership store from, so no field change is needed. * fix(sandbox): release the teardown lease from its heartbeat, not the caller `_held_teardown_lease` joined its heartbeat only briefly and the caller cleared the `del:` marker right after the stop. A refresh `claim` still in flight (`RedisOwnershipStore` had no socket timeout, so a round trip could block) could land *after* that release and rewrite `del:` on a container whose stop had already completed — refusing a fresh `take()` (or rolling back a fresh create) until the TTL. Move the release into the heartbeat's own `finally`, after its loop stops, so no refresh can run after it. The three destroy paths no longer release after the `with` (`destroy()`'s no-container branch still does, since no lease was held there). Bound every store round trip with a socket timeout so the in-flight refresh — and thus the deferred release — stays finite, and broaden the heartbeat's `except` so an unexpected error cannot strand the marker during a long stop. Also fold in the review follow-ups: stop re-resolving an already-resolved ownership config in the factory, document the Redis-outage-vs-TTL boundary in config.example.yaml, and add a tests/blocking_io anchor pinning that `release()`'s store round trip stays off the event loop. * fix(sandbox): refuse a non-destroy claim that would unwind our own teardown `claim(for_destroy=False)` against our own `del:` lease fell through and overwrote it with `own:`, cancelling a teardown that was already in flight. The container stop cannot be recalled, so downgrading the marker would let a `take()` hand out a container that is about to die -- #4206, self-inflicted. No caller does this today: the two non-destroy callers run against an absent key (the LAPSED re-claim) or an unowned one (post-grace reconcile). The contract has to forbid it rather than rely on that staying true. Fixed in both backends. The redis rule lives in Lua and the memory rule in Python, so fixing one only would let them drift silently -- and the shared contract suite is what is supposed to catch that drift, so it now covers this. Also adds a contention test for `claim`. The suite drove sequential calls only, so it pinned the exclusion predicate but not the atomicity that predicate depends on; eight instances now race for one container and exactly one must win. * fix(sandbox): bound the container stop so it cannot outlive its teardown lease `_stop_container` passed no `timeout` to `subprocess.run`, so a wedged container runtime blocks it forever. The `del:` marker is what keeps a peer from re-acquiring the container while the stop runs, but a marker is a lease and a lease can lapse: a store outage longer than the TTL frees it, a peer's `take()` succeeds against the still-running container, and the stop then lands on the turn that was just handed it -- the exact #4206 failure. The teardown heartbeat already covers the case where the store stays reachable. This bounds the worst case independently of the ownership layer, which is the point: it holds even when the ownership layer is the thing that failed. A timeout is not swallowed like a `CalledProcessError`. That error means the runtime answered "I could not stop it"; a timeout means we do not know, and the container is probably still running -- returning normally would let `_destroy_warm_entry` report a clean stop and drop the warm entry, leaking a running container nothing tracks. * fix(sandbox): exclude this instance's own reapers from its acquire path An ownership lease excludes peers and nothing else. `claim()` and `take()` both succeed against our own `own:` lease by design -- that is what lets a destroy path claim what it already owns -- so `del:` says nothing to this process's other threads. Meanwhile every reaper decides outside `_lock`, because a store round trip must not be held under the lock that guards every acquire. So each reaper acts on a decision its own acquire path may already have invalidated, and the store cannot see the difference. Six paths end in an irreversible act (a container stop, or closing a host-side client) on a decision made outside the lock. All six reproduce: _evict_oldest_warm re-checks warm membership, then releases the lock _reap_expired_warm no re-check at all _cleanup_idle_sandboxes re-verifies idle, then releases the lock _renew_owned_leases acts on a stale renew() -> LOST release() same staleness on its own refresh _drop_unhealthy_sandbox untracks before claiming, opening discovery Both warm reapers are a regression from the deferred pop this branch introduced: `WarmPoolLifecycleMixin` popped under the lock, so a reclaim's membership check failed and the race could not occur. Deferring the pop is still right (popping first loses the container on a refused claim), so the exclusion has to be made explicit instead. The idle path is pre-existing in shape, but this branch widened it from a few instructions to a network round trip by claiming ownership before untracking. Two guards, because the two directions want opposite answers: Reaping -- nothing may promote it. The reaper reserves the id, and every promote path refuses a reserved id exactly as it refuses a peer's `del:` (drop and cold-start). The "is this still reapable?" test travels with the reservation as a predicate and runs in the same critical section, because checking first and reserving second is the window, not a narrower version of it. Forgetting -- the peer legitimately wins, so the promote is what to detect. `_publish_ownership` bumps a per-id acquire epoch; the callers that decide from a store round trip snapshot it first, and the pop is skipped if it moved. Object identity cannot substitute: the reuse path re-publishes ownership while handing out the same tracked `AioSandbox`, so an identity check sees nothing and the pop closes a client mid-turn. `still_reapable` is required rather than defaulting to unconditional -- the safe default is the one that makes a new call site think about it. That diverges from the mixin hook, which is safe because this provider overrides both mixin callers, and loud rather than silent if those are ever dropped. Also closes a client leak on the discover path: "nothing to roll back" was true of the container but not of the HTTP client constructed before the publish, which the sibling create path already closes. The shared-store test view rebound `owner_id` outside the store's lock, so a concurrent claim could execute under the wrong id and read its own lease as a peer's. Serialized, so the heartbeat-hold tests stop flaking. * fix(sandbox): mark acquire intent before the ownership round trip A guard must become visible no later than the transition it guards. The acquire epoch cannot manage that for `take()`: the takeover is durable before `take()` returns -- redis has committed the SET while the reply is still in flight -- and the epoch can only be written afterwards. In that interval the store already says the container is ours while the epoch still reads as it did when a renewal decided `LOST`, so the stale forget walks through, drops the maps and closes the client the acquire is about to hand back. Acquire then returns an id the provider no longer tracks and `get()` answers `None` for the rest of the turn. `_publish_ownership` now publishes an intent mark under `_lock` before the round trip; the epoch keeps covering the other half, "an acquire completed since you decided". `_forget_lost_sandbox` honours the intent mark unconditionally rather than only when an epoch is supplied -- today's epoch-less callers cannot reach the window, but "no epoch" reading as "no guard" is how the next caller of a dangerous primitive gets written. The same invariant had four more instances, all reproduced: reuse returns a decision the forget already invalidated -- before the mark is set a `LOST` is both current and correct, so the forget legitimately runs and the entry reuse decided to hand out is gone. Re-check after publishing and fall through to discovery instead. reclaim installs an entry a reaper reserved after its check -- the warm entry is still visible during the stop, and the reaper's claim succeeds because reclaim's own take() just made the lease ours. Re-check likewise. the reservation was released before the entry was removed -- the pop belonged to the caller, leaving a gap where the container is stopped, the entry is still in `_warm_pool`, and nothing marks it. `_destroy_warm_entry` removes it itself, inside the reservation; the pop stays deferred relative to the stop, just not to the reservation. reconcile adopts a container this instance is tearing down -- adoption is a promote and needs the same reservation check as the others. Neither existing guard excludes it: the claim succeeds because the lease is ours, and on `memory` the recovery grace is skipped outright. The pre-round-trip checks in reuse and reclaim are kept as early-outs, since they skip a health check and a store round trip on a doomed entry, and are pinned to that job rather than to a correctness role they no longer hold. The teardown reservation predicate runs under `_lock`, so it must not touch the lock. Documented rather than engineered around: making the lock reentrant to tolerate it would trade a loud hang for a quiet class of re-entrancy bugs across the rest of the provider. * fix(sandbox): honor local teardown after ownership publish * fix(sandbox): clear a stale warm entry when an id becomes active Active and warm are exclusive states, and the two register paths were the only place that could hold both: they inserted into `_sandboxes` without popping `_warm_pool`, so one container ended up with two reapers. `_reap_expired_warm` judges an entry by its warm timestamp and never consults `_last_activity`, so it stops a container an agent is actively using while `_sandboxes` still hands out its client. Reachable because `_reconcile_orphans` adopts an untracked-but-running container into the warm pool inside the register's publish -> track window, and on the `memory` store it adopts on sight: `_adoptable_after_grace` short-circuits when `supports_cross_process` is False, so an id carrying this process's own lease reads as adoptable. That window is new to this branch -- on main the track was a single locked insert with nothing before it. Both register paths now pop the warm entry inside the same locked section that installs the active one. * fix(sandbox): harden ownership renewal teardown --------- Co-authored-by: Willem Jiang <willem.jiang@gmail.com> |
||
|
Matt Van Horn
|
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> |
||
|
VectorPeak
|
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 Jiang
|
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 acfc7617): P1 - the generation guard measured construction order, not config freshness, so a stale AppConfig(cap=3) constructed after a fresher AppConfig(cap=1) could restore the higher cap; and on a downscale 3->1 release() handed excess permits to queued waiters, keeping in_flight pegged at the old cap. Replace the pseudo-generation path with a startup-only cap: the first middleware __init__ resolves and freezes the cap; later instances (newer or older config) are no-ops. No runtime cap mutation means no downscale race and no freshness/construction-order race. Per-call gate is now `limiter is None` only, so a reloaded instance with max_concurrent_calls=0 cannot silently drop the frozen cap. Removes _owner_seq / set_limit_if_newer / _grant_to_queued_locked / _next_instance_seq; file 946 -> 926 lines. P2 - burst-rate calls are capped at 2 attempts but the retry log line, the llm_retry stream event max_attempts, and the user-facing message still used self.retry_max_attempts (3), so the frontend showed 1/3 then stopped after attempt 2. Thread the effective max_attempts (_max_attempts_for) through the logger, _emit_retry_event, and _build_retry_message. Also: document max_concurrent_calls as startup-only in the config field description and config.example.yaml (prose only - the startup-only: prefix is top-level AppConfig-field granularity and would mislabel the otherwise hot-reloadable llm_call section / break the reload_boundary drift test). Rewrite the cap-mutation tests for startup-only semantics; add P2 retry-budget event tests (sync+async, teeth-verified red on the bug); fix bot nits (empty except blocks -> gather(return_exceptions=True); bare await statements -> assigned+asserted). Co-Authored-By: Claude <noreply@anthropic.com> --------- Co-authored-by: Claude <noreply@anthropic.com> Co-authored-by: Copilot Autofix powered by AI <223894421+github-code-quality[bot]@users.noreply.github.com> |
||
|
Aari
|
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_Feng
|
24a45a4e68
|
feat(tui): add clear command (#4306) | ||
|
Daoyuan Li
|
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. |
||
|
Aari
|
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 |
||
|
Aari
|
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.
|
||
|
Aari
|
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> |
||
|
lllyfff
|
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> |
||
|
Aari
|
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 Li
|
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 Li
|
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 Li
|
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. |
||
|
Hanchen Qiu
|
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 jiyin
|
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 |
||
|
Aari
|
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. |
||
|
Andrew Chen
|
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>
|
||
|
Andrew Chen
|
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 jiyin
|
90d511f3d2
|
refactor(sandbox): consolidate E2B client lifecycle helpers (#4262)
* refactor(sandbox): consolidate E2B client lifecycle * test(sandbox): cover E2B lifecycle cleanup |
||
|
yym36991
|
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. |
||
|
Aari
|
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. |
||
|
Huixin615
|
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 Li
|
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. |