* 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.
* Add Monocle behavioral test suite
Trace-based tests under tests/monocle/ asserting against the agent's Monocle execution traces: 4 offline tests load a recorded trace by file (with_trace_source), one per curated question, plus 1 live end-to-end test. Fluent structural asserts (agent, tools, input/output, token/duration budget); additive only, no app-code changes.
* Address review: restructure suite, move under backend/tests/monocle/
Responds to the review on this PR.
Behavioural coverage is now the live tests, not frozen fixtures. Keep one
offline test as a worked example of the fluent assertion API (loads a recorded
trace by file, no keys), and add two live tests that drive the agent end-to-end
through DeerFlowClient and assert on the trace the real run emits (web-research
and sandbox paths). The live tests skip without OPENAI_API_KEY or the app.
Other review fixes:
- Move the suite under backend/tests/monocle/ so backend pytest collects it.
- Split helpers into _helpers.py; conftest is fixtures-only (run_agent), with
Monocle setup owned by the validator and .env load scoped to the live path.
- Resolve the model from config.yaml instead of hardcoding gpt-4o; skip the
live tests when config.yaml is absent.
- Keep one trace with a stable name (web_research_ev_battery.json); drop the
other three (removes ~2,200 lines of fixture blobs).
- Drop the flaky wall-clock duration bound on live runs.
- Keep monocle_test_tools in a standalone requirements.txt rather than the
backend dev group: it hard-depends on the ML eval stack (torch, transformers,
sentence-transformers, ~48 packages, +950 lines in uv.lock), so isolating it
keeps the app's locked deps clean. importorskip skips the suite when absent.
* docs(monocle tests): explain the golden-trace workflow
Add a "How this is meant to be used" section: capture a run you are happy with
as a golden, labelled trace, turn it into assertions (the offline example), then
point the same assertions at the live agent so every later run has to reproduce
that behaviour.
* Address review: fix docstring pytest paths, state the suite is not run in CI
The docstring commands now use the backend/tests/monocle/ form (matching
the README, which also gains the backend-dir uv variant), and the README
states explicitly that the suite is skipped in CI and run on demand.
* Address review: document the committed trace, pin assertions context
- README: new section on the committed trace. It is a full, unmodified
real-run recording (system prompt of the recording date + fetched web
content, no credentials), committed whole so the offline example parses a
genuine trace. The offline assertions are pinned to this trace and the
monocle_apptrace 0.8.8 span shapes; re-record when prompt, tools, or model
change.
- README: note that the monocle_trace_asserter fixture comes from
monocle_test_tools' auto-registered pytest plugin (pytest11 entry point).
- test_deerflow.py: comment why web_fetch asserts min_count=2 rather than the
recorded exact count of 5 (fetch counts vary run to run; keep it a floor).
- requirements.txt: loose pin python-dotenv>=1.0.
* Address review: make live tests explicit opt-in, drop OpenAI-only gate
Two execution-gating fixes from review:
- Live tests are now opt-in via MONOCLE_LIVE_TESTS=1 (default off). Previously
the documented offline command collected the live tests too, and on a
configured checkout (.env + config.yaml present) they would run for real,
spending model tokens and hitting the network. Now the plain
`pytest backend/tests/monocle/` run cannot go live regardless of what
credentials are present; test_live_gate_defaults_off pins the gate.
- The run_agent fixture no longer requires OPENAI_API_KEY. config.yaml resolves
the model, which may be any provider (Anthropic, Gemini, Volcengine, ...), so
a hard-coded OpenAI gate skipped valid configurations and passed invalid
ones. Credentials are validated by the configured model itself.
README and docstrings updated to match: offline command is offline by
construction, live is MONOCLE_LIVE_TESTS=1, credentials described as the
configured model's rather than OpenAI's.
Verified both modes: default run is 2 passed 2 skipped with no network; opted
in, all 4 pass with real end-to-end runs.
---------
Co-authored-by: Willem Jiang <willem.jiang@gmail.com>
* 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.
`_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>
`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>
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.
* fix(scripts): broaden support bundle secret-key redaction denylist
SECRET_KEY_RE only matched a fixed keyword allowlist, so a secret stored
under an unanticipated key name inside an open-ended config dict (e.g.
guardrails.provider.config, an arbitrary provider-kwargs dict) was emitted
verbatim into config-summary.json even though manifest.json claims
redacted_secret_fields=true. This gap was flagged on PR #3886's review
before merge but not fully addressed.
Broaden the key-name match to mirror env_policy.py's wildcard denylist
(*KEY*/*SECRET*/*TOKEN*/*PASS*/*CREDENTIAL*/*DSN*) already used for sandbox
env-scrubbing, plus its no-flag credential exact names (GH_PAT/GITHUB_PAT/
REDIS_AUTH/REDISCLI_AUTH/PGSERVICEFILE). The new bare key/pass/dsn
alternatives are boundary-guarded so they match only their own delimited
token, not an unrelated word that starts with the same letters (routing
"keywords", guardrails "passport").
* fix(scripts): stop the pass token boundary from missing passphrase/passcode
SECRET_KEY_RE's bare "pass" alternative, (?<![a-zA-Z])pass(?![a-zA-Z]),
excludes any key where "pass" is followed by another letter. That
correctly keeps "passport" out of the redaction set, but it also
excludes genuine secret-bearing key names like "passphrase" and
"passcode" -- both of which env_policy.py's *PASS* substring denylist
does catch, so a secret stored under either name in an open-ended
config dict (e.g. guardrails.provider.config) would still leak into
config-summary.json.
Narrow the lookahead to only exclude a trailing "port" -- pass(?!port)
-- so passphrase/passcode/pass/db_pass all match while passport stays
excluded; compass/bypass stay excluded via the existing leading-letter
lookbehind, independent of the lookahead.
Added a regression test covering both the newly-caught names and the
still-excluded ones in one place. Reverting to the old lookahead
reproduces the exact leak (passphrase left unredacted); with the fix,
tests/test_support_bundle.py (30 tests) is green, and ruff check/format
are clean.
* fix(artifacts): serve inline binary artifacts via FileResponse for Range support
Audio/video/image artifacts that are previewed inline (not downloaded, not
active content) were served by reading the whole file into memory and
returning it through a plain Response. That response never sets
Accept-Ranges and ignores any Range header the browser sends, so seeking
an <audio>/<video> element backed by this endpoint always gets the full
200 response back instead of a 206 partial response for the requested byte
range -- which is why dragging the seek bar on an audio artifact restarts
playback from the beginning instead of jumping to the new position.
Route this case through FileResponse instead (as the active-content/
download branch already does), which handles Range/If-Range natively.
Verified with a real Range request against the endpoint: initial load now
reports Accept-Ranges: bytes, and a ranged GET returns 206 with the correct
Content-Range and only the requested slice of bytes.
Fixes#3240
* fix(artifacts): address review nits on the inline FileResponse branch
Two non-blocking nits from review:
- Drop the redundant filename= kwarg on the inline_file FileResponse
call. Content-Disposition is already set explicitly on this branch,
and FileResponse only uses filename to setdefault that same header
-- which is a no-op once it's already present. Harmless today, but
removes the latent risk that a future Starlette version turning that
setdefault into a hard set would silently flip inline preview to
attachment.
- Reword the blocking-IO regression-anchor docstring: it claimed
awaiting get_artifact for a binary artifact does "zero filesystem
IO", but _read_artifact_payload still runs exists/is_file/
mimetypes.guess_type/is_text_file_by_content (an 8 KB read) for
binary files too, just offloaded via asyncio.to_thread like the text
branch. The gate has nothing to catch because that IO is off the
event loop, not because there's none -- reworded to say no full-file
read happens, matching the accurate framing already used one
paragraph up.
tests/test_artifacts_router.py, tests/blocking_io/test_artifacts_router.py
(19 tests) are green, and ruff check/format are clean on both changed
files.
nginx's default client_max_body_size (1m) and proxy_request_buffering (on)
were never overridden for the chat/LangGraph route, only for the upload
route. A long pasted prompt either exceeded the default size limit (413) or
got spooled to a temp file before reaching Gateway, which can fail with a
raw nginx 500 on a non-root local run if that temp directory isn't
writable -- reproduced independently (real nginx 1.28.3, byte-identical
error page and matching error-log Permission denied line) while confirming
fancyboi999's diagnosis in issue #3952.
Mirrors the upload route's existing client_max_body_size / proxy_request_
buffering fix onto /api/langgraph/ in all three places this nginx config is
maintained (Docker prod, local make dev, and the Kubernetes/Helm ConfigMap),
sized smaller (20M) since this route only ever carries JSON chat text, never
binary file uploads.
* 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.
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.
* fix(middleware): recover malformed tool-call ids in dangling repair
DanglingToolCallMiddleware normalizes malformed tool-call names (#4008) and
arguments (#4193) so strict OpenAI-compatible providers do not reject the next
request. The id is the third field of that same recovery contract and was left
alone.
A provider that emits an empty id -- or omits it -- parses into a well-formed
tool_calls entry, so it reaches the middleware through the normal path. The
empty id never enters the pairing set, so the orphan pass drops the call's
already-produced ToolMessage and the placeholder pass skips the call. The
request then goes out carrying an empty id and with the real tool result gone.
Normalize ids up front and re-point each already-paired ToolMessage at its
call's new id, so the existing pairing/orphan/placeholder logic no longer sees
a malformed id. Only the view that is actually read and serialized is
relabelled; a valid id is left byte-for-byte alone, since it is matched
verbatim against ToolMessage.tool_call_id.
* fix(middleware): scope malformed-id result pairing to its own turn
Malformed tool-call ids are all equally empty, so pairing recovered results by
the original id alone was a global FIFO over the whole transcript. An earlier
dangling call then consumed a later turn's result: the real result was served
to the wrong call while the call that actually ran got the interrupted
placeholder. An orphan result whose originating AIMessage was already gone
could likewise be adopted by a later malformed call, resurrecting it instead of
being dropped as the orphan pass intends.
Walk the messages once in document order so only the most recent AIMessage's
unanswered calls are claimable, which keeps a result answering the turn that
issued it, and rule out the wrong parallel sibling within a turn by tool name.
A result whose name matches no open call is left malformed for the existing
orphan pass to drop rather than repurposed as some other call's answer.
* fix(middleware): keep the shadowed raw view out of id recovery
* fix(middleware): only claim a malformed result when the pairing is forced
* docs(middleware): cite ToolNode's ordering guarantee for positional pairing
_poll_loop persisted the long-poll cursor (get_updates_buf) for the whole
getupdates batch immediately on receipt, then iterated data["msgs"] with no
per-message error handling. If any single message's processing raised (e.g.
an attachment that fails to decrypt), the for loop aborted and every message
after the failing one in that batch was never handled -- but since the cursor
had already advanced past the whole batch, the next poll would never re-fetch
them. The loss was silent and permanent, not a delay.
Fix is two parts:
- Wrap each _handle_update call in its own try/except (re-raising
CancelledError) so one bad message is logged and skipped instead of
aborting the rest of the batch.
- Move the cursor advance/persist to after the per-message loop instead of
before it, so a hard crash mid-batch leaves the cursor unmoved. Worst case
becomes re-fetching and re-processing the batch on restart, not silently
skipping messages that were never actually handled.
Regression test drives the real _handle_update over a 3-message batch where
the middle message is a WeChat image item with deliberately non-block-aligned
"encrypted" bytes, so decryption raises a genuine cryptography ValueError
(the same failure shape a corrupt real attachment would produce). Confirms
the first and third messages both still reach the bus, the failure is logged
with the message id, and the persisted cursor reflects the fully-attempted
batch.
_handle_builtin's "quit" branch called self.exit() unconditionally,
unlike action_interrupt (Ctrl+C), which checks self._streaming and
interrupts before any teardown. During an active run, /quit tore the
app down while the worker thread was still live; the next
call_from_thread call from that orphaned thread then failed silently
(the app's loop is gone), quietly abandoning the in-flight turn and any
post-run persistence such as the thread title.
Mirror action_interrupt's check: if self._streaming, run the same
interrupt/cleanup path before calling self.exit(). /quit still always
exits -- it now just does so safely.
Adds a regression test using a fake client that blocks mid-stream (a
real worker thread genuinely stuck, not a hand-flipped flag) to catch
/quit during a run, plus a Ctrl+C contrast test under the same setup.
`extract_connect_code` matched `command in {"/connect", "connect"}`, so any
message whose first word is the ordinary English word "connect" was parsed as
`/connect <code>` and its next word swallowed as a one-time bind code. For
example `extract_connect_code("connect the database to the api")` returned
`"the"` instead of `None`, meaning a normal chat message never reached the
agent and instead attempted a channel bind.
Every other channel control command requires a leading slash:
`is_known_channel_command` only matches `/`-prefixed tokens and
`KNOWN_CHANNEL_COMMANDS` holds only slash forms (`/goal`, `/new`, ...). The
docstrings and AGENTS.md advertise only `/connect <code>`. The bare alias was
inconsistent with all of that and produced the false-positive bind.
Match only `/connect`. The `.lower()` keeps it case-insensitive (`/Connect`)
and the mention-prefix handling from #4222 is unaffected, so
`@bot /connect <code>` still binds.
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* Improve tool output preview synopsis
* Add JSON path anchors to tool output synopsis
* Fix JSON synopsis line anchors
* fix(synopsis): tighten detectors and fix CSV first-row join
Address review feedback from @willem-bd on PR #3377.
Detectors:
- _looks_yaml now requires >=3 key-shaped lines and refuses bare
uppercase-tag lines ('INFO: ...', 'ERROR: ...') that look like log
lines and would round-trip into a flat string dict via safe_load.
Previously a 200-line log file was classified as 'YAML object with
3 top-level keys' and lost every line, count, and middle signal.
- _try_yaml refuses payloads that safe_load collapses to a dict of
all strings (the shape tracebacks and log lines collapse into).
- _try_table applies the header-must-look-like-identifiers and
minimum-row-count guards only to TSV, since the same safeguards
would reject legitimate small CSVs. Refuses tab-indented bash
output, ls -l listings, and tree dumps.
Rendering:
- CSV first data row is now rendered as a key=value list joined by
' | ' (e.g. 'name=Ada | description="a fine, brilliant logician"
| score=98'). The previous delimiter.join(rows[1]) silently
re-split cells that contained the delimiter inside a quoted cell,
which made the synopsis report a 3-column table as 5 columns and
misled the model about column count and content.
Text summary:
- _summarize_text now omits the closing excerpt entirely when the
input is shorter than 2 * _TEXT_EXCERPT_CHARS, since the previous
opener/closer slices overlapped and duplicated text for short
inputs (build_tool_output_synopsis is reachable directly from
tests and other callers that pass small inputs).
Tests:
- Update test_table_preview_extracts_columns to assert the new
key=value list format.
* fix(synopsis): drop JSON path line/byte offset hints
The path-location hint was computed by string-searching for the
quoted key in the original content and reporting its byte offset and
line number. This anchors at the first textual occurrence of the
key string, which is wrong when the key also appears as a value
earlier in the document, or when the same key recurs at multiple
depths. With nested paths the anchor drifts further on every step
because the search cursor is advanced past each previous match.
Concrete cases:
content = '{"label": "items", "items": {"id": 1}}'
_json_path_location(content, ['items'])
-> ' (line 1, byte offset 10)' # the value, not the key
content = '{"data": {"info": 1, "data": {"info": 2}}}'
_json_path_location(content, ['data','data','info'])
-> ' (line 1, byte offset 30)' # the inner first 'info', not the second
The synopsis instructed the model to 'Start near the line hints
above when present', so a wrong anchor would send read_file into
the wrong region of the persisted .tool-results file.
Drop the hint entirely. The path itself ('$.data.items') is
already useful navigation; the agent uses read_file with start_line
based on its own judgement of where the relevant slice is.
Tests:
- Update test_json_preview_reports_nested_paths to assert no 'line '
or 'byte offset ' appears in the body before the Access section.
- Rename test_json_line_hints_use_original_content_offsets to
test_json_paths_are_emitted_without_line_hints and invert the
assertions to check the hints are absent.
* fix(synopsis): bound _scalar_examples recursion depth
Mirror the _JSON_STRUCTURE_DEPTH cap used by _json_container_paths
and _json_shape so that deeply nested JSON cannot trigger
RecursionError inside build_tool_output_synopsis.
In ToolOutputBudgetMiddleware.awrap_tool_call the synopsis is built
inside asyncio.to_thread(_patch_result, ...); a RecursionError
would surface as a tool-call failure and the user would lose the
entire output. 300-level nested JSON is well inside what an
attacker-controlled MCP tool, a JSON-RPC-over-JSON-RPC chain, or a
buggy serializer can produce.
* feat(synopsis): restore inline raw head/tail sample
The synopsis-only preview silently dropped the raw head/tail bytes
that preview_head_chars / preview_tail_chars used to inline. For
text/code/log outputs the agent lost first/last KB of the actual
content and had to issue a follow-up read_file round-trip to see
the trailing region (last paragraph of a fetched article, final
error line in a traceback, closing diagnostics of a bash run).
Restore an inline 'Raw sample (head + tail)' section in the preview.
The section is composed by slicing head_chars from the start and
tail_chars from the end of the content (with a '...' separator
between them, and the tail suppressed when it would overlap the
head). For binary-like output, the synopsis's own sample is reused
unchanged.
This makes preview_head_chars / preview_tail_chars operational
again for every kind except binary, which already had a sample
channel.
Tests:
- Rename test_json_preview_extracts_structure_instead_of_head_tail
to test_json_preview_includes_structure_and_raw_sample and assert
the raw sample section is present and the payload is reachable
in the head slice.
* test(synopsis): add regression tests for willem-bd review findings
Add 8 regression tests under TestToolOutputSynopsis, one per
finding in @willem-bd's review of PR #3377:
- test_review_5_log_lines_are_not_misclassified_as_yaml
Pins the YAML detector to refuse 'LEVEL: message' log lines.
- test_review_6_json_paths_are_emitted_without_byte_offset
Pins the removal of byte/line hint from JSON path descriptions.
- test_review_7_scalar_examples_respects_depth_cap
Pins that 500-deep nested JSON does not raise.
- test_review_8_csv_first_row_quoted_cells_round_trip
Pins the new key=value list format for CSV first-row rendering
and asserts that quoted cells with embedded delimiters survive.
- test_review_9_tsv_detector_rejects_tab_indented_bash
Pins that tab-indented bash output is not classified as TSV.
- test_review_10_preview_includes_raw_head_and_tail_sample
Pins the restored inline 'Raw sample (head + tail)' section.
- test_review_11_short_text_does_not_duplicate_excerpts
Pins that closer is suppressed for inputs shorter than
2 * _TEXT_EXCERPT_CHARS.
- test_review_12_preview_head_tail_chars_are_operational
Pins that head_chars / tail_chars are wired into the rendered
preview and not silently dropped.
Also removes the now-stale 'byte offsets are approximate anchors'
sentence from render_tool_output_preview's Access block; the
synopsis no longer emits byte/line hints, so the guidance to
'start near the line hints' was misleading.
* fix(synopsis): resolve lint errors on tool output budget tests
Local 'make lint' on feat/tool-output-synopsis-preview (after fast-forward
to current main) failed with three errors in tests added by PR #3377:
- E501: 307-char bash_out literal in test_review_9_tsv_detector_rejects_tab_indented_bash
- E741: ambiguous single-letter 'l' in test_review_11_short_text_does_not_duplicate_excerpts
- E741: same ambiguous 'l' on the closing assert
Replace the long literal with a join of per-row entries, rename the loop
variable from 'l' to 'ln', and run ruff format on the two touched files
to absorb the formatting drift introduced by the merge with main.
Verification:
- make lint -> All checks passed; 643 files already formatted
- pytest tests/test_tool_output_budget_middleware.py -> 110 passed
* fix: address willem-bd review findings (code/csv misclassification, text duplication, line snapping, dead constant, xml hardening, depth consistency)
- _CODE_HINTS: require stronger signals for use/fn (trailing ; or parenthesised)
- _try_table: apply _TABLE_MIN_DATA_ROWS gate to CSV too (not just TSV)
- config.example.yaml: correct misleading comment about preview_head/tail_chars
- _summarize_text: skip opener/closer excerpts when raw sample will be appended
- _build_raw_sample: snap to line boundaries for clean truncation
- Remove dead constant _TABLE_FIRST_ROW_CHARS
- Prefer defusedxml for XML parsing (billion-laughs protection), fallback to stdlib
- Replace _json_shape magic number 2 with named _JSON_SHAPE_MAX_DEPTH constant
- Update tests to match new CSV gate (>=5 rows) and line-snapped sample counts
* style: ruff format fix for tool_output_synopsis.py and test_tool_output_budget_middleware.py
* fix(tool-output): address 4 review comments - DoS hardening + size cap
1. XML entity-expansion DoS: skip _try_xml when defusedxml is not
available (SafeET is None), falling through to text + raw sample.
(cid=3587721336)
2. YAML alias-bomb DoS: refuse to parse YAML content > 500 KB.
(cid=3587721340)
3. Unbounded content parse: add _MAX_SYNOPSIS_INPUT_BYTES=5MB cap;
oversized output falls back to raw head/tail sample instead of full
parse. (cid=3587721346)
4. Scalar examples surface mid-document values: add docstring note
that the synopsis is a structural summary, not a confidentiality
filter. (cid=3587721353)
* fix(tool-output): ruff format the synopsis string to one line
---------
Co-authored-by: qinchenghan <qinchenghan@huawei.com>
* fix(agents): refuse empty SOUL.md updates in update_agent
setup_agent already rejects empty/whitespace soul (#3553). update_agent
is the sibling write path and previously reported success while wiping
a working SOUL.md. Mirror the same guard before staging.
* fix(agents): guide the retry in the empty-SOUL update rejection
Append "Omit the soul field if you do not want to change it." to the
empty-soul error so the model self-corrects in one step instead of
retrying with another null-like value, matching the "No fields provided"
sibling message's helpfulness. Both regression tests assert the guidance.
* fix(helm): default sandbox Services to ClusterIP (#3929)
The K8s sandbox provisioner supports both NodePort and ClusterIP via
SANDBOX_SERVICE_TYPE (added in #4016), but the Helm chart never set it,
so real-cluster installs inherited the NodePort default. That bound the
code-execution sandbox on every node's interfaces - including externally
reachable ones on GKE/EKS/AKS - and pinned every sandbox URL to one node
IP (SPOF on node reboot/drain/ephemeral-IP).
Default the chart to ClusterIP: the provisioner returns a cluster-DNS URL
(http://sandbox-<id>-svc.<ns>.svc.cluster.local:8080) so the gateway->
sandbox hop stays inside the cluster network - no node IP, no 30xxx port,
no external exposure. The chart always runs the gateway in-cluster, so
ClusterIP is always correct there.
NodePort remains an opt-in (provisioner.sandboxServiceType: NodePort +
nodeHost) for the Docker-Compose/hybrid path where the gateway is not in
K8s and cannot resolve .svc.cluster.local; the provisioner code default
stays NodePort for that path.
- values.yaml: add provisioner.sandboxServiceType ("ClusterIP")
- provisioner-deployment.yaml: emit SANDBOX_SERVICE_TYPE; gate the
NODE_HOST block on NodePort mode (default "ClusterIP" for upgrade safety)
- NOTES.txt + README.md: document ClusterIP default + NodePort opt-in
No change to docker/provisioner/app.py (already mode-aware since #4016)
or RBAC (services verbs already cover ClusterIP).
* test(helm): assert sandbox Service-type gating + CHANGELOG the default flip (#3929)
Address review on #4190:
- Add scripts/check_chart_sandbox_service.sh: renders the chart for the
default (ClusterIP, no NODE_HOST), the NodePort opt-in (both emitted),
and NodePort+nodeHost (literal value, not downward API). Locks in the
#3929 gating so a regression (e.g. re-adding an unconditional NODE_HOST,
or dropping the `default "ClusterIP"` upgrade-safety fallback) fails CI.
Wired into .github/workflows/chart.yaml validate-chart job. (#2)
- CHANGELOG [Unreleased] -> Changed: note the NodePort->ClusterIP default
flip on upgrade + the `sandboxServiceType: NodePort` opt-back-in. (#4)
No chart template changes (the gating itself landed in the first commit).
---------
Co-authored-by: Willem Jiang <willem.jiang@gmail.com>
* fix(frontend): use code-aware stripLeakedSystemTags for all internal marker tags
Replace the ad-hoc /<\/?memory>/g in ClipboardSafeStreamdown with a
dedicated stripLeakedSystemTags() function in preprocess.ts that:
- Covers all INTERNAL_MARKER_TAGS (<memory>, <system-reminder>,
<current_date>, <uploaded_files>, <slash_skill_activation>) instead
of only <memory>
- Is code-aware: skips fenced code blocks (```) and indented code
blocks (4-space indent), so user-written meta-discussions about the
memory system are not silently stripped
- Handles opening tags, closing tags, self-closing tags, and tags with
attributes
- Adds 15 unit tests covering all the above cases
* fix(frontend): use marker-aware fence tracking in stripLeakedSystemTags
The boolean insideFence toggle incorrectly closes a fence on any line
matching 3+ backticks or tildes, regardless of the actual delimiter.
This causes tags inside a tilde-fenced block containing a backtick
sub-fence, or a 4-backtick block containing a 3-backtick sub-fence, to
be silently stripped.
Track the opening fence marker (character and run length) so that only
a matching marker with at least the same length closes the fence.
Adds 4 new test cases:
- Tilde fence with inner backtick fence
- 4-backtick fence with inner 3-backtick fence
- Shorter tilde closing inside longer tilde fence
- Tags stripped after real closing fence
* fix(frontend): fix TypeScript strict errors in fence marker tracking
- Use non-null assertion (!) on fenceMatch[1] (guaranteed by regex)
- Use charAt(0) instead of [0] to avoid string | undefined type
* fix(channels): accept leading @mentions before /connect bind codes
Group chats often deliver "@bot /connect <code>" (Feishu/DingTalk leave the
mention in the text). extract_connect_code required the message to start with
/connect, so those binds silently failed while Slack/Discord already strip
mentions before parsing. Skip leading mention tokens in the shared helper.
* test(channels): pin mention variants and case-insensitive /connect parsing
* feat(context): record effective memory identity per run
* fix(context): address memory identity review feedback
---------
Co-authored-by: Willem Jiang <willem.jiang@gmail.com>
Re-ports this feature onto the pluggable-memory backend introduced in #4122
(the original #4143 was force-pushed clean by accident and auto-closed). The
#4122 refactor moved the staleness logic into the self-contained DeerMem
backend (backends/deermem/deermem/core/) and reverted it to the pre-feature
global-threshold version, so the per-fact lifetime work is re-applied here
against the new module layout + DI (MemoryUpdater is now (config, storage,
llm)-injected; config lives on DeerMemConfig, not host MemoryConfig).
**expected_valid_days (creation)**
The LLM assigns a per-fact review window when storing each new fact. The
prompt exposes five tiers (<=14 d transient -> >365 d very stable). The value
is capped at write time by staleness_age_days x staleness_max_lifetime_multiplier
(default 20.0 -> 1800 d ~= 5 years; range 1.0-100.0) so the model cannot set
an initial lifetime so long the fact is never re-evaluated. The default 20.0
makes the "> 365 d very stable" tier achievable out of the box (3.0 silently
clamped it to 270 d).
**staleFactsToExtend (review)**
During staleness review the LLM can emit extension entries for kept facts
whose window seems miscalibrated. new_evd = min(days_since_created +
extend_by_days, staleness_max_extension_days). Extensions use an absolute
ceiling (default 3650 d ~= 10 years; range 90-36500) rather than the creation
multiplier - they are deliberate review decisions that must be able to advance
the window beyond the initial cap, but the absolute bound prevents timedelta
overflow (a model-supplied extend_by_days of 10**9 previously crashed every
later candidate-selection pass with OverflowError) and LLM misfire.
**Invariant correctness**
- Read-time cap removed from _effective_fact_staleness_age; cap is write-time
only so extensions actually advance the review window.
- proposed_remove_ids hoisted out of the removals sub-block and used to exclude
from extension, so a cap-surviving proposed-removal fact is never extended.
- extend_by coerced to int before the > 0 guard (a fractional 0.9 would pass
the float check then int() to 0, silently writing a zero-delta extension).
- days_since uses total_seconds() // 86400 (not .days truncation).
- staleness-section html.escape uses quote=False to match the prompt.py
convention; only <, >, & break element-text structure.
**Tests**
test_memory_staleness_review.py was module-level skipped by #4122 ("full
unit-test migration is a follow-up"). This PR performs that migration: DI
construction via (DeerMemConfig, _FakeStorage), _build_staleness_section back
to the (candidates, config) signature, plus new coverage for per-fact
selection, EXTEND with the absolute cap, the overflow next-cycle regression,
the proposed-removal-not-extendable case, fractional extend_by skipping, and
the creation-time cap. 67 tests, all green.
* fix(github): match allow_authors logins case-insensitively
GitHub logins are case-insensitive, and the sibling gates in this
module already treat them that way. allow_authors used a bare string
membership test, so a YAML casing mismatch silently dropped owner
webhooks that should have bypassed require_mention.
* test(github): parametrize allow_authors case-fold over both directions
Cover the reverse cfg "alice" / payload "Alice" direction plus the
all-caps and exact-case rows from the PR's E2E matrix. Each casing-differs
row is red on the pre-fix source; the exact-case row stays green both ways,
pinning that case-folding is a superset of the old exact match.
config.example.yaml ships backend_config.model: as a bare key whose children
are all comments, which YAML parses to None (make config-upgrade then writes
an explicit model: null). DeerMemConfig.model is a non-Optional field with a
default, so from_backend_config(**{"model": None}) raised a ValidationError
and every run failed with "Input should be a valid dictionary or instance of
DeerMemModelConfig". Drop None entries in from_backend_config so YAML null /
empty keys fall back to field defaults, matching the documented "empty =
host default LLM" semantics. Upstream bug (#4122 schema); regression-pinned
in test_deermem_self_contained.py.
Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
* fix: read run stop_reason from runtime context
* fix: address review feedback for #4188 stop_reason integration
- migration 0005: use safe_add_column for consistency and drift detection
- worker: clear runtime.context stop_reason at start of each _stream_once
turn so a clean continuation doesn't inherit a prior cap reason
- tests: replace circular unit test with real middleware integration
tests that exercise LoopDetectionMiddleware._apply and
TokenBudgetMiddleware._apply through the worker, proving the full
middleware → runtime.context → persist pipeline
* fix(test): resume conftest
* fix: stamp stop_reason in all guard middlewares, fix clearing semantics
The goal evaluator (runtime/goal.py) runs from runtime/runs/worker.py after
the main graph run has already completed, so there is no graph root for it
to inherit tracing from. create_goal_evaluator_model was built with
attach_tracing=False, and evaluate_goal_completion invoked the model with a
bare config={"run_name": "goal_evaluator"} — no tracing callbacks, no
Langfuse session/user attribution. Every goal-evaluator LLM call went
untraced.
Same class of gap fixed by #2944 for the main agent graph and by #3902 for
memory_agent/suggest_agent: a standalone call site that invokes a model
directly instead of through a traced graph root must attach its own tracing
callbacks and inject Langfuse trace-attribute metadata itself.
- create_goal_evaluator_model: attach_tracing=False -> True, matching the
other standalone non-graph callers (oneshot_llm.run_oneshot_llm,
MemoryUpdater).
- evaluate_goal_completion: accept optional thread_id/user_id/
deerflow_trace_id and inject Langfuse trace metadata onto the ainvoke
config via the shared inject_langfuse_metadata() helper, mirroring
oneshot_llm.py's pattern.
- worker.py: thread user_id (resolve_runtime_user_id(runtime)) and
deerflow_trace_id through _prepare_goal_continuation_input into
evaluate_goal_completion so the evaluator's trace groups under the
triggering run's thread/session.
Updates the existing test that pinned attach_tracing=False as expected
behavior, and adds a regression test asserting the ainvoke config carries
Langfuse trace metadata when enabled.
_apply_assistant_delta matched AssistantDelta.id anywhere in the transcript,
which is correct for a genuine per-message id but not for an empty one.
runtime._as_str() coerces a missing/None chunk id to "", and that value is
shared by every id-less chunk from every turn, not just the current one.
Once one assistant row had id="", every later, unrelated AssistantDelta that
also carried id="" (e.g. from a provider that never stamps per-chunk ids)
matched that same stale row instead of starting a fresh one, silently
folding a second turn's answer backward into the first turn's bubble.
Route empty-id deltas to a dedicated path that tracks the current turn's
row by position (streaming_anonymous_row_index, reset on RunStarted/
RunEnded/ClearRows) instead of by id, mirroring the existing empty-id guards
in _apply_tool_started/_apply_tool_result but adapted for assistant text:
unlike a tool call, an id-less assistant delta still needs to be displayed,
so it starts a new row rather than being dropped. Multiple id-less chunks
legitimately arrive within one turn (per-token streaming), so they keep
coalescing into that row -- but only while it is still the transcript tail;
once a tool card is appended after it (the same way a genuine id naturally
changes across a tool round-trip), the next empty-id delta starts fresh
instead of reaching backward past the tool card.
Add regression coverage for the cross-turn merge, same-turn coalescing,
the tool-call-interleaved edge case, and non-interference with the
existing id-keyed path.
* fix(mcp): offload blocking filesystem IO in MCP config update
update_mcp_configuration resolved the extensions config path, probed its
existence, read the raw JSON, wrote the merged config, and reloaded it — all
blocking filesystem IO on the event loop (PUT /api/mcp/config). The whole
read-modify-write after the async admin check has no interleaved awaits, so it
moves into one _apply_mcp_config_update helper dispatched via asyncio.to_thread;
the masked response is built on the loop. The secret-preserving merge, error
codes, and the stdio command allowlist are unchanged.
Found via `make detect-blocking-io`. Same class as #3457 / #3529 / #3551.
Add tests/blocking_io/test_mcp_router.py anchor, verified red->green under the
strict Blockbuster gate.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* fix(mcp): serialize concurrent config updates with a write lock
Address review on #3552: offloading the read-modify-write to a worker thread
dropped the implicit serialization the single-threaded event loop provided, so
two concurrent PUT /api/mcp/config calls could interleave and clobber each
other. Guard the offloaded RMW with a module-level asyncio.Lock to restore
within-process atomicity (cross-process writers remain a separate, pre-existing
concern).
Add a serialization regression test (red->green: without the lock the tracked
max concurrency exceeds 1).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* fix: address mcp config blocking io review
---------
Co-authored-by: ly-wang19 <ly-wang19@users.noreply.github.com>
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-authored-by: Willem Jiang <willem.jiang@gmail.com>
* fix(gateway): keep create_thread idempotent when the insert loses a race
The /api/threads create endpoint checks for an existing row and then
inserts, but the two steps are not atomic. When two requests create the
same thread_id concurrently, one commits first and the other's INSERT
fails on the duplicate primary key. The SQL-backed thread store raises,
the handler catches it as a generic failure, and the loser gets a 500 —
even though the docstring promises the call is idempotent.
Re-read the row after a failed insert: if it is now present (the
competing request won), return it instead of surfacing the conflict. A
genuine write failure where the row is still absent keeps the 500.
Covered by a regression test that simulates the lost race and asserts
the endpoint returns the existing thread rather than erroring.
Signed-off-by: Yufeng He <40085740+he-yufeng@users.noreply.github.com>
* fix(gateway): scope create_thread race recovery and mirror owner reconciliation
Address review on #3800:
- Scope the insert-race recovery to sqlalchemy IntegrityError instead of a
broad `except Exception`, so a non-race failure that coincides with an
existing row is no longer silently returned as a 200. Any other error logs
and surfaces as a 500. (The memory store overwrites on duplicate rather than
raising, so it never reaches this path.)
- Route both the fast path and the recovery path through a shared
`_resolve_existing_thread` helper so the recovery performs the same trusted-
owner reconciliation (claiming a legacy unscoped `user_id=None` row) the fast
path does. Thread ownership no longer diverges based on which path resolved
the record.
Add regression tests for both: the recovery claims an unscoped row for a
trusted owner, and a non-IntegrityError failure surfaces as a 500.
---------
Signed-off-by: Yufeng He <40085740+he-yufeng@users.noreply.github.com>
Co-authored-by: Willem Jiang <willem.jiang@gmail.com>
* fix(memory): bounded shutdown flush via MemoryManager.shutdown_flush
Re-applies the memory-queue shutdown drain on top of the pluggable
MemoryManager abstraction (#4122): the old top-level MemoryUpdateQueue
singleton is gone, so the drain is now a backend contract instead of
host code reaching into the queue.
- MemoryManager ABC: shutdown_flush(timeout) -> bool. Every backend
implements a bounded graceful-shutdown drain.
- DeerMem: queue.flush_sync (daemon-thread + Event.wait hard timeout
for the uninterruptible sync LLM call; joins an in-flight worker
first so contexts a debounce Timer already pulled out are not lost on
exit; skips inter-item sleep on the drain path; per-item
succeeded/failed count), exposed via shutdown_flush.
- noop: shutdown_flush is a clean no-op success.
- Gateway lifespan: call get_memory_manager().shutdown_flush(timeout)
after channels/scheduler stop, via asyncio.to_thread, try/except
bounded. No host-level pending/processing guard -- the backend
short-circuits on an idle buffer, so the host cannot "forget" the
in-flight case (structurally eliminates the guard race flagged on the
prior revision).
- shutdown_flush_timeout_seconds added to the shared MemoryConfig
(host-owned lifecycle budget, default 30, 1-300) + exposed on
MemoryConfigResponse and the embedded client; config_version 25 -> 26.
Tests: queue flush_sync (7), lifespan drain incl. False-branch caplog
assertion + disabled gate (3), ABC contract noop/deermem (3).
* fix(chart): gateway grace period so memory drain is not SIGKILLed
K8s defaults terminationGracePeriodSeconds to 30s, shorter than the
Gateway's graceful-shutdown work (channel stop ~5s + memory queue drain
default 30s). Without an explicit grace period, K8s SIGKILLs the memory
drain mid-flight and silently re-introduces the loss shutdown_flush is
fixing (flagged on the prior revision).
- gateway pod: terminationGracePeriodSeconds (default 45, configurable).
- gateway container: preStop sleep (default 5, 0 disables) so the
Service/ingress deregisters the pod before SIGTERM begins the drain.
- values.yaml + README: both configurable; README documents that the
grace period must track memory.shutdown_flush_timeout_seconds.
* docs(memory): document shutdown_flush_timeout_seconds + lifespan drain
Add the host-shared field to the memory config list and Config Schema
summary in backend/AGENTS.md, noting the lifespan drain and the K8s
grace-period relationship.
* fix(chart): bump embedded config_version to 26
The chart's embedded `config:` block (values.yaml + README example) still
had config_version: 25 after commit f3ca8e9f raised config.example.yaml to
26, failing the validate-chart config_version drift check. Bump both to 26.
_prepare_goal_continuation_input calls the same _persist closure twice
with the identical next_count in one evaluation cycle: once to commit
the real continuation, and again to record a thread_changed_before_continuation
stand-down if a race is detected right after that commit. The second
call re-passed continuation_count=next_count, so #4088's defensive
max(continuation_count, current_count + 1) guard saw the first call's
own write as a "current_count" bump and added another +1 on top of it -
silently consuming 2 units of the continuation budget for a cycle that
delivered zero actual continuations. #4088's guard is correct for the
independent-concurrent-continuations race it targets; this is a
separate call site incorrectly re-triggering that same guard against
itself.
The second _persist call no longer passes continuation_count, matching
every other stand-down call site in this function - the count was
already correctly committed by the first call.
Adds a regression test mirroring the existing
thread_changed_after_evaluation race test's checkpointer-wrapper
technique, since this sibling branch had zero prior coverage.
get_skills_prompt_section() without app_config resolved get_app_config()
only to read container_path, then let the enabled-skills load fall back
to the warm cache. On a cold start the cache is empty and the first call
returns an empty skills list while the synchronously-loaded disabled
section is populated, so manually assembled agents (create_deerflow_agent
style integrations) got a prompt with no enabled skills.
Rebind the resolved config so the storage and enabled-skills loads below
use it too; when no config is resolvable the cache-only fallback is
unchanged. Adds a cold-cache regression test.
Fixes#4144
Co-authored-by: fancyboi999 <fancyboi999@users.noreply.github.com>
