* 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(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>
binding, Internal Auth) and group IM bindings with direct HTTP under a
shared platform-trust model. Explain Internal Auth trust boundaries:
DeerFlow validates only the platform token, does not manage end-user
accounts in users, and relies on the channel to maintain owner identity.
Cross-link AUTH_DESIGN, API.md, and docs README
* fix(channels): dedupe GitHub webhook redeliveries
The inbound dedupe added for the IM channels in #3584 keys
ChannelManager._is_duplicate_inbound on a top-level metadata["message_id"]
plus a workspace id. The GitHub channel added later in #3754 never
populated either: the X-GitHub-Delivery GUID was buried in
metadata["github"]["delivery_id"] and no workspace id was set, so every
GitHub delivery produced a None dedupe key and was never deduped.
GitHub reuses the same X-GitHub-Delivery GUID when a delivery is retried
after a timeout or replayed via the repo/App "Redeliver" button, so a
redelivery re-ran the agent with real side effects (e.g. a duplicate PR
comment) while the other channels absorbed an identical redelivery.
fanout_event now stamps the dedupe identity the manager already consumes:
- workspace_id = repo (globally unique, always present; mirrors
Telegram/WeChat keying the workspace on the chat id)
- metadata["message_id"] = f"{delivery_id}:{agent.name}"
A single delivery fans out to N agents, so the id is scoped to
(delivery, agent): an identical redelivery reproduces the same pairs
(deduped) while two agents matching the same delivery keep distinct ids
and both still fire. When the delivery header is absent the id is left
None, so the manager fails open exactly as before.
Tests: dispatcher-level coverage that the identity is stamped, stable
across redelivery, and distinct per agent/delivery; a ChannelManager
regression that an identical GitHub redelivery dispatches once while a
new delivery and a second agent on the same delivery still fire.
* fix(channels): scope GitHub dedupe id by owning user
_inbound_dedupe_key indexes on (channel, workspace_id, chat_id,
message_id). For GitHub, workspace_id and chat_id are both the repo,
so the owning user was never represented anywhere in the key -
fanout_event stamped the id as f"{delivery_id}:{agent.name}".
Two different users each binding an agent of the same name (e.g.
"reviewer") to the same repo+event therefore produced an identical
dedupe id for both fan-out messages. ChannelManager._is_duplicate_inbound
treated the second as a replay of the first and silently dropped it,
even though GitHub delivered the webhook once and both users' agents
legitimately matched.
Fold match.user_id into the id: f"{delivery_id}:{match.user_id}:{agent.name}".
Genuine redeliveries (same user, same agent, same delivery) still
produce the same id and are deduped; two agents - same-named or not,
same user or not - on one delivery now always keep distinct ids.
Tests: new cross-user regression pins that two users' same-named
agents both dispatch instead of the second being deduped against the
first; the existing per-(delivery, agent) test and the literal id
assertion in test_delivery_id_populates_inbound_dedupe_identity are
updated for the new id shape.
* fix(channels): point cross-user dedupe comment at its regression test
Replace the inline reviewer-handle attribution with a pointer to
test_dedupe_identity_distinguishes_same_agent_name_across_users, which
ages better than a person's name in committed code.
* feat(memory): pluggable + self-contained memory system (MemoryManager plan phases 1 & 2)
Phase 1 — Pluggable (steps 0-10):
- ABC MemoryManager (9 methods) + singleton factory + drop-in backend discovery
- DeerMem default backend with core/ (storage/queue/updater/prompt/message_processing)
- NoopMemoryManager backend (proves pluggability)
- All call sites (middleware/hook/prompt/gateway/client/app) routed through manager
- hasattr capability probing for DeerMem-internal methods (no hard imports)
- MemoryConfig gains manager_class field; shared vs DeerMem-private annotated
Phase 2 — Self-contained DeerMem (steps 11-18):
- backend_config passthrough + DeerMemConfig (all DeerMem-private fields moved off MemoryConfig)
- DI: DeerMem owns storage/queue/updater/llm as instance attributes (no global singletons)
- Storage independence: core/paths.py with own root (~/.deermem or ),
factory auto-injects deer-flow's runtime_home() as absolute base_dir (zero-config)
- LLM independence: core/llm.py via langchain init_chat_model (no create_chat_model)
- Trace independence: optional tracing_callback replaces inject_langfuse_metadata/request_trace_context
- Message processing independence: hide_from_ui default-skip + optional should_keep_hidden_message hook
- Internal imports → relative (only deer_mem.py ABC import is host-relative)
- Carrier (deer_mem.py adapter) / portable (deermem/ config+core) split
- New tests: test_deermem_self_contained + test_memory_manager_pluggable; all memory tests migrated
- Other-agent demo: samples/other_agent_demo/ + automated portability test
- config.example.yaml memory section updated to phase-2 schema
* feat(memory): port consolidation + staleness fix into self-contained DeerMem; phase-2 host hooks
Port upstream #3996 (memory consolidation) and #3993 (staleness KeyError fix)
from origin/MemoryManager into the pluggable, self-contained DeerMem structure
(backends/deermem/deermem/), adapted to the DI MemoryUpdater (config injected,
not get_memory_config globals):
- DeerMemConfig: add consolidation_enabled (opt-in, default false) /
consolidation_min_facts / consolidation_max_groups_per_cycle /
consolidation_max_sources
- prompt.py: factsToConsolidate JSON field + {consolidation_section} placeholder
+ CONSOLIDATION_PROMPT constant
- updater.py: _coerce_source_confidence / _select_consolidation_candidates /
_build_consolidation_section module helpers (matching the existing
_select_stale_candidates style); consolidation normalization in
_normalize_memory_update_data; consolidation apply in _apply_updates (after
max_facts trim, with apply-time guardrails mirroring staleness); staleness
KeyError fix (f["id"] -> f.get("id") is not None) applied to both the
staleness guardrail and the consolidation allowed_source_ids comprehension
- config.example.yaml: consolidation section under memory.backend_config
- tests/test_memory_consolidation.py: 40 DI-adapted tests (running, not skipped)
incl. the staleness KeyError regression
Also includes in-flight phase-2 host-integration work: storage_path semantics
(any absolute/relative value = root dir) and host-default tracing_callback /
should_keep_hidden_message hooks injected into backend_config by the factory.
Co-Authored-By: Claude <noreply@anthropic.com>
* feat(memory): add noop backend template and backends guide
- backends/noop/: complete drop-in template (config.py with zero deer-flow
imports, noop_manager.py with a 6-step new-backend walkthrough in its
docstring, commented optional fact-CRUD capabilities).
- backends/README.md: which files to touch when adding/swapping a backend,
the 5-item backend contract, and common pitfalls.
- manager.py: generalize backend examples in comments (drop mem0-specific
references).
Co-Authored-By: Claude <noreply@anthropic.com>
* fix(frontend): guard formatTimeAgo against invalid timestamps
Return a neutral placeholder when the input date is invalid (e.g. an empty lastUpdated from a backend with no memories) instead of throwing 'Invalid time value' from date-fns.
Co-Authored-By: Claude <noreply@anthropic.com>
* feat(memory): wire tool-driven memory mode through the MemoryManager ABC
tools.py (memory_search/add/update/delete) now calls get_memory_manager()
instead of the removed host memory module, so tool mode (memory.mode: tool)
works for any backend. DeerMem.search is implemented (case-insensitive
substring match, ranked by confidence) as a stand-in for the planned
semantic retrieval; noop.search returns [] (unchanged). Fact-CRUD tools
use getattr+callable probing -- backends lacking those ops (noop) get a
clear JSON error instead of crashing.
Tests: test_memory_tools rewired to mock the manager (handler tests) +
TestModeGating retained; test_memory_search now covers DeerMem.search;
pluggable stubs test updated (search no longer a stub).
Co-Authored-By: Claude <noreply@anthropic.com>
* fix: resolve lint errors (import sorting, type annotation quotes, E402 in skipped tests)
* docs: restore explanatory comments in config.example.yaml memory section
* fix(security): port html-escape memory facts fix (#4097) to vendored DeerMem prompt.py
* fix(memory): address review + port dropped upstream memory fixes
Review blockers (vendored DeerMem):
- #4044 restore _escape_memory_for_prompt (current_memory blob in
MEMORY_UPDATE_PROMPT) - prevents </current_memory> breakout
- #4028 html.escape staleness-section cat/content in _build_staleness_section
- #4119 add _escape_summary for injection-path summaries (Work/Personal/
Current Focus/Recent/Earlier/Background)
- default-model silent no-op: factory injects host default chat model via a
new host_llm slot (create_chat_model(name=None)); DeerMem prefers host_llm
over build_llm(model). Zero-config extraction works out of the box again
- MemoryConfigResponse: fix stale docstring (backend-agnostic shape; DeerMem
knobs live under backend_config, not top-level - restoring flat would
re-couple the API to DeerMem). Frontend audited: does not read /memory/config
- _host_default_tracing_callback: restore langfuse assistant_id/environment
- search: push category onto the ABC signature; DeerMem filters BEFORE the
top_k slice (was filtered client-side after slicing -> starved results)
- _do_update_memory_sync: split into wrapper+impl; bind trace_id into the
request-trace ContextVar on the Timer/executor worker via a new
trace_context_manager host hook (None trace_id left unbound - no fabrication)
- client.py fact-CRUD now passes user_id (was writing to the global bucket
while get_memory reads per-user)
- _resolve_manager_class: fail-fast (raise ValueError) on an unresolved
explicit manager_class instead of silently falling back to DeerMem (memory is
persistent state - a wrong store is a silent data-integrity footgun)
Upstream memory fixes dropped by the host->vendored rename conflict, re-ported
to backends/deermem/deermem/core/ (+ deer_mem.py):
- #4073 queue busy-timer-spin -> _reprocess_pending flag (core/queue.py)
- #4074 null source.confidence in staleness -> _coerce_source_confidence
(core/updater.py: _build_staleness_section + _apply_updates stale sort)
- #4075 factsToRemove is optional (drop from _REQUIRED_MEMORY_UPDATE_TOP_LEVEL_KEYS)
- #4076 null confidence in search ranking -> _coerce_source_confidence
(deer_mem.py DeerMem.search)
host_llm + trace_context_manager are host-injected via backend_config (factory
in manager.py), keeping backends/deermem/ at exactly one `from deerflow` line
(the ABC contract) - portability test preserved.
Co-Authored-By: Claude <noreply@anthropic.com>
* fix: resolve lint errors (F541 f-string without placeholders, E501 line too long)
* fix(memory): restore hide_from_ui clarification preservation, expose mode
Two memory-system fixes (F541/E501 lint was already fixed on this branch):
- filter_messages_for_memory: restore default preservation of well-formed
human_input_response clarification answers (v2 regression). The
self-containment refactor made the bare function skip ALL hide_from_ui when
no hook was passed, but upstream preserves well-formed clarification
responses by default (test_hide_from_ui_human_input_response_is_preserved).
Inline a host-agnostic _is_human_clarification_response mirror of
read_human_input_response as the default keep-decision; the host-injected
should_keep_hidden_message hook still overrides (production path unchanged).
Portable package stays zero `from deerflow`.
- /memory/config: expose `mode` (middleware|tool) in MemoryConfigResponse +
the config/status endpoints + client.get_memory_config. mode is a host-
shared, behavior-determining field missing from the response projection.
Sync tests (mock .mode; e2e assert mode present).
- Align manager_class field docstring with fail-fast behavior.
Tests: filter/self-contained/portability (35) + memory-config (4) pass;
ruff clean.
Co-Authored-By: Claude <noreply@anthropic.com>
* fix(memory): resolve ruff format failures in memory module + tests
`make lint` runs `ruff format --check` in addition to `ruff check`; 8 memory
files had pending format changes -- 7 pre-existing (deer_mem, updater, tools,
test_memory_queue/router/search/tools) + message_processing from the
hide_from_ui fix. Apply `ruff format`: whitespace/wrapping only, no logic
change. 109 memory tests pass; ruff check + format --check both clean.
Co-Authored-By: Claude <noreply@anthropic.com>
* fix(memory): address PR review - legacy field migration, fact_id contract, path/docs
Address willem-bd's review on PR head bc8bf0d4 (risk:high, persistent state):
- config: auto-migrate pre-abstraction top-level memory.* DeerMem fields
(storage_path, max_facts, debounce_seconds, model_name, token_counting,
staleness_*, consolidation_*) into backend_config on load + warn, so an
upgrade does NOT silently revert customized settings (was: silent
extra='ignore' drop). model_name -> backend_config.model.model. Unknown
top-level keys warned.
- factory: resolve a relative backend_config.storage_path against runtime_home()
(base_dir-relative, CWD-independent) to preserve pre-abstraction semantics;
paths.py stays portable (no runtime_home import).
- tools: memory_add uses the fact_id returned directly by create_fact instead of
re-deriving it via content-key matching (coupled the tool to the backend's
content normalization; could misreport a storage cap). create_fact now returns
(memory_data, fact_id); gateway/client/tool updated. Fix terse
{"error":"content"} -> {"error":"empty content"}.
- app.py: update stale token_counting=="char" warm-up comment to point at
manager.warm (DeerMem.warm re-checks char and returns early).
- router: comment explaining reload_memory silent fallback vs fact 501 asymmetry
(read-only degrade vs write fail-loud).
- CHANGELOG: document breaking changes (/memory/config + client.get_memory_config
shape flat->backend_config; custom storage_class path moved + __init__ must
accept config) and the legacy-field auto-migration.
- tests: add regression test pinning the per-user memory path
({storage_path}/users/{safe_user_id}/memory.json == host make_safe_user_id)
across the abstraction; update create_fact mocks for (memory_data, fact_id).
Tests: 273 passed (memory suite); ruff check + format clean.
Co-Authored-By: Claude <noreply@anthropic.com>
* fix(memory): address PR review - storage_path, max_facts, tracing, parsing
Six review findings (willem-bd), each verified against upstream:
- storage_path semantics (file -> root dir): migration drops file-style
(.json) legacy values with a warning; factory raises if storage_path
resolves to an existing file (avoid silent NotADirectoryError write
failure). CHANGELOG + config.example.yaml comment updated.
- create_memory_fact enforces max_facts again (via _trim_facts_to_max) and
returns (memory, None) when the cap evicts the new fact; memory_add tool
reports "not stored", client raises ValueError, POST /memory/facts -> 409.
- max_facts trim uses _coerce_source_confidence (was raw f.get("confidence",
0) -> TypeError on non-float imported/legacy confidence, swallowed as
silent update failure).
- memory-tracing assistant_id restored to "memory_agent" (was "lead-agent"
copy-paste; matches upstream + DeerMem run_name).
- _is_human_clarification_response cross-checked against
read_human_input_response (drift guard test).
- empty-string legacy values skipped silently in migration (narrow fix, not
broad "if not value" which would skip explicit bool False).
8 new regression tests. make lint + 406 memory tests pass.
Co-Authored-By: Claude <noreply@anthropic.com>
* fix(memory): address internal review - storage fail-fast, build_llm degrade, config warn, noop template
Addresses 4 findings from the PR #4122 internal supplemental review
(parallel to willem-bd's review, no overlap):
- create_storage fail-fast: a misspelled/unimportable storage_class now
raises ValueError instead of silently falling back to FileMemoryStorage.
Memory is persistent state, so a wrong store is a data-integrity footgun;
mirrors the existing manager_class resolution policy. (storage.py)
- noop template create_fact signature: the commented template used
keyword-only `content` and returned a bare dict, while DeerMem's actual
create_fact takes positional `content` and returns tuple[dict, str|None]
(the memory_add tool passes content positionally; gateway/client/tools all
tuple-unpack). A backend copied from the template would 500 on fact-CRUD.
Template fixed; delete_fact/update_fact templates left (callers compatible).
(noop_manager.py)
- build_llm graceful degrade: wrap init_chat_model in try/except, degrade to
None + WARNING on failure (mirroring _host_default_llm) so a misconfigured
explicit model does not crash app startup -- non-LLM memory ops still work
and an update raises at runtime with the error logged. (llm.py)
- from_backend_config unknown-key warning: log a WARNING for unknown
backend_config keys (mirrors the host layer's load_memory_config_from_dict)
so a typo like `storage_pat` does not silently fall back to the default and
write memory to an unintended location. (config.py)
Tests: rewrote 3 create_storage fallback tests to expect ValueError; added 4
tests (build_llm zero-config/degrade, from_backend_config warn/silent).
make lint green; full memory suite passes.
Co-Authored-By: Claude <noreply@anthropic.com>
---------
Co-authored-by: lllyfff <2281215061@qq.com>
Co-authored-by: Claude <noreply@anthropic.com>
Co-authored-by: lllyfff <122260771+lllyfff@users.noreply.github.com>
update_agent (the harness tool) and PUT /api/agents/{name} (the same
operation over HTTP) share an identical guard meant to block updates to
an agent that only exists in the legacy shared layout. The guard checked
bare directory existence:
if not agent_dir.exists() and paths.agent_dir(name).exists():
When memory is enabled, the first time a user chats with a legacy shared
agent, the memory writer creates a per-user directory containing only
memory.json (no config.yaml). agent_dir.exists() is then true, so the
guard never fires: the tool falls through to load_agent_config, which
resolves through to the legacy shared config via the already-hardened
resolve_agent_dir, and silently writes a brand-new config.yaml/SOUL.md
into the memory-only directory. That forks the agent for just this user;
every other user keeps reading the original shared config forever, with
no error or warning.
resolve_agent_dir itself was already hardened against exactly this
failure mode: it requires config.yaml to exist, not just the directory.
Mirror that condition at both call sites here.
format_conversation_for_update embeds raw user turns into the <conversation>
slot of MEMORY_UPDATE_PROMPT. This is the most attacker-influenced input in the
prompt, and it was unescaped: a message containing
"</conversation><current_memory>..." closes the conversation block and forges a
<current_memory> authority section for the extraction LLM, which can be steered
into persisting an arbitrary high-confidence fact — and that fact is later
injected into the lead-agent system prompt's <memory> block, which the prompt
declares trusted.
This is the last unguarded sibling of a rule the repo has established repeatedly.
#4044/#4060 html-escaped the current_memory slot of this exact template; #4097
escaped the <memory> injection renderer. In updater.py the same .format() call
escapes current_memory and leaves conversation raw. The memory updater sees raw
text because InputSanitizationMiddleware only rewrites the ModelRequest and never
mutates state, while MemoryMiddleware queues the raw state messages.
Escape content with html.escape(quote=False), mirroring _escape_summary /
_format_fact_line — after truncation so a trailing "..." cannot split an entity,
on both human and assistant turns. Render-time only: no stored value is mutated,
so the apply path is unaffected. The conversation function already strips
<uploaded_files> here, so tag hygiene in this renderer is established.
Scope is the memory updater. The summarizer's <new_messages> / <existing_summary>
blocks are the same rule unguarded, but their output is quarantined as untrusted
durable context rather than promoted to system authority; that hardening will be
a separate change.
select_skill_packages() resolved every changed non-SKILL.md path to its
owning package via an unconditional depth-3 fallback, then queued that
path for review. When a PR deletes an entire public skill package (not
just SKILL.md, but its scripts/assets/etc. too), the fallback still
returned the package directory even though it no longer exists on disk
post-deletion. The review CLI then reported a false
structure.missing-skill-md blocker for a path that isn't there,
failing CI on a routine, correct package removal.
Skip a resolved package only when every changed file under it was a
deletion and the package directory itself is gone from disk - i.e. the
whole package was intentionally removed. A package left in a
broken/partial state (e.g. SKILL.md deleted while sibling files
remain) still resolves to an existing directory, so it is unaffected
and continues to be queued and flagged.
