Merging main rebased this branch's feedback migration to
0008_feedback_tags on top of main's 0007_scheduled_run_active_index,
so the chain head moved past the pin in main's new dedupe test. The
test runs `upgrade head` and asserts the chain tip; its actual subject
(the dedupe pass + partial unique index) is unaffected.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Renumber the feedback tags migration to 0008 on top of main's
0007_scheduled_run_active_index so the alembic chain keeps a single
head; bootstrap head pins follow.
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.
Resolve the one import conflict in thread_runs.py: keep main's new
checkpoint_lineage imports (#4358 regenerate-in-branched-threads fix)
alongside this branch's get_feedback_service (feedback domain-service
refactor). All other files auto-merged. Verified: backend feedback/
regenerate/branch/lineage tests (112 passed) and the full frontend suite.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* 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.
The feedback migration (0007_feedback_tags) advanced the alembic chain
head, but the bootstrap/migration tests still hardcoded 0006_agents as
the expected head. Update the HEAD constants and the post-upgrade
version assertions so the head-pin guard and the legacy/concurrency
bootstrap tests match the current chain tip. Test-only; no migration or
schema change.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
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
The single-line `class X: ...` exception stubs tripped
github-code-quality's "Statement has no effect" rule: a bare `...` is a
discarded expression statement. Replace each with a one-line docstring,
matching the empty-error convention used elsewhere (GoalWriteConflict,
ConflictError) and silencing the false positive. No behavior change.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
buildVisibleHistoryMessages now carries feedback onto each flattened
message (c9e6aa83). Update the superseded-runs test to expect the new
feedback: null field, and apply Prettier line wrapping to
feedback-dialog.tsx so the format check passes.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
One rating at a time: the other thumb is hidden while a rating exists
and must be retracted before switching. Thumbs-down stores the rating
immediately, then opens a follow-up dialog (reason chips + optional
details) submitted as a second idempotent PUT. Reason slugs are
language-neutral; labels localize via the i18n catalog. The buttons move
to the assistant turn action bar next to copy/regenerate.
The message-list page rows attach the current user's feedback on the
wrapper level, next to run_id; flattening RunMessage into Message kept
run_id but dropped feedback, so thumb state was lost after a reload.
The ORM row stays behind (single alembic chain until the infrastructure
move); everything else is served by the domain slice. Repository tests
become a contract suite that runs the same cases against the SQL adapter
and an in-memory fake; ownership and timezone tests use explicit user_id
instead of the AUTO-sentinel context.
The composition root wires FeedbackService with its SQL adapter; routers
become protocol translation only (domain errors map to 400/404/409).
BREAKING CHANGE: removes five feedback endpoints with no frontend
consumers (POST, GET list, GET stats, DELETE by id under /threads, and
GET /runs/{id}/feedback). PUT/DELETE now follow the mainstream chat UX:
idempotent upsert plus retract, echoed through the message-list payload.
SqlFeedbackRepository implements the domain port (queries migrated
unchanged from the legacy repository); RunStoreRunLookup adapts the run
store for ownership checks. FeedbackRow gains a JSON tags column with an
idempotent alembic migration. Legacy repository untouched in this step.
Hexagonal inner ring for the feedback bounded context: frozen aggregate
with construct-time invariants (rating, reason-tag slugs), technology-
neutral ports (typing.Protocol), and the application service with
explicit user_id. Guarded by an AST purity test that keeps domain/ free
of infrastructure imports.
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.
