Branch creation seeds the new thread's run-event feed from its checkpoint
so inherited history survives the first run (#4380). Every seeded row
carried one shared run id, but run_id is a *turn* identity to the feed's
consumers, not a provenance tag: regenerating the inherited answer
resolves that row's run id as the superseded source, and
GET /messages/page then drops every row carrying it. One shared id for
the whole seed therefore deleted the complete inherited history on a
branch's first regenerate, leaving only the regenerated turn.
Group seeded rows into one synthetic run per inherited turn
(branch-seed-{thread_id}-{n}), a new turn opening at each persisted human
message — the same boundary a real run has, including the allowlisted
hidden ask_clarification reply, which resumes as its own run. Supersession
is then confined to the turn actually regenerated.
Co-authored-by: Willem Jiang <willem.jiang@gmail.com>
Frontend unit tests run in a plain node environment, so nothing that
renders can be covered and no hook can be tested under real React. The
workaround that forces is already in the tree: the use-global-shortcuts
test mocks out react itself, so its useEffect never re-runs on a
dependency change and never goes through React's commit/cleanup
ordering -- it pins the stub rather than the hook.
Split the suite into two rstest projects: *.test.ts(x) keeps running on
node, *.dom.test.ts(x) runs on happy-dom. A global DOM environment was
measured first and rejected -- it takes the same 743 tests from 3.3s to
9.5s -- and rstest has no per-file environment docblock, so projects is
the only way to charge that cost to the tests that need it.
useIsMobile is the first consumer: it had no tests and cannot have any
without a document, since it reads window.matchMedia.
* fix(summarization): own the run model for compaction; bound failure
With summarization.model_name: null the summary model resolved to
config.models[0] while the executing model is selected per run; when they
differ and models[0]'s provider is broken (expired key, quota, outage)
compaction silently failed every triggered turn and context grew unbounded
until the main provider 400s the run (#3103's shape), even though the run's
own model was healthy.
Model ownership is now sourced from the builders, not re-derived at runtime:
- The lead, subagent, and manual /compact builders each pass the resolved run
model into create_summarization_middleware(run_model_name=...). The middleware
no longer reads runtime.context / get_config(), which do not carry a custom
agent's or a subagent's resolved model, so a custom-agent lead run and a
distinct-model subagent now summarize with their own model, not models[0] /
the parent's. Runtime re-resolution and the per-name model cache are removed.
- model_name: null summarizes with the run's own model; an explicitly configured
summary model generates and falls back to the run model on failure. The
fallback is built lazily after the primary fails and its construction is
guarded, so a broken fallback cannot skip a healthy primary or escape the
automatic failure boundary.
Failure is bounded and side-effect-safe:
- An empty or whitespace-only response is treated as a generation failure, not a
valid summary, so compaction never removes all history for an empty replacement.
- compact_state/acompact_state take raise_on_failure independent of force: the
manual /compact path always surfaces a generation failure (even force=false)
and routes it to the existing ContextCompactionFailed path (HTTP 500 ->
frontend error toast) instead of an unconsumed response reason. The automatic
path leaves compaction state unchanged.
- before_summarization hooks fire only after a replacement summary exists.
SummarizationConfig.model_name, config.example.yaml, and docs/summarization.md
document the final lead/subagent/manual ownership rules.
Part of RFC #4346 (section A). Evaluating fraction/triggers against the run
model's profile (profile ownership) is a separate follow-up.
* fix(summarization): manual /compact model ownership + fail-open construct/parse
Manual /compact carried only agent_name, so it derived the run model from the
custom-agent model or config.models[0] and missed the request-selected model the
run path uses (request -> custom-agent -> default). Carry model_name through
ThreadCompactRequest and the frontend compact call, resolve with the same
precedence, and move the custom-agent config read off the event loop (asyncio
.to_thread) with user_id so the strict blocking-IO gate is not bypassed by the
broad except.
Make one summary attempt own its full lifecycle so the fail-open boundary covers
construction and response parsing, not just invocation: build each candidate model
lazily and guarded (a raising constructor falls through to the healthy run model
instead of breaking agent construction), build the model_name:null primary from the
run model rather than config.models[0], and run response text extraction inside the
invocation try so a failing .text accessor falls back instead of escaping compaction.
Adds factory-level constructor-failure, response-extraction-failure (sync/async), and
route-path model-ownership tests.
Queue comments received while a run is active, then submit one deduplicated follow-up after it finishes. Failed drains are requeued and watcher tasks stop cleanly with the channel manager.
Sync the harness docs with the #3875 subagent middleware changes:
- subagents.mdx (en/zh): fix built-in max_turns defaults (general-purpose
160->150, bash 80->60) to match code and config.example.yaml; document the
new config (global + per-agent override) and add a
'Runaway guards' section covering the LoopDetection / TokenBudget /
Summarization middlewares now mirrored on the subagent chain.
- middlewares.mdx (en/zh): note that loop-detection, token-budget, and
summarization guards are shared with the subagent chain (#3875), instead of
implying they are Lead-Agent-only.
Code, contracts/subagent_status_contract.json (v2), and config.example.yaml
already reflect #3875; only the docs were lagging.
* fix(skills): offload blocking filesystem IO in get_custom_skill_history
The GET /api/skills/custom/{name}/history handler ran its storage probes and the
per-skill .history read directly on the asyncio event loop:
get_or_new_skill_storage(), custom_skill_exists(), get_skill_history_file().exists()
and read_history() are all blocking filesystem IO. make detect-blocking-io flagged
the existence probe (routers/skills.py:224) as DIRECT_ASYNC.
Move the whole read into a nested sync function run via asyncio.to_thread; a None
return signals 404 (distinct from an empty history list). Behavior is unchanged.
Per the blocking-io-guard SOP:
- Candidate: get_custom_skill_history (FILE_METADATA, DIRECT_ASYNC) -> FIX+ANCHOR.
- Re-scan: the finding no longer appears for this handler.
- Anchor: tests/blocking_io/test_skills_router.py drives the real handler against a
real on-disk skill + history; teeth verified red (pre-fix) -> green (post-fix)
under make test-blocking-io.
Scoped to this self-contained read handler. rollback_custom_skill and update_skill
also touch blocking IO but interleave it with awaits (security scan / cache refresh)
and do a read-modify-write, so offloading them needs the asyncio.Lock serialization
treatment (cf. #3552) and is left as a separate fix unit.
* test: trim dead skills history setup
* fix(skills): use the user-scoped storage accessor in the offloaded history read
The merge with main left the offloaded reader calling get_or_new_skill_storage,
which is not defined in this module (ruff F821), so lint failed and the handler
would raise NameError at runtime. Use _get_user_skill_storage(config) — the same
accessor every other handler in this router uses.
Also update the regression test for the current route signature: the handler is
now admin-only and takes a Request, so the test supplies request.state.user
(mirroring tests/blocking_io/test_channel_runtime_config_store.py) and seeds the
history through the same user-scoped accessor.
---------
Co-authored-by: ly-wang19 <ly-wang19@users.noreply.github.com>
Co-authored-by: Willem Jiang <willem.jiang@gmail.com>
* fix(subagents): align prompt and middleware subagent limit; allow min of 1
SubagentLimitMiddleware clamped max_concurrent to [2, 4] internally, but
agent.py and client.py fed the raw config value into the system prompt, so
a user-configured 1 (or 5) produced a prompt that disagreed with the
enforced middleware limit. Lower MIN_SUBAGENT_LIMIT to 1 and clamp the raw
config value with _clamp_subagent_limit() at both the agent factory and the
embedded client so the prompt and middleware see the same value.
* fix: remove unused imports MAX_CONCURRENT_SUBAGENT_CALLS, MIN_CONCURRENT_SUBAGENT_CALLS, clamp_subagent_concurrency
* fix: harmonize clamp range [1,4] across middleware, config, and prompt path; fix lint
- Changed MIN_CONCURRENT_SUBAGENT_CALLS from 2 to 1 so prompt.py's
clamp_subagent_concurrency and the middleware's _clamp_subagent_limit
both clamp to [1,4] — eliminating the divergence where the prompt told
the model 'max 2 task calls' but the middleware enforced 1.
- Applied _clamp_subagent_limit at build_middlewares (agent.py:360) so
all 3 construction sites (agent.py:360, agent.py:450, client.py:259)
consistently clamp the config-resolved limit.
- Derived MIN_SUBAGENT_LIMIT / MAX_SUBAGENT_LIMIT from
MIN_CONCURRENT_SUBAGENT_CALLS / MAX_CONCURRENT_SUBAGENT_CALLS so the
two module-level definitions stay in sync.
- Added TestConfigParity.test_prompt_path_and_middleware_clamp_agree
regression test.
- Fixed lint.
* fix(lint): add missing imports for MIN_CONCURRENT_SUBAGENT_CALLS and MAX_CONCURRENT_SUBAGENT_CALLS
* docs+test: update AGENTS.md clamp range to 1-4; add prompt/middleware parity regression test
- backend/AGENTS.md still documented the old [2,4] clamp in two places;
updated to [1,4] to match MIN_CONCURRENT_SUBAGENT_CALLS = 1.
- Added test_apply_prompt_template_single_subagent_limit_matches_middleware:
renders the real system prompt with max_concurrent_subagents=1 and asserts
the advertised HARD LIMITS value equals SubagentLimitMiddleware's enforced
max_concurrent — the end-to-end check that would have caught the [1,4] vs
[2,4] prompt-path divergence flagged in review.
* refactor: simplify per review — restore clamp delegation, drop redundant call-site clamps
Per willem-bd's review, reduce the PR to the one behavioral change plus
docs/tests:
- _clamp_subagent_limit delegates to clamp_subagent_concurrency again
instead of inlining a byte-identical copy; with a single source of
truth the TestConfigParity sync-check class is unnecessary — dropped.
- Revert the call-site clamps in agent.py (build_middlewares,
_make_lead_agent) and client.py (_ensure_agent) to main: both
downstream consumers (SubagentLimitMiddleware.__init__ and the prompt
path) already clamp internally, and the cross-module private import
of _clamp_subagent_limit goes away with them.
- Keep MIN_CONCURRENT_SUBAGENT_CALLS = 1 (the fix), the [1, 4]
docstring updates, the AGENTS.md range corrections, and the
end-to-end prompt/middleware parity test for single-subagent mode
(docstring reworded: on main a configured 1 was bumped to 2 by both
paths — there was no divergence to fix, just a silently raised floor).
* test: fix stale comment referencing reverted agent.py/client.py call-site clamps
---------
Co-authored-by: nankingjing <nankingjing@users.noreply.github.com>
Co-authored-by: Willem Jiang <willem.jiang@gmail.com>
Enforce one queued or running scheduled-task run per task with a partial unique index. The migration resolves legacy duplicates before creating the index, and losing inserts use the existing conflict or skip outcomes.
Normalize YAML frontmatter keys in the shared parser so validation and review report malformed fields instead of failing while sorting mixed key types.
Add GIF to the view_image allowlist: map the .gif extension to
image/gif and detect the GIF87a/GIF89a magic bytes so the existing
extension/content cross-check accepts GIFs instead of rejecting them
as an unsupported format. Covered by a new success test.
* fix(runtime): stop subgraph stream frames impersonating root frames
The web frontend always requested stream_subgraphs, and since delegated
subagent graphs inherit the parent checkpoint namespace (#4215), their
values snapshots and token chunks ride the parent stream. The worker's
_unpack_stream_item dropped the namespace and published every subgraph
frame under a bare event name, so a subagent's values snapshot replaced
the whole thread view in SDK clients (#4399), its token chunks flooded
the parent message stream, and a subagent's LLM error fallback could be
mistaken for the parent run's.
Publish subgraph frames under namespace-qualified SSE event names
(mode|ns1|ns2, LangGraph Platform style) and keep root-only consumers
(file-tool chunk batcher, subagent event persistence, error-fallback
detection) on root frames only. Drop streamSubgraphs from the frontend
submit paths: subtask progress arrives via root-namespace task_* custom
events, so the flag only exposed the leak.
* test(runtime): add production-shaped subgraph stream regression tests
Address review: the namespace tests validated the publishing helpers
with hand-fed namespaces, while the #4399 regression lived in the
integration between LangGraph's delegation routing and the worker's
stream loop. Add TestWorkerSubgraphStreamIntegration: a real parent
graph delegates through the real SubagentExecutor and streams through
run_agent into a real MemoryStreamBridge, locking both stream_subgraphs
modes -- delegated frames arrive namespaced (never bare), a delegated
error fallback cannot mark the parent run as errored, and without the
flag delegated frames stay out while task_* custom events remain.
* fix(frontend): strip and parse the <current_uploads> upload context tag
#4174 renamed the upload context block that UploadsMiddleware prepends to
the user message from <uploaded_files> to <current_uploads>, but the
frontend tag vocabulary was not updated, so the raw block (file list plus
usage guidance) rendered inside the user's chat bubble and the file-chip
content fallback stopped matching. Add the new tag to
stripUploadedFilesTag, INTERNAL_MARKER_TAGS (covers export and streamdown
preprocessing), parseUploadedFiles, and the chip fallback detection.
<uploaded_files> stays supported for history and IM-channel messages.
* fix(frontend): parse upload-context sizes to bytes for the file chip
parseUploadedFiles stored the raw leading number of the human-readable
size (e.g. parseInt("177.6 KB") -> 177) into FileInMessage.size, which
is documented as bytes. On the content-fallback chip path (history/IM
messages without additional_kwargs.files) formatBytes then rendered a
177.6 KB file as "0.2 KB".
Convert the backend's "<n> KB" / "<n> MB" form back to bytes so the chip
re-renders at the original magnitude; update the parse tests to assert
byte values.
* fix(frontend): keep leading orphan tool messages visible
#3880 stopped dropping orphan tool messages that arrive after a terminal
group, but left the leading-orphan branch dropping the message with a
console.error on every render. The case is reachable: history pagination
cuts by event seq, not turn boundaries, so the first loaded page can
begin mid-turn with tool results whose AI tool-call message sits on an
unloaded older page — in dev mode the diagnostic surfaces as a
full-screen Next.js error overlay (#4399). Open a processing group for
the leading orphan instead; loading the older page re-groups it under
its real turn.
* test(frontend): lock leading-orphan accumulation invariant; clarify self-heal comment
Address review on #4408:
- The self-heal note only holds for the pagination path. The
hidden-only-precedent case and a truly orphaned tool have no older page to
load, so the processing group persists and renders as an empty
ChainOfThought shell (convertToSteps emits steps only for type === ai).
Document that as an accepted degradation instead of implying it always
self-heals.
- Add a test locking the accumulation invariant: a leading orphan followed by
a real tool-call turn folds into one processing group, not a stranded empty
group beside the real turn.
An empty assistant message from a provider safety filter (content_filter with
no content, no tool calls) was persisted into thread history and replayed to
strict OpenAI-compatible providers, which reject it with HTTP 400 ("message ...
with role 'assistant' must not be empty") — breaking every later turn until a
new chat is started.
SafetyFinishReasonMiddleware only handled the tool-call case (#3028) and
TerminalResponseMiddleware only the post-tool case (#4027), so a plain empty
content-filter response fell through both. Extend the safety middleware to
backfill a user-facing explanation when a safety-terminated message is
otherwise blank, so the persisted turn is non-empty (and the user sees why it
was blocked).
Fixes#4393
PR #4289 corrected the false claim that GitHub auto-retries 5xx webhook
deliveries, but its replacement wording overcorrected: it described a
mistaken 200 response as dropping the webhook "forever with no way to
recover it" / "permanently" - implying manual recovery is impossible,
not just unprompted. fancyboi999 flagged this in a CHANGES_REQUESTED
review on #4289 (submitted 23:21:23Z, referencing
github_webhooks.py:190-197,325-335 and
test_github_webhooks.py:548-559) that went unaddressed before the PR
was approved and merged roughly 40 minutes later.
Verified directly against GitHub's documentation before changing
anything: the manual "Redeliver" button and the REST/App redelivery
endpoints place no failed-status precondition on the delivery id - any
past delivery, success or failure, can be redelivered within GitHub's
~3-day window
(https://docs.github.com/en/webhooks/testing-and-troubleshooting-webhooks/redelivering-webhooks,
https://docs.github.com/en/rest/repos/webhooks#redeliver-a-delivery-for-a-repository-webhook).
The real problem with swallowing a transient failure into 200 is
discoverability, not recoverability: the delivery never shows up as
failed in Recent Deliveries, and GitHub's own recommended
scripted-recovery pattern filters on non-OK status by convention, not
because the platform blocks redelivering a success. A 200'd delivery
can still be redelivered by hand if an operator happens to look - they
just get no signal telling them to, unlike the 503 path, which stays
correctly flagged as failed and so is actually found.
- github_webhooks.py: reworded the route docstring and the inline
fan-out comment to describe the 200-vs-503 difference as
discoverability, not raw recoverability, and added the redelivery
docs link alongside the existing failed-deliveries link.
- test_github_webhooks.py: reworded
test_dispatch_failure_returns_503_not_200's docstring the same way.
No assertions changed.
All 44 tests in test_github_webhooks.py pass, plus
test_github_dispatcher.py / test_github_channel.py /
test_github_registry.py / test_channels.py (322 total). ruff check and
ruff format --check are clean on both touched files.
* fix(gateway): seed branch run-events so inherited history survives (#4380)
The thread feed (GET /messages, /messages/page) reads the run-event store,
but branch creation only wrote checkpoint state - a fresh branch had no
message rows, so the parent history vanished from the UI as soon as the
branch's first run refreshed the feed. Seed the branch's run_events from
the same checkpoint snapshot the branch was created from, mirroring
RunJournal's message-event contract (event types, hidden-message rules,
original-user-text restoration). Best-effort: a seeding failure degrades
to the old behavior and is reported as history_seed_mode=failed.
* docs(gateway): correct branch-seed docstring on RunJournal divergences
The "consumers cannot tell a seeded row from a journaled one" claim was
overstated for AI rows: seeded rows omit run-scoped enrichment (usage /
latency_ms / llm_call_index) and stamp caller=lead_agent rather than the
message's original caller, neither recoverable from a checkpoint message.
Rewrite the docstring to state these divergences explicitly and note they
are display-invisible today (no consumer indexes those keys; per-message
caller drives no attribution). Also add a code comment marking the
hide_from_ui filter as intentionally stricter than the live paths.
* fix(gateway): seed dict-shaped checkpoint messages + persist hidden AI/tool rows
Two review-driven fixes to build_branch_history_seed_events:
1. Checkpoint messages can arrive as model_dump()-shaped dicts (the
branch-matching helpers in threads.py already handle both BaseMessage
and dict). The seed only handled BaseMessage, so a dict-backed
checkpoint seeded nothing and the branch reported skipped_empty while
history existed. Coerce dicts back to BaseMessage via messages_from_dict
(faithful: tool_calls / tool_call_id / additional_kwargs survive);
unparseable dicts are dropped best-effort.
2. RunJournal.on_llm_end and _persist_tool_result_message persist
hide_from_ui AI/tool rows unconditionally (the frontend hides them
client-side); the hide check only gates the reconciliation pass. The
seed dropped them, so a hidden turn vanished from a forked feed and
seeded rows diverged from journaled ones. Match RunJournal and write
them, restoring true row-level parity.
Adds tests for dict deserialization, the unparseable-dict drop, and the
hidden AI/tool persistence contract.
Declaring the injected runtime arg as `Annotated[Runtime, InjectedToolArg] | None`
made the top-level annotation a Union, so LangChain no longer treated it as
injected. It leaked into the model-facing schema and pydantic raised
PydanticInvalidForJsonSchema on the ToolRuntime dataclass the moment the tool
was bound to a model. The tool is bound by default for the lead agent, so any
default run on an OpenAI-compatible provider failed at tool-bind time.
Declare runtime as a bare Runtime first param, matching every other built-in
tool (present_files, view_image, task, ...), which LangChain auto-injects and
auto-excludes from the schema. Add a schema regression test that binds the tool.
* feat(agents): database-backed storage for custom agent definitions
Add an agent_storage.backend switch (default file, behaviour-unchanged) with a
db backend that stores each custom agent as a row in the shared SQL persistence
layer, so a multi-instance deployment sees the same agents on every node
(#4331, #4357). Introduces an AgentStore interface routing all read/write
surfaces, an agents table + migration 0006, startup validation, and a file->db
importer. Follows the thread_meta store / run_events backend-switch /
0003_scheduled_tasks migration patterns; no new dependency.
* fix(agents): make db storage path production-ready (review round 1)
Addresses review feedback on the db/sync agent-storage path:
- sql.py: mirror the async engine's per-connection SQLite PRAGMAs on the sync
engine (busy_timeout=30000, synchronous=NORMAL, foreign_keys=ON, WAL) so both
engines behave identically against the shared DB; guard the engine cache with
a lock (double-checked) so concurrent first-touch cannot build duplicate
engines or register the connect listener twice.
- routers/agents.py + routers/assistants_compat.py: offload the sync-store reads
that ran on the event loop (list/get/check, update's pre-read + legacy guard +
refresh, and assistants_compat's four list routes) via asyncio.to_thread — on
db+postgres each was a network round trip stalling the loop. Writes were
already offloaded.
- file.py: translate the create() mkdir(exist_ok=False) race FileExistsError
into AgentExistsError (router 409, matching SqlAgentStore's IntegrityError
path); correct the _write docstring — per-file atomic replace, two commits
sequential not transactional.
Tests: sync-engine PRAGMA + engine-cache reuse assertions; file create-race ->
AgentExistsError; strict Blockbuster anchor over the read endpoints so a
regression back onto the loop fails CI.
* fix(agents): address round-2 review on the db store path
- update_agent tool: align the docstring/inline comment with FileAgentStore._write.
Cross-field write atomicity is db-only; the file backend commits config then
soul via two sequential os.replace (a crash between them can leave a fresh
config.yaml beside a stale SOUL.md). The dropped partial-write *reporting* is
an intentional tradeoff — the stage-then-replace safety is preserved
(test_update_agent_soul_failure_does_not_replace_config still holds).
- SqlAgentStore.update(): true upsert. Catch IntegrityError on the
insert-on-missing branch, re-fetch and apply, so two concurrent first-time
writes (e.g. two setup_agent handshakes) converge instead of surfacing a raw
UNIQUE(user_id, name) violation as a 500. Symmetric with create().
- get_agent_store(): document the graph-subprocess config-resolution invariant
(the except->file fallback is a genuine no-config path, not a mask for a
misconfigured graph process) and pin it with two tests driving the real
get_app_config() file resolution: db resolves from an on-disk config.yaml,
file fallback when config is unresolvable.
* test(agents): cover SqlAgentStore.update() write-race upsert recovery
Mandatory-TDD test for the round-2 fix in 0680340a: two concurrent first-time
update()s where the loser's insert hits UNIQUE(user_id, name). Deterministically
forces the IntegrityError recovery path by making the first _row probe miss the
committed winner, and asserts last-writer-wins instead of a surfaced 500.
The reasoning-effort selector trigger only appended a value for the four
explicit efforts; when reasoning_effort was unset it rendered
"Reasoning Effort:" with an empty value. The dropdown already treats an
unset effort as medium (highlighted and checked), so the trigger and the
menu disagreed on the default. Show "Medium" in the trigger when unset to
match the dropdown's existing default.
* fix(mcp): use threading.Lock for OAuth token refresh to avoid cross-thread deadlock
OAuthTokenManager created one asyncio.Lock per server for the process
lifetime. The embedded/TUI sync tool-call path (DeerFlowClient.stream()
-> LangGraph's ToolNode._func -> a ThreadPoolExecutor ->
make_sync_tool_wrapper's per-call asyncio.run()) invokes
get_authorization_header from a fresh event loop on a fresh OS thread
for every concurrent tool call. asyncio.Lock binds to whichever loop
first contends on it; when a caller on a different loop later releases
or wakes a waiter, it does so without call_soon_threadsafe, so the
waiting loop's selector is never woken and that caller hangs forever
with no exception. A third concurrent caller instead raises a
synchronous RuntimeError ("bound to a different event loop"). Either
way, two concurrent OAuth-protected tool calls (including the very
first cold-start token fetch) can freeze the entire agent turn.
Gateway's async path (ToolNode._afunc) is unaffected.
Replace the asyncio.Lock with a plain threading.Lock, acquired via
asyncio.to_thread so the blocking wait never blocks the event loop,
and released synchronously in a finally block. This keeps the
single-fetch de-duplication the lock provided while making it safe
across however many event loops/threads call into the same server's
lock.
Adds a regression test that runs three threads, each with its own
event loop, calling get_authorization_header concurrently for the same
server, and asserts (with a bounded join timeout so a regression fails
fast instead of hanging the suite) that none hang or raise, and that
only one real token fetch happens.
* fix(mcp): make OAuth lock acquisition cancellation-safe
get_authorization_header acquired the per-server threading.Lock via a
bare `await asyncio.to_thread(lock.acquire)`, with the try/finally that
guarantees release only starting after that await returned. Once the
executor thread had actually started running lock.acquire(), cancelling
the awaiting caller only stopped the caller -- Python cannot interrupt a
running OS thread. CancelledError was still delivered to the caller
immediately, but the thread kept blocking until the current holder
released, then silently acquired the lock with nobody left to call
release() for it. The lock stayed locked forever and every later OAuth
token refresh for that server blocked permanently at the same line --
the exact cross-thread deadlock this lock was introduced to prevent,
reintroduced via a different path under cancellation (e.g. a caller
wrapped in asyncio.wait_for/asyncio.timeout, or task-group cancellation).
Run the acquisition as an explicit asyncio.create_task, awaited via
asyncio.shield() so cancelling the caller no longer cancels the
underlying acquisition task. If the caller is cancelled, keep
(re-)waiting on the still-shielded acquisition task -- tolerating
further cancellation during this cleanup by simply retrying -- until it
actually finishes, release the lock immediately, and only then
re-raise. This guarantees the lock is released regardless of when or
how many times the caller is cancelled: before the acquisition is even
scheduled, while queued, or after it has already been silently granted.
Adds a regression test that holds the per-server lock, starts a second
caller that has to wait for it, cancels that caller while it is
genuinely blocked in its executor thread, releases the original holder,
and asserts a third caller completes within a bounded asyncio.wait_for
and still performs exactly one token fetch. Every potentially-hanging
await is bounded so a regression fails the test quickly instead of
hanging the suite.
* fix(feishu): check response.success() on card/reaction SDK calls
_reply_card, _create_card, _update_card, and _add_reaction call the
lark-oapi SDK and only used the response on the happy path, never
checking response.success(). lark-oapi signals a business-level
failure (invalid/expired card, permission error, etc.) by returning a
response with success()=False rather than raising, so these calls
looked identical to callers whether Feishu accepted them or not.
This file's own _upload_image/_upload_file/_receive_single_file
already guard against exactly this by checking response.success()
before trusting the response; the card/reaction helpers just didn't
follow that established pattern.
The gap is most exposed on _update_card: Feishu supports streaming, so
a single conversation issues many _update_card patches, each one a
chance to silently drop an update. _send_card_message already has a
try/except around _update_card that retries (via _send_with_retry) on
non-final failures and falls back to a brand-new card on final ones -
but that logic was unreachable because _update_card could never raise
on a business failure.
Adds response.success() checks to all four methods, raising for
_reply_card/_create_card/_update_card (mirroring the upload helpers,
and making the existing retry/fallback logic in _send_card_message
reachable) and logging a warning for _add_reaction (mirroring
_receive_single_file, since a failed reaction is fire-and-forget and
must not trigger a redundant resend of the whole card).
Adds regression coverage in TestFeishuCardSuccessChecks: a
business-failure mock response for each of the four methods, plus two
tests driving _send_card_message end to end to confirm the retry and
fallback-to-new-card paths actually engage now.
* fix(feishu): include log_id in card SDK failure errors + cover create_card retry path
willem-bd's review on this PR suggested two non-blocking follow-ups:
- _reply_card/_create_card/_update_card's RuntimeError on a business-level
failure omitted the Feishu log_id, unlike _add_reaction and
_receive_single_file in this same file, which already include it in their
warning logs. Adding it gives a Feishu support-traceable id once retries
exhaust and the error reaches the caller.
- _create_card's failure on the no-thread_ts path (the tail of
_send_card_message) only had direct unit coverage
(test_create_card_raises_on_business_failure_response), unlike
_update_card's failure path, which also has an end-to-end test through
send() confirming _send_with_retry engages
(test_send_retries_after_update_card_business_failure_then_succeeds).
Adds the mirrored end-to-end test for the _create_card path.
invoke_acp_agent had no timeout anywhere in its call path, and
ACPAgentConfig had no timeout field. If the ACP agent subprocess answers
initialize/new_session correctly but then hangs inside prompt(), the tool
call - and therefore the whole agent turn - blocks indefinitely, with the
child process left running. MCP stdio servers already guard against this
class of hang via tool_call_timeout; ACP agent invocations had no
equivalent.
Add ACPAgentConfig.timeout_seconds (default 1800, ge=1), mirroring the
shape/default of subagents.timeout_seconds, and wrap the conn.prompt()
call in asyncio.wait_for(). On TimeoutError, return a clear error instead
of hanging; exiting the spawn_agent_process context block triggers the
ACP library's own graceful-then-forceful subprocess cleanup, so the hung
process is actually terminated.
* refactor(memory): pluggable MemoryManager interface for backend onboarding
Optimize the MemoryManager interface layer so new backends (mem0/openviking)
onboard with less code and the contract stays stable as capabilities are
added. A minimal backend now implements only from_config + add + get_context
(verified by test_memory_manager_interface.py::_MinimalBackend onboarding via
the factory); the factory no longer knows a backend's private hooks.
- MemoryManager: ABC -> pydantic BaseModel; three-tier methods (tier-1
add/get_context abstract; tier-2 management defaults; tier-3 optional hooks
warm/reload/fact + on_pre_compress/on_turn_start). Dropped 3 self-serving
hooks. 6 hasattr probe sites -> direct call + try/except NotImplementedError.
- from_config classmethod: factory thins to resolve + inject storage_path +
collect host hooks + call from_config; DeerMem-specific hook consumption
moved from factory to DeerMem.from_config.
- Invariants: @model_validator (mode='tool' requires search via supports_search
ClassVar); DeerMemConfig storage_path-is-file check moved here from factory.
- Async: aadd/aget_context/asearch default to the sync path (speculative).
- Callbacks: MemoryCallbacks + LangfuseMemoryCallbacks; on_memory_llm_call
subsumes tracing_callback (same signature/timing/mutation); deleted the
tracing_callback field. DeerMem decoupled from langfuse (portability).
- noop keeps read-op empty overrides (avoids router 500s on the
disable-memory-via-noop path); only delete/export inherit the base raise.
Behavior preserved: 661 passed / 13 skipped. Docs: backends/README.md rewritten
(three-tier + from_config + callbacks); samples README updated; removed stale
private doc paths.
Co-Authored-By: Claude <noreply@anthropic.com>
* fix(memory): 501 on unsupported read/manage endpoints + accurate warm log
Review follow-up on the three-tier MemoryManager refactor.
- Read/manage endpoints (GET /memory, /memory/export, /memory/status,
DELETE /memory, POST /memory/import) and the /memory/reload fallback now
catch NotImplementedError -> 501, matching the fact-CRUD endpoints. The
hasattr->try/except migration had skipped these: they were @abstractmethod
before (every backend implemented them, so they never raised), so once they
became tier-2 default-raise a minimal backend (only add + get_context) hit a
raw 500 -- there is no global NotImplementedError handler. get_memory is
shared via _get_memory_or_501 (covers /memory, export, status, reload
fallback). noop is unchanged: its read-op empty overrides never raise.
- warm() base default returns None (tri-state: True=warmed, False=failed,
None=nothing to warm) so the Gateway lifespan logs "skipping" for a
non-DeerMem backend (e.g. noop) instead of the inaccurate "warmed
successfully" it never earned. DeerMem.warm keeps True/False.
- Tests: 6 router 501 tests (read/manage + reload fallback) + 2 lifespan
warm-log tests (None->skipping, False->warning); conformance/pluggable
assert warm() is None.
705 passed / 13 skipped; lint clean.
Co-Authored-By: Claude <noreply@anthropic.com>
* fix(memory): review follow-ups - search-flag consistency, client reload, backend_config purity
Address review feedback on the three-tier MemoryManager refactor:
- [Medium] supports_search/search drift: the invariant now requires the
supports_search ClassVar flag to MATCH whether search() is actually
overridden (type(self).search is not MemoryManager.search), so the flag
can't drift from the impl. Catches both directions at instantiation: a
backend that overrides search() but forgets supports_search=True (was a
misleading tool-mode rejection), and one that sets the flag without
overriding (was a runtime NotImplementedError on the first memory_search).
noop sets supports_search=True to match its search() override. Conformance
adds drift + consistent-backend tests.
- [Low] client.reload_memory fallback: wrap the get_memory fallback so a
minimal backend (only add + get_context) surfaces a clean NotImplementedError
("implements neither reload_memory nor get_memory") instead of an uncaught
propagation -- mirrors the router's 501. Test added.
- [Low] backend_config purity: DeerMem.from_config restores backend_config to
the pure data the host passed after model_post_init parses the injected hooks
into DeerMemConfig (self._config, PrivateAttr); the field stays serializable
(no callables/LLM) and matches the README ("host hooks NOT in backend_config").
Test asserts purity + hooks wired.
- [Low] CHANGELOG: breaking-change note that mode='tool' + non-search backend
now fails fast at startup (was silently empty) so operators recognize it on
upgrade.
- [Nit] .gitignore: drop the env-specific .tmp-pytest/ entry (--basetemp is
local-only, not make test/CI).
709 passed / 13 skipped; lint clean.
Co-Authored-By: Claude <noreply@anthropic.com>
* docs(changelog): correct memory tool-mode fail-fast note
The CHANGELOG entry said mode='tool' + a non-search backend "(e.g. noop)"
fails fast at startup, but noop overrides search() (returns []) and sets
supports_search=True (required by the consistency invariant), so noop IS
search-capable and noop+tool does NOT fail fast. The fail-fast only affects a
custom backend that onboards without overriding search(). Reworded to drop the
misleading noop example and state both shipping backends implement search().
Co-Authored-By: Claude <noreply@anthropic.com>
---------
Co-authored-by: Claude <noreply@anthropic.com>
* fix(sandbox): bound E2B output synchronization resources
E2B release-time output sync pulled every changed file back from the
remote VM with only a per-file size cap and no aggregate bound, so a
pathological outputs tree (thousands of files, or many sub-cap files
summing to gigabytes, or a slow VM) could make release download
unboundedly on a hot path that runs at every agent turn end.
Add three aggregate ceilings on top of the per-file cap — total bytes,
file count, and a wall-clock deadline — enforced in the sync loop. When
a ceiling is hit the pass stops early, logs what it dropped, and defers
the rest to the next release. A truncated pass skips stale-manifest
pruning so files it never reached are reconciled next time instead of
being forgotten and re-downloaded.
Closes#4340
* test(sandbox): pin multi-pass convergence of bounded output sync
The four truncation tests each exercise a single capped pass. Add a
two-pass test that locks in the invariant the design relies on for
correctness: already-synced files are skipped before the budget check,
so they never consume the cap and the deferred tail drains over
successive releases instead of the leading files being re-downloaded
every turn. A refactor that let a skipped file consume the cap would
pass the single-pass tests but fail this one.
