`_provider_config` resolved a request-supplied provider name with an
unallowlisted `getattr(config, provider, None)`. `ChannelConnectionsConfig`
carries non-provider attributes -- the `enabled` and `require_bound_identity`
bool fields plus the `provider_status` method -- so a name matching one of
them returned that attribute instead of falling through to the intended 404.
Callers (e.g. `POST /api/channels/{provider}/connect`, reachable by any
authenticated user) then dereferenced the bool/method as a provider config,
crashing with `AttributeError` -> HTTP 500.
Validate `provider` against the `_PROVIDER_META` allowlist before the lookup,
matching how `_credential_fields` / `_connect_instruction` / `_connect_url`
already gate provider names, so unknown providers get a clean 404.
Add a parametrized router regression test covering `enabled`,
`require_bound_identity`, `provider_status`, and an unknown name.
* Make the sync checkpointer honor the unified database config
The sync checkpointer factory (`get_checkpointer` and `checkpointer_context`)
read only the legacy `checkpointer:` config section and fell back to
`InMemorySaver` when it was absent — it never consulted `database:`. The async
`make_checkpointer` factory and both sync/async Store providers already resolve
the unified `database:` section (legacy `checkpointer:` takes precedence,
otherwise `database:` drives the backend), so the sync checkpointer was the
lone outlier.
Consequence: with `database: {backend: sqlite|postgres}` and no legacy
`checkpointer:` section, the sync checkpointer silently returned `InMemorySaver`
while the Store — same process, same config — correctly persisted to
sqlite/postgres. Embedded callers (`DeerFlowClient`) and the TUI hit this: e.g.
the TUI writes `threads_meta` rows to sqlite (thread appears in the Web UI) but
its checkpoints went to memory and were lost on exit. This also contradicts
backend/AGENTS.md ("the unified `database` section selects the Gateway's
LangGraph checkpointer, LangGraph Store, and DeerFlow SQL repositories").
Mirror the sync Store provider: add `_resolve_checkpointer_config` /
`_get_checkpointer_config` (legacy precedence, else translate `database:` into
a CheckpointerConfig) and route both the singleton and the context manager
through them. The Gateway is unaffected — it uses the async path.
Adds a TestCheckpointerDatabaseConfig suite mirroring the Store's database
tests (singleton + context-manager honor `database:`, legacy precedence,
explicit-memory, missing-config fallback). The two `uses_database_config`
tests fail (return InMemorySaver) before the fix and pass after.
* Handle database=None in sync checkpointer resolution
The unified-config change made `checkpointer_context` / `get_checkpointer`
consult `app_config.database` when no legacy `checkpointer:` section is set.
`AppConfig.database` is always a `DatabaseConfig` in production, but the
existing regression test for issue #1016
(`test_checkpointer_none_fix.py::test_sync_checkpointer_context_returns_in_memory_saver_when_not_configured`)
mocks the app config with `database` left unset, exercising a `database=None`
path that now raised `ValueError: Unknown database backend`.
Mirror the async `make_checkpointer` factory, which already tolerates
`database=None` (falls back to memory), by treating a `None` database as the
memory backend in `_resolve_checkpointer_config`. Update the sync test to set
`mock_config.database = None`, matching its async sibling in the same file.
* Address review: mirror None-guard in store resolver, add sqlite coverage
`select:` names its targets explicitly, so capping it silently drops schemas the
model asked for by name -- and picks the survivors by catalog order, not request
order. The model is told the tool it wanted was not returned by nothing at all;
it then tries to call a tool that is still deferred.
The rule is already stated three times in the repo, and this is the one place
that breaks it:
- backend/AGENTS.md:447 -- "select: returns all requested skills without a
result cap; other modes cap at MAX_RESULTS=5"
- skills/catalog.py:71 -- SkillCatalog.search returns select: uncapped; the
ranked modes slice. DeferredToolCatalog shares its query grammar and its
MAX_RESULTS = 5, and is capped.
- tool_search's own docstring -- "select:Read,Edit -- fetch these exact tools
by name" versus "notebook jupyter -- keyword search, up to max_results best
matches". Only the ranked form promises a cap.
The cap is applied twice: once inside `DeferredToolCatalog.search` and again in
the tool closure. `search` already caps the ranked branches internally, so the
closure's slice is redundant for them and is the only thing capping `select:`
once the first is removed -- fixing one site alone changes nothing the model can
observe. Its sibling closure, `skills/describe.py::describe_skill`, calls
`catalog.search(name)` with no slice.
Both slices are removed. The ranked modes keep their cap.
* fix(sandbox): guard the output-masking regex with a segment boundary
`_compiled_mask_patterns` builds the same class of host→virtual matcher as
`LocalSandbox._reverse_output_patterns`, but without the segment-boundary
lookahead that one carries. The trailing group needs a separator to consume
anything, so when the character after a host base is `-`, `.`, `_`, a digit or
a letter, the group matches empty and the regex still matches the bare base.
`replace_match` then takes its `matched_path == base` branch and rewrites the
sibling: with `/mnt/skills` mounted at `.../skills`, output naming a sibling
`.../skills-extra/data.txt` is handed to the model as `/mnt/skills-extra/data.txt`
— a container path forward resolution explicitly refuses to map back, so
reading it raises FileNotFoundError.
This is the sibling site of #4035, which fixed the identical bug in
`local_sandbox.py`. That PR's scope argument enumerated the prefix matchers in
that file and missed this one; `mask_local_paths_in_output` runs on every
glob/grep match and on local bash output.
The boundary class mirrors `_content_pattern`'s, not `_command_pattern`'s: this
runs over arbitrary command output, where a base can legitimately be followed
by `,`, `:` or `\`, all of which the shell-oriented class rejects.
* test(sandbox): anchor the sibling boundary at the ACP source too
The sibling-rejection cases only fed the skills source. `_compiled_mask_patterns`
builds every source's matcher in one loop, so the ACP workspace carried the same
defect: nothing maps its parent, and `/mnt/acp-workspace-backup/hello.py` is
unresolvable in both directions.
User-data is the exception and is now pinned as such: `_thread_virtual_to_actual_mappings`
also maps the virtual root `/mnt/user-data` to the three dirs' common parent, so a
sibling of `outputs` is still inside a mount and has a real virtual path — the output
is byte-identical with and without the boundary. That test is green on main; it guards
the boundary from being narrowed into one that stops translating a mapped path.
Reverting only `boundary` turns the 5 skills + 4 ACP cases red and leaves the 3
user-data cases green.
* fix(sandbox): honor single-bound line ranges in read_file
read_file only sliced content when BOTH start_line and end_line were
provided, so read_file(path, start_line=100) or read_file(path,
end_line=50) silently returned the whole file. Handle one-sided ranges:
default the missing bound (start->1, end->EOF), clamp start to 1, and
return clear messages when start_line exceeds the file length or start_line
> end_line.
* address review: guard one-sided end_line <= 0 in read_file
Add symmetric end_line guard and run the inverted-range check regardless
of which bounds are explicit, so a lone end_line<=0 returns a clean
error instead of a negative-index slice. Per @willem-bd review on #4078.
Co-Authored-By: Claude <noreply@anthropic.com>
---------
Co-authored-by: Claude <noreply@anthropic.com>
`_fallback_title` sliced the user message to `min(max_chars, 50)` and then
appended a three-character ellipsis, so the returned title could be three
characters longer than the configured cap. `_parse_title`, six lines above,
slices the model's answer to `max_chars` exactly -- both read the same
`TitleConfig.max_chars`, only one honoured it.
This is the default path, not an error branch: `config.example.yaml` ships
`title.model_name: null` ("null = fast local fallback"), so every title is
produced here unless the operator opts into a title model. `max_chars` is a
documented key with a pydantic range of 10..200; any value in 10..52 makes a
long first message overshoot its cap.
Reserve room for the ellipsis before slicing. At the shipped `max_chars: 60`
the body is still 50 characters, so default output is unchanged.
The existing `test_sync_generate_title_respects_fallback_truncation` asserted
the shape of the truncation but never its length -- at its own `max_chars=50`
it was passing on a 53-character title. It now asserts the bound it is named
after.
* fix(artifacts): honor trusted owner-user-id header
The artifact endpoint resolved paths only via the effective user, so a
trusted internal caller acting on behalf of an owner (carrying
X-DeerFlow-Owner-User-Id) read the synthetic internal user's storage and
404'd on files the owner's run had written.
Resolve the owner via get_trusted_internal_owner_user_id and pass it
through resolve_thread_virtual_path (now accepting an optional user_id),
matching the memory and threads routers. Browser/API callers send no
such header and fall back to the effective user, so their behavior is
unchanged.
* fix(artifacts): normalize trusted owner id through make_safe_user_id
The owner-user-id header carries the raw platform owner id, while runs
store files under the make_safe_user_id bucket. Resolve artifacts with
the normalized id, mirroring the memory router.
MemoryStreamBridge is the DEFAULT stream backend. Its subscribe() creates a
fresh ended=False stream on every call via _get_or_create_stream(), but the
Gateway's reconnect guard _terminal_record_stream_missing only detects a
missing stream on bridges that expose stream_exists — and MemoryStreamBridge
did NOT define it (only RedisStreamBridge did). After worker cleanup pops the
stream (~60s post-run), a browser SSE reconnect or POST /wait hits
subscribe() -> creates a zombie stream -> yields heartbeat forever without
ever sending END_SENTINEL. The UI spinner never resolves and the coroutine
pins a server-side connection/request until external timeout.
Add the missing stream_exists method, mirroring RedisStreamBridge.
* ci: add nightly build workflow for images + helm chart
Nightly build of backend/frontend/provisioner images and the helm chart,
pushed to GHCR with nightly + nightly-YYYYMMDD tags (latest stays on v*
releases). amd64 only. Gated to the upstream repo (bytedance/deer-flow).
Documented in RELEASING.md.
* ci(nightly): harden chart patch + dedupe config_version check
Address PR #4050 review feedback:
- Gate the in-workflow chart patches with grep assertions so a drifted
Chart.yaml/values.yaml fails loudly instead of silently shipping a chart
that pulls the release `latest` images (sed exits 0 on zero matches).
- Suffix the nightly chart version with the short SHA
(<base>-nightly.<date>-<sha>) so a same-day re-dispatch re-publishes
cleanly; OCI chart versions are immutable and otherwise collide.
- Note in the image-tags comment that :nightly-<date> is mutable within a
day and :sha-<short> is the only truly immutable pin.
- Extract the config_version drift check into scripts/check_config_version.sh,
shared by chart.yaml and nightly.yaml, so the parsing logic lives in one
place.
* feat(runs): cross-process run ownership with lease + reconciliation (#3948)
Implements work items 2 and 3 of the multi-worker P0 plan
(docs/multi_worker.md). Work item 1 (Postgres startup gate, #3960)
already landed; this PR makes run creation race-safe across worker
processes and lets Postgres deployments recover orphaned inflight runs
from crashed workers without mis-marking live runs as orphans.
Work item 2 — cross-process atomic create_or_reject
- Alembic revision 0004_run_ownership adds runs.owner_worker_id,
runs.lease_expires_at, idx_runs_lease, and a partial unique index
uq_runs_thread_active (one pending/running run per thread). The
index is declared on RunRow.__table_args__ with sqlite_where +
postgresql_where (mirroring uq_channel_connection_active_identity)
so the empty-DB bootstrap path — which runs Base.metadata.create_all
+ alembic stamp head without executing any revision's upgrade() —
also lands it on fresh deployments. Migration 0004 additionally
creates it idempotently for legacy/versioned upgrades.
- RunRepository.create_run_atomic is the new atomic primitive:
- reject: INSERT directly; the partial unique index catches
duplicate active runs; the manager surfaces the result as
ConflictError.
- interrupt/rollback: SELECT FOR UPDATE the conflicting rows,
skip rows whose lease is still valid AND owned by another live
worker (raise ConflictError — the INSERT would have failed on
the index anyway, and a retry loop cannot make progress),
cancel the rest in the same transaction, then INSERT the new
row. Rows owned by this worker are interruptible regardless of
lease state.
- RunManager.create_or_reject dispatches to the store under the
existing local lock; same-worker in-memory cancellation runs after
the store commit succeeds. MemoryRunStore mirrors the same
semantics for tests and database.backend=memory.
Work item 3 — lease heartbeat + Postgres reconciliation
- RunOwnershipConfig (lease_seconds=30, grace_seconds=10,
heartbeat_enabled=false by default), registered as startup-only in
reload_boundary.STARTUP_ONLY_FIELDS because the heartbeat background
task is created once in langgraph_runtime() and is not rebuilt on
config.yaml edits.
- When heartbeat_enabled, each worker renewes leases on its own
active runs with interval = lease_seconds / 3. The loop is bounded
and stop-event-cancellable so shutdown is prompt.
- reconcile_orphaned_inflight_runs now runs on every backend — the
sqlite-only gate in app/gateway/deps.py is dropped in the same
commit so there is no window where Postgres would mis-mark live
Worker A runs as orphans. Reconciliation errors only runs whose
lease is NULL (legacy pre-ownership rows) or older than
grace_seconds. In single-worker mode (heartbeat off, NULL leases)
all inflight rows reclaim immediately, preserving the pre-ownership
recovery latency.
- Heartbeat starts AFTER startup reconciliation and stops BEFORE the
in-flight run drain on shutdown so the two cannot race.
GATEWAY_WORKERS=1 with heartbeat_enabled=false keeps current behavior.
Verified: 170 related tests + full backend suite (minus Docker-gated
live tests) green; ruff check + ruff format clean.
* fix(runs): tighten unique-violation handling and document clock-sync budget
Three follow-up fixes to the cross-process run ownership work in #3948,
surfacing during review.
1. _is_unique_violation: detect by driver-native signal, not message text
The previous substring heuristic ("unique" + "violat", or "duplicate")
missed SQLite's actual phrasing "UNIQUE constraint failed: <table>.<index>"
— SQLite says "failed", not "violates", and never "duplicate". On SQLite
the detector returned False, the reject path re-raised the raw
IntegrityError, and clients saw HTTP 500 instead of ConflictError 409.
The conversion is the load-bearing piece of the "store is source of
truth" design but was untested — every atomic test used MemoryRunStore,
which raises ConflictError directly and never reached this branch.
Now prefers driver-native signals: psycopg pgcode/sqlcode "23505" and
sqlite3 sqlite_errorcode SQLITE_CONSTRAINT_UNIQUE (reachable through
SQLAlchemy IntegrityError.orig). Message matching stays as a fallback
with SQLite's exact "unique constraint failed" phrase added.
2. interrupt/rollback: convert exhausted-retry IntegrityError to ConflictError
The reject branch converts unique violations to ConflictError. The
interrupt/rollback retry loop did not — on the 3rd attempt it re-raised
the raw IntegrityError, leaking HTTP 500 for the same race condition
that reject surfaces as 409. Symmetric conversion added after the loop;
callers now see a consistent ConflictError regardless of strategy.
3. Document clock-sync requirement for multi-worker lease reconciliation
reconcile_orphaned_inflight_runs compares another worker's UTC
lease_expires_at against this worker's datetime.now(UTC). The only skew
budget is grace_seconds (default 10s) — worst case, with the owning
worker's heartbeat just about to fire, a peer whose clock is more than
~grace_seconds ahead can mis-reclaim a still-live run as an orphan.
Documented in RunOwnershipConfig's docstring (with the math) and in
config.example.yaml (with operational guidance), so operators in
NTP-poor environments know to raise grace_seconds. Default unchanged:
10s is reasonable for NTP-synced K8s/cloud, and bumping it would slow
recovery of genuinely dead workers (lease_seconds + grace_seconds from
last heartbeat to reclaim).
Tests:
- test_create_run_atomic_reject_propagates_conflict_on_unique_violation:
end-to-end against a real SQLite-backed RunRepository, pre-inserts an
active run, asserts reject-strategy create surfaces as ConflictError
rather than raw IntegrityError.
- test_is_unique_violation_detects_real_sqlite_integrity_error: unit test
for the detector against a real SQLite-raised IntegrityError; asserts
driver-level sqlite_errorcode is SQLITE_CONSTRAINT_UNIQUE.
- test_interrupt_exhausted_retries_surface_as_conflict_error: pins the
symmetric 409 behavior after the retry loop exhausts.
Verified: ruff check + ruff format clean; multi-worker + run_repository
+ owner_isolation + reload_boundary suites green.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
* fix(runs): close multi-worker ownership gaps in lease heartbeat and unique-violation detection
Five code-review fixes from docs/multi_worker.md:
1. Drop unused ``claim_inflight_runs`` primitive — no caller anywhere.
``create_run_atomic`` does its own inline claim (SELECT FOR UPDATE +
cancel) inside the INSERT transaction; a separate claim primitive
would split that into two transactions and open a claim→INSERT race.
Removes ~40 lines across base.py / memory.py / sql.py plus the
unused ``now_iso`` parameter, freeing future RunStore implementations
from providing it.
2. Broaden ``_renew_leases`` filter to renew pending/running runs owned
by this worker even when ``record.task is None``. The previous
``task is not None`` requirement skipped the brief window between
``create_run_atomic`` inserting the row and the worker spawning the
agent task; under event-loop load that window can approach
``lease_seconds``, after which peer reconciliation marks the run
``error`` (visible) or a peer's ``create_or_reject("interrupt")``
silently kills the queued run. Filter now:
``task is None or not task.done()``.
3. Document the unsynchronised ``record.lease_expires_at = new_expiry``
write. ``lease_expires_at`` is the only field on an existing record
this path mutates; ``set_status`` / ``_persist_status`` touch other
fields, so there is no concurrent writer to race against. Re-acquiring
``self._lock`` would serialise unrelated run mutations for no gain.
4. Gate ``_is_unique_violation`` message fallbacks on
``isinstance(current, (SAIntegrityError, sqlite3.IntegrityError))``.
The driver-code path (pgcode/sqlite_errorcode) remains load-bearing;
substring fallbacks are now belt-and-suspenders only for cases where
the driver attribute isn't reachable through the cause chain. Without
the gate, any application exception whose ``str()`` happens to contain
"duplicate key" / "unique" + "violat" (CHECK constraint, validation
error) would silently surface as HTTP 409 instead of 500.
5. Route ``update_lease`` through ``_call_store_with_retry`` for
consistency with every other store call, and wrap
``await self._renew_leases()`` in ``_heartbeat_loop`` with
``except Exception: logger.warning(...)``. Previously a transient
error from the snapshot path or an unexpected exception would kill
the heartbeat task silently — after which no lease is ever renewed
again and every active run eventually looks orphaned.
``except Exception`` lets ``CancelledError`` (BaseException since
3.8) propagate so shutdown cancellation still works.
Regression tests:
- ``test_heartbeat_renews_pending_run_before_task_is_spawned``
- ``test_is_unique_violation_does_not_misclassify_application_exception``
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
* fix(runs): harden multi-worker migration, memory atomicity, and tz-naive lease comparison
Three follow-up fixes to the multi-worker run ownership work:
- migration 0004 dedupe pass: cancel superseded duplicate active rows per
thread before creating the partial UNIQUE index ``uq_runs_thread_active``
so dirty DBs (Postgres deployments that had reconciliation skipped by the
old sqlite-only gate, or any env that ran GATEWAY_WORKERS>1 before this PR)
do not abort the alembic upgrade and block gateway startup. Keeps the
newest active row per thread, marks the rest as error with an explanatory
message.
- MemoryRunStore.create_run_atomic interrupt/rollback path: split the single-
pass loop into two passes (collect candidates, validate, then mutate) so a
ConflictError raised on a later candidate does not leave earlier candidates
half-interrupted. Mirrors the SQL store's transactional rollback semantics;
the entire test_multi_worker_run_ownership.py suite runs against memory so
this divergence was giving false confidence.
- RunRepository.create_run_atomic interrupt path: coerce tz-naive
``row.lease_expires_at`` to UTC before comparing against the aware
``cutoff``. SQLite drops tzinfo on read despite ``DateTime(timezone=True)``
(this file's own comment acknowledges it), so the Python-side comparison
raised ``TypeError: can't compare offset-naive and offset-aware datetimes``
whenever heartbeat was enabled on SQLite and a lease was non-NULL. Defaults
(heartbeat off -> leases always NULL) masked it, but there was no guard
against the combination. Follows the existing "naive is UTC" convention
from ``coerce_iso``.
Each fix ships with a regression test pinning the behavior.
Co-Authored-By: heart-scalpel <heart-scalpel@users.noreply.github.com>
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
* fix(runs): enforce heartbeat for multi-worker, fix memory-store datetime comparison, lazy-import ConflictError in store layer
Three fixes from code review:
1. Extend the startup gate (GATEWAY_WORKERS>1) to also require
run_ownership.heartbeat_enabled=true. Without heartbeat every run has
a NULL lease, so reconciliation treats all inflight rows as orphans
and Worker B would kill Worker A's live runs on every rolling update
or scale-up.
2. Fix MemoryRunStore.list_inflight_with_expired_lease to parse
created_at as datetime instead of ISO string lexical comparison,
and handle tz-naive lease values uniformly with the SQL store.
3. Store layer (sql.py, memory.py) now lazy-imports ConflictError
inside create_run_atomic instead of importing from the higher
RunManager layer at module level.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
* fix(runs): add owner check to update_lease, document create() assumption, restore deleted comment
- update_lease (SQL + memory) now requires owner_worker_id match in WHERE
clause so the primitive is safe by construction against misuse
- create() docstring notes it bypasses atomic create_run_atomic and
assumes no active run exists for the thread
- restore explanatory comment in MemoryRunStore.aggregate_tokens_by_thread
that was dropped in an earlier commit
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
* fix(runs): add psycopg3 sqlstate detection and periodic orphan reconciliation
- _is_unique_violation now checks sqlstate attribute (psycopg3 uses this
instead of pgcode). On Postgres, the only supported multi-worker backend,
detection was falling through to the message-substring fallback.
- _heartbeat_loop now runs reconcile_orphaned_inflight_runs every 3rd
cycle (every lease_seconds) to catch orphans whose lease expires between
pod restarts. Single-worker deployments are unaffected (heartbeat off).
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
Co-authored-by: heart-scalpel <heart-scalpel@users.noreply.github.com>
Co-authored-by: Willem Jiang <willem.jiang@gmail.com>
* feat(subagents): show runtime metadata on task cards
* fix(subagents): stop task-card render loop and dedupe model fetches
Address code review on the runtime-metadata cards:
- P1 render loop: the terminal ToolMessage is re-parsed on every
MessageList render and always carries modelName/usage, so the
presence-based setTasks condition fired a fresh state object each
render -> "Maximum update depth exceeded". computeNextSubtask now
returns a value-compared `changed` flag and a pure subtaskNotification()
routes terminal transitions through the deferred after-render path
while skipping no-op re-parses.
- Per-card useModels refetch: add staleTime: Infinity to the ["models"]
query so every subtask card shares one /api/models fetch instead of
refetching on each mount.
* make format
* refactor(subagents): dedupe token-usage validators + tidy event narrowing
Address PR review follow-ups:
- DRY: extract one shared token-usage validator per side. Backend
status_contract.normalize_token_usage() now backs both the terminal
ToolMessage metadata and the subagent.step/.end run events
(step_events.py), and frontend messages/usage.normalizeTokenUsage()
backs both the live task_running event (lifecycle.ts) and the terminal
ToolMessage metadata (subtask-result.ts). Prevents the input/output/
total_tokens validation from drifting across the four former copies.
- Nit: onCustomEvent narrows event.type once instead of re-checking the
object shape per branch; the redundant task_started early-return
(already validated by taskEventToSubtaskUpdate) is dropped.
* fix(subagents): classify LLM error fallbacks as failed (#4041)
* fix(subagents): address #4042 review on LLM error fallback
- clarify _extract_llm_error_fallback docstring: tail-only scan is safe
because subagents append their own terminal message, so the last
AIMessage is never a stale parent-history marker (cross-ref worker.py)
- compute final_result and pop stop_reason only on the COMPLETED branch
so the guard stop-reason pop no longer fires on the discarded FAILED path
- note the error_detail/"LLM request failed" fallbacks are defensive;
the middleware always populates a non-empty content
- add regression test locking the stale parent-history marker invariant
---------
Co-authored-by: Willem Jiang <willem.jiang@gmail.com>
* fix(security): html-escape fact content in memory prompt sections
Raw memory fact content was injected verbatim into prompt XML — a fact
containing a literal `"` could break the `"..."` delimiter, and a
closing tag like `</consolidation_candidates>` could prematurely end
the XML block, both potentially confusing the model.
Apply `html.escape()` to `content` in `_build_staleness_section` and
`_build_consolidation_section`, and to `cat` in the consolidation
section's XML attribute. Tests added for both sections covering special
characters, XML tag injection, and attribute injection.
Follow-up to #3996 as noted by reviewer willem-bd.
* fix(security): address reviewer follow-ups on html-escaping PR
- Escape `cat` in _build_staleness_section for symmetry with the
consolidation section (both sections now consistently html-escape
all LLM-derived category values that appear in the prompt)
- Add comment at current_memory=json.dumps() documenting the conscious
accept: json.dumps leaves < > & unescaped; lower-risk than
staleness/consolidation (read-only context, not delete/merge
instructions); fix at fact-content insert time if revisited
- Add test for category escaping in the staleness section
* fix(security): reference tracking issue #4044 in conscious-accept comment
* style: compress conscious-accept comment to two lines
* fix(subagents): inject durable context before compaction
* fix(subagents): coalesce system messages after durable-context injection
Address #4040 review:
- append SystemMessageCoalescingMiddleware innermost on the subagent chain
so the SystemMessage(authority) DurableContextMiddleware injects is merged
into one leading system_message; otherwise the durable fix trades #4039's
assistant-first 400 for a duplicate-system 400 on strict backends
- add a two-system regression guard driving the real builder output through
a strict model; assert exactly one leading SystemMessage
- assert single-leading-system in the compaction integration test too
- update the middleware count/last-element assertion (coalescer is now
unconditionally last, removing the summarization-dependence ambiguity)
- compare _skills_root against posixpath.normpath(container_path)
- document the coalescer on the subagent chain in backend/AGENTS.md
* Fix circuit breaker wedging after a non-retriable half-open probe
When the circuit breaker is half-open it admits a single probe call by
setting `_circuit_probe_in_flight = True`. If that probe raised a
*non-retriable* error (e.g. quota/auth), the except block skipped both
`_record_failure()` (correct - business errors must not trip the breaker)
and any probe reset, so the circuit stayed `half_open` with
`_circuit_probe_in_flight = True` permanently. Every later call then
fast-failed in `_check_circuit()` forever, because no call could run the
handler to reach `_record_success` / `_record_failure`.
Release the probe on the non-retriable path (mirroring the existing
GraphBubbleUp handler) so the next call admits a fresh probe. The breaker
still never trips on non-retriable errors. Applied to both the sync and
async paths.
Adds sync + async regression tests asserting the probe is released and the
next `_check_circuit()` re-admits a probe.
* Address review: extract _release_half_open_probe helper
`format_memory_for_injection` bound `facts_header` / `all_fact_lines` only
inside the `if isinstance(facts_data, list) and facts_data:` block, but the
structure-aware overflow-truncation path at the end of the function
references both unconditionally.
When a user's memory has sizeable user-context / history (so `sections` is
non-empty and the assembled output exceeds `max_tokens`) but an empty or
missing `facts` list, that block is skipped, so the truncation branch hits
`UnboundLocalError: cannot access local variable 'all_fact_lines'` and
aborts memory injection entirely.
Hoist the two initializers to function scope, alongside the existing
`guaranteed_line_tokens = 0`, so they are always bound. Behaviour is
unchanged when facts are present.
Adds a regression test (empty facts + oversized user context) that fails
with UnboundLocalError before the fix and truncates gracefully after.
`before_agent` guards `context = runtime.context or {}` at the top, but the
`run_id` stamp on a trailing HumanMessage still read the raw `runtime.context`,
so a None context (thread_id resolved from `config.configurable`) plus a
HumanMessage last message raised `AttributeError: 'NoneType' object has no
attribute 'get'`. Use the guarded local `context` instead.
Adds a regression test.
* fix(sandbox): guard the reverse path-translation regex with a segment boundary
_reverse_output_patterns matches a mount's resolved local root with no
segment-boundary lookahead, unlike every other prefix match in this file: both
forward regexes carry one, and the three string-prefix resolvers compare against
`root + "/"`. Its optional trailing group needs a `/` or `\` to consume more, so
against a sibling that merely shares the prefix (".../skills-extra/data.txt") the
group matches empty and the regex still yields the bare root.
That extracted text then *equals* the mount root, which satisfies
_reverse_resolve_path's own `+ "/"` guard -- so the sibling is rewritten to
"/mnt/skills-extra/data.txt". Forward resolution refuses to map that back, so the
model is handed an absolute-looking container path it can never read. This runs
over every bash stdout/stderr and over read_file content.
Mirror _content_pattern's boundary class rather than _command_pattern's: this
regex sees arbitrary command output, where a root can legitimately be followed by
"," ":" or "\" -- all of which the shell-oriented class would reject, silently
dropping translations that work today. The trailing group keeps [/\\] so Windows
paths still match.
* test(sandbox): pin the end-of-output boundary that stops a host-path leak
The lookahead's `$` alternative had no test: deleting it left all 6866
backend tests green. It is not cosmetic. Output ending exactly at a mount
root -- printf '%s' "$PWD", a stripped last line, a capture truncated at
the buffer limit -- matches neither `/` nor `[^\w./-]`, so the pattern
fails and _reverse_resolve_paths_in_output emits the raw host path to the
model. main's unbounded pattern has no end-of-string problem; the
narrowing added here is what introduced the exposure.
Pin it: three prefixes, asserting both the container path and that the
host root is absent from the output. Deleting only `$` turns exactly
those three red.
Lift the boundary class and trailing group into named locals. The
compiled pattern is byte-identical; correctness hinges entirely on the
boundary class, so it should not sit at column 120 of a comprehension.
The apply-time staleness guardrail built ``candidate_ids`` with a direct
``f["id"]`` access over ``_select_stale_candidates`` output:
candidate_ids = {f["id"] for f in _select_stale_candidates(current_memory, config)}
Every other fact access in ``updater.py`` uses ``f.get("id")``; this was the
lone direct-subscript outlier. An aged, non-protected fact that lacks an
``id`` key — common in legacy / hand-edited / migrated ``memory.json`` — is a
valid staleness candidate, so it reached ``f["id"]`` and raised
``KeyError: 'id'``, aborting the entire background memory-update cycle for
that user. The guardrail runs unconditionally (independent of the
``staleness_review_enabled`` flag), so any id-less aged fact triggers it as
soon as the LLM returns a non-empty ``staleFactsToRemove``.
Skip id-less candidates when building the intersection set. They can never
be targeted by the id-based removal set anyway, so behaviour is otherwise
unchanged.
Adds a regression test with an aged, id-less fact that raises KeyError
before the fix and applies cleanly after.
Two memory-config docs disagreed with the actual defaults/behavior in
`config/memory_config.py` and `agents/memory/storage.py`:
- backend/AGENTS.md: `staleness_age_days` was documented as "default: 180;
range: 1-3650" but the field is `default=90, ge=30, le=365`;
`staleness_max_removals_per_cycle` was documented as "default: 5; range:
1-20" but the field is `default=10, ge=1, le=50`.
- config.example.yaml: the `storage_path` comment said "Path relative to
backend directory", but a relative `storage_path` resolves against the data
`base_dir` (`get_paths().base_dir / p`), not the backend working directory,
and an absolute path is what opts out of per-user isolation (matching the
field's own description in `memory_config.py`).
Doc-only change; no behavior change.
* fix(sandbox): scrub abbreviated *_PASS vars and Postgres PGPASSFILE from skill env
env_policy blocks the full PASSWORD/PASSWD spellings but not the ubiquitous
abbreviated _PASS form (DB_PASS, SMTP_PASS, MYSQL_PASS, ...), whose value is the
plaintext password itself, so every skill subprocess inherited it. Postgres's
file-based credential sources PGPASSFILE (.pgpass locator) and PGSERVICEFILE
(pg_service.conf, may carry a password) were likewise missed -- the psql analog
of the MYSQL_PWD/REDISCLI_AUTH no-flag sources #4018 added.
Collapse *PASSWORD*/*PASSWD* into a single *PASS* pattern (which subsumes both,
covers the abbreviated family, and catches PGPASSFILE) and add PGSERVICEFILE to
the exact-name denylist. *PASS* deliberately does not touch PWD/OLDPWD, which
carry no PASS substring. Capability-handle vars (KUBECONFIG, DOCKER_CONFIG,
GIT_ASKPASS, ...) are a broader, non-value-bearing class and are left out of
this fix.
test_secret_like_names_are_blocked already pins the contract; the new names are
added to its parametrize list (red before, green after) and the benign-names
test guards against over-scrubbing PWD/OLDPWD.
* test(sandbox): pin the *_ASKPASS over-scrub that *PASS* introduces
*PASS* also matches the credential-helper vars GIT_ASKPASS, SSH_ASKPASS and
SUDO_ASKPASS, which main inherits. Each names a program whose purpose is to hand
the caller a credential, so inheriting the pointer is the same leak class as
inheriting the value -- a skill can simply invoke it. Scrubbing them is intended;
pin all three in test_secret_like_names_are_blocked (red on main) so the
behaviour is a decision rather than a side effect of the pattern's shape.
test_benign_names_are_allowed could not have caught this: none of its 14 names
carries a PASS substring, so it passes identically before and after the
broadening and asserts nothing about it. Record that in its docstring -- the
absence of a benign PASS-bearing name is a decision, and PWD/OLDPWD are the
boundary the list does pin.
Also note the incidental COMPASS_*/BYPASS_* over-scrub in the module comment
(over-scrubbing is this module's fail-safe direction; required-secrets is the
escape hatch), and list PGPASSFILE alongside PGSERVICEFILE in AGENTS.md.
* feat(memory): add memory consolidation to synthesize fragmented facts
When a fact category accumulates many individual entries, the LLM
reviews them during the normal memory-update call (same invocation,
no extra API cost) and decides whether groups of related facts can be
synthesized into a single richer fact. This completes the memory
lifecycle: extraction → guaranteed injection → staleness review →
consolidation.
- Select fragmented categories by min-facts threshold, surface the most
fragmented groups first; prompt-layer caps aligned with apply-layer
guardrails so the LLM never sees groups it cannot act on
- Cap consolidated confidence at source maximum to prevent inflation;
reject results below fact_confidence_threshold
- Double-consume protection prevents a fact from being merged into
multiple consolidation targets
- Feature-gated at both prompt and apply time with per-cycle safety caps
- Add 26 tests covering candidate selection, normalization, apply
guardrails, and prompt integration
* fix(memory): address consolidation correctness issues from PR review
Six fixes based on maintainer review of #3996:
1. Deduplicate sourceIds in normalization — ["f1","f1"] previously
bypassed the ≥2-distinct-sources check; dict.fromkeys collapses it
to ["f1"] which is correctly rejected.
2. Run consolidation after max_facts trim — previously, sources were
deleted then the merged fact could be evicted by the trim, leaving
no record of either. Moving consolidation last ensures source facts
exist in the post-trim index before removal.
3. Fix count= attribute in consolidation prompt — advertised the full
category size but listed only max_sources IDs; now uses
min(len(group), max_sources) to match what the LLM can act on.
4. Exempt staleness_protected_categories from consolidation candidates
— mirrors the existing staleness-review contract so correction facts
are never surfaced for merging.
5. Strip and default category in consolidation normalization — " " or
" preference " are now normalised, matching _normalize_memory_update_fact.
6. Propagate sourceError from source facts into consolidated fact —
correction context is no longer silently lost on merge.
* fix(memory): add apply-time guardrails and tests for consolidation
P1: mirror the staleness-pass defense-in-depth pattern — build
allowed_source_ids from _select_consolidation_candidates at apply time
so a protected-category or below-threshold fact proposed by the LLM is
rejected regardless of model behavior.
P2a: test that LLM-returned confidence is capped at max source confidence
and that a capped result below fact_confidence_threshold is rejected.
P2b: test that factsToConsolidate with consolidation_enabled=False is a
no-op at apply time (35 tests, all pass).
* fix(memory): address three correctness issues from second review round
1. Default consolidation_enabled=False — consolidation is lossy (source
content is permanently replaced, only consolidatedFrom IDs preserved);
new lossy features default to off. config.example.yaml updated to match.
2. Unify confidence coercion between prompt and apply — _build_consolidation_section
now calls _coerce_source_confidence(fact) instead of an inline 0.0-default
coercion, so a null-confidence fact renders with 0.50 in the LLM prompt and
is capped at 0.50 at apply time (same value, same function).
3. Preserve staleness clock on merge — consolidated fact now carries the
newest source's createdAt (not now) so aged information does not gain a
fresh staleness-review window just by being consolidated; consolidatedAt
is added as an explicit audit field.
Three regression tests added (default=false, null-confidence consistency,
createdAt policy); all guardrail tests now set consolidation_enabled=True
explicitly so they test the guardrail, not the feature flag. 38 tests pass.
* fix(memory): harden createdAt comparison and confidence handling
1. createdAt max via _parse_fact_datetime — replaces string max() which
crashes on non-string createdAt (numeric unix timestamps) and sorts
Z/+00:00 mixed formats incorrectly. Mirrors how staleness computes age.
2. Remove dead min(..., 1.0) — _coerce_source_confidence already clamps
each source confidence to [0, 1], so max(source_confidences) ≤ 1.0
by contract; the outer min could never bind.
3. Clamp raw_llm_conf to [0, 1] before applying the source cap — out-of-
range values like 1.5 are safe today (pinned by the cap) but defensively
clamped first so the invariant holds even if the cap is ever loosened.
4. Doc: expand the apply-time guardrails comment to call out the protected-
category exclusion via allowed_source_ids — this is the central safety
property ("explicit user feedback is never silently merged away").
5. Test: add test_confidence_fallback_to_max_source_when_llm_omits_field
covering the else-branch (LLM omits confidence → uses max_source_conf).
6. Fix lint: reorder imports in test file (stdlib before third-party).
39 tests, all pass.
---------
Co-authored-by: Willem Jiang <willem.jiang@gmail.com>
Phase 3 of #3875 — subagents previously inherited none of the lead's
context-compaction, so a deep-research subagent (max_turns up to 150)
could accumulate >1M cumulative input before max_turns/timeout/token_budget
engaged, even after Phase 2's budget capped the pathological tail.
- Gate the subagent runtime chain on the SAME ``app_config.summarization.enabled``
switch the lead reads (per maintainer guidance in #3875), via the shared
``create_summarization_middleware`` factory. One config covers both chains;
no separate ``subagents.summarization`` field. No-op when summarization is
off (factory returns None).
- ``skip_memory_flush=True`` on the subagent path: the factory otherwise
attaches ``memory_flush_hook`` (when memory.enabled), which flushes
pre-compaction messages into durable memory keyed by thread_id. Subagents
share the parent's thread_id, so without skipping the hook a subagent's
internal turns would pollute the PARENT thread's durable memory
(#3875 Phase 3 review point).
- Harden ``capture_new_step_messages`` to tolerate history contraction:
summarization rewrites the messages channel via
``RemoveMessage(id=REMOVE_ALL_MESSAGES)``, shrinking len(messages) below
the step-capture cursor. Without a reset, every step appended after the
compaction point was dropped until length overtook the stale cursor (#3845
interaction, maintainer validation point (a)). Cursor now resets to the
new tail; id/content dedup prevents re-emitting pre-compaction steps.
- Couple the DEFAULT token-budget ceiling to ``summarization.enabled``
(#3875 Phase 3 review point): 1M when compaction is on, 2M when off
(preserves Phase 2's deliberate headroom for summarization-off
deep-research runs that can exceed 1M). A user-set budget (global or
per-agent) always wins regardless of the switch. Flagged tunable.
The summarization middleware does not implement ``consume_stop_reason``, so
the Phase 2 guard-cap stop-reason channel is unaffected.
Refs: https://github.com/bytedance/deer-flow/issues/3875
`image_search` set both `image_url` and `thumbnail_url` to the DDGS result's
`thumbnail` field, so the tool never returned the full-resolution image even
though its docstring/usage_hint promise reference-quality images (and a result
with no `thumbnail` returned an empty `image_url`). DDGS `.images()` exposes the
full-res source under the separate `image` key — read that for `image_url`,
matching the serper/brave providers which keep image vs thumbnail distinct.
Adds a regression test.
* fix(agent): snap tool-output tail forward so fallback truncation respects max_chars
_snap_to_line_boundary() moves an offset backwards to the preceding newline.
That is correct for the head's end offset, but _build_fallback() also applied it
to the tail's start offset, where moving backwards lengthens the tail. On output
whose last newline sits in the second half (log lines followed by one long
unbroken line: minified JSON, base64 artifacts), the returned string exceeded
max_chars by up to 17x, defeating the guard that exists to stop a single large
tool result from blowing the model context.
Add _snap_start_to_line_boundary(), the forward-snapping mirror, and use it for
the tail offset. The existing head_end guard becomes redundant because a forward
snap can only move the tail start away from the head.
test_result_never_exceeds_max_chars already asserted this invariant but passed
newline-free content, so the snapping branch was never exercised.
* test(agent): pin the forward-snap direction of the fallback tail
The bound test only asserts that the result fits max_chars. Its content has
no newline inside the tail's snap window, so _snap_start_to_line_boundary
returns pos unchanged and the test stays green even with the snap removed.
Place a newline in the window so the snap has to fire: the tail must begin
after it, which the pre-fix backward snap does not do.
* feat(mcp): auto-promote deferred MCP tools from routing hints
When tool_search.enabled=true defers MCP tool schemas, PR1 routing hints
still require the model to spend a tool_search discovery round trip before
it can call the tool the routing metadata already points at. This adds a
McpRoutingMiddleware that matches the latest user message against PR1
routing keywords and promotes the matching deferred schemas before the
model call, removing that round trip.
Design (soft routing, opt-in, additive):
- Matches only the latest real HumanMessage (shared is_real_user_message
helper, reused by SkillActivationMiddleware so the two cannot drift);
case-insensitive substring match, no tokenizer dependency.
- Ordering: priority desc, then tool name asc; capped by the new global
tool_search.auto_promote_top_k (default 3, clamped 1..5). Does not add or
consume a per-tool auto_promote_top_k (PR1 schema unchanged); a per-tool
value is ignored with a DEBUG note.
- Returns a plain {"promoted": ...} state update (not a Command) and relies
on ThreadState.merge_promoted for union/dedupe, so auto-promote and a
model-triggered tool_search converge on the same catalog hash.
- Installed before DeferredToolFilterMiddleware on every deferred-tool path
(lead agent, subagent, embedded client, webhook via shared builders);
a construction-time assert rejects the reversed order. catalog_hash is
None / no routing index is a complete no-op, so bootstrap and ACP skip it.
- Privacy: never executes tools, never promotes policy-filtered tools, adds
no routing keywords or matched tool names to trace metadata or INFO/WARN
logs.
No behavior change when tool_search.enabled=false.
Tests: index construction, matching semantics, middleware state updates,
same-cycle deferred-filter interaction, lead/subagent/embedded-client
builder wiring + order invariant, config clamping, config.example.yaml
parseability, and privacy assertions.
* refactor(mcp): address auto-promote review nits
- executor: access app_config.tool_search.auto_promote_top_k directly to match
the lead-agent and embedded-client paths (drop the over-defensive getattr that
masked missing config); update the subagent test mock to carry tool_search.
- tool_search / mcp_routing_middleware: cross-reference the duplicated routing
priority/keyword normalization between the builder and the middleware's
defensive _normalize_index so they cannot silently drift.
- MCP_SERVER.md: document that auto-promote keyword matching is a case-insensitive
substring test (not word-boundary), advising distinctive keywords.
* fix(sandbox): scrub MYSQL_PWD and REDISCLI_AUTH from the inherited skill env
env_policy blocks the MySQL and Redis connection strings (MYSQL_URL, REDIS_URL)
because they embed a password, but not the password variables the same clients
read directly: mysql/libmysqlclient takes MYSQL_PWD and redis-cli takes
REDISCLI_AUTH with no further configuration. Both names carry no
KEY/SECRET/TOKEN/PASSWORD/PASSWD substring, so they were inherited by every
skill subprocess -- the leak surface this module exists to close (#3861).
They need exact entries rather than a pattern: *PWD* would also strip PWD and
OLDPWD. REDIS_AUTH is included as the common non-canonical variant.
* Apply suggestions from code review
- Distinguish REDIS_AUTH (defensive, non-canonical) from MYSQL_PWD and
REDISCLI_AUTH (documented no-flag credential sources) in the comment.
- Pin that request-scoped injection still overrides the host value for the
newly blocked names, keeping required-secrets a working escape hatch.
* bench: add provider-agnostic sandbox benchmark with BoxLite warm pool results
- scripts/bench_sandbox_provider.py: CLI for measuring acquire/run/release
across providers, scenarios, workloads, and concurrency levels
- scripts/summarize_bench.py: JSONL aggregation with p50/p95/p99 tables
- bench_results.jsonl: 110 turns across 7 scenarios on real BoxLite 0.9.7
Key findings:
cold acquire: ~860ms
warm reclaim: ~14ms (60x speedup)
release: ~0ms
warm_hit_rate: 95% (warm_same_thread)
* perf(boxlite): skip health check for recently-released warm pool boxes
Boxes released within health_check_skip_seconds (default 5.0s)
are promoted directly without the ~14ms echo-ok round-trip.
A VM alive seconds ago is overwhelmingly likely to still be alive.
Add sandbox.health_check_skip_seconds config option.
Set to 0 to always health-check (old behaviour).
Benchmark (warm_same_thread, noop, 20 iters):
acquire p50: 14.9ms → 0.0ms
total p50: 29.9ms → 14.0ms
* chore: move benchmark scripts into backend/scripts/benchmark/
* fix: address BoxLite benchmark review findings
* fix(boxlite): only skip warm reclaim checks for released boxes
* fix(benchmark): keep BoxLite shim workaround off the event loop
* fix(boxlite): invalidate dead boxes from command path
* test(boxlite): cover skip window and invalidation edge cases
* fix(boxlite): treat sandbox-has-been-closed as terminal in _exec
* fix(boxlite): harden warm-pool reclaim and benchmark accounting
* fix(boxlite): validate warm-pool reclaims by default
* fix(config): expose boxlite health-check skip setting
* fix(boxlite): tighten failure classification and benchmark workaround
* Update config_version to 21 in values.yaml
---------
Co-authored-by: Willem Jiang <willem.jiang@gmail.com>
The tag-publish workflow (container.yaml) built backend/Dockerfile with no
build-args, so UV_EXTRAS was empty and the published *-backend image shipped
without the Postgres driver (only --extra redis). Multi-replica deployments
(K8s/Helm) that need shared Postgres persistence instead of file-based SQLite
could not use the release image without rebuilding it.
Pass UV_EXTRAS=postgres so the release image includes
deerflow-harness[postgres]. Additive only: single-replica sqlite/redis setups
keep working; the Postgres driver is added, mirroring how redis is already
always baked in.
