_download_file did a bare requests.get and wrote resp.content, so the
whole body was buffered before anything could object -- a socket
timeout alone never fires on a server that keeps trickling bytes, and
the vendor decides the size of the file it signs a link for.
Route it through common.media_download, the helper the LinkAI voice
download in voice/linkai already uses: the body is streamed, counted
chunk by chunk and cut off at the shared cap, and a refused download
leaves no partial file behind.
upload_media() downloads a remote URL into the tmp dir and never
removes it: the success, errcode and exception paths of the upload all
leave the file behind, and nothing sweeps the tmp dir. The voice send
converts the TTS output to a sibling .amr that is never removed either
-- and pydub opens that target before it spawns ffmpeg, so even a
failed conversion leaves a half-written file.
Every sibling sender already cleans up after itself (weixin's
`if downloaded: remove_download(...)`, wecom_bot's temp_files +
finally); dingtalk was the one that did not. The download now lands on
a unique state_dir.tmp_file() name and is removed in a finally around
the upload, and the amr is registered before the conversion so a
failure cleans it too. The TTS source itself is left in place, the way
the siblings leave a local source alone.
_save_config_to_file wrote to get_root()/config.json (the bundle)
while load_config() reads get_data_root()/config.json. The desktop
build (COW_DATA_DIR=~/.cow) and the docker image (/home/agent/.cow)
point the two at different directories, so every cloud-pushed config
change -- on_config and the three channel handlers -- was written
where nobody reads it: dropped with a warning when the bundle holds
no config.json, or shadowed by a stale file that is never loaded.
Write to the data root instead. Source deployments are unchanged:
without COW_DATA_DIR, get_data_root() falls back to get_root().
Two sibling asymmetries on the subagent tool path:
1. _collect_tasks coerces goal and context with str() but passed
subagent_type through raw. The model quotes scalars the way
_coerce_integer_args exists for, and a non-string reached the
unknown-type report, where sorted() over mixed types and then join()
raised a bare TypeError -- so instead of "Unknown sub agent type(s):
123. Available: ..." the call failed with a Python internal error,
which the model typically retries verbatim until the consecutive-
failure guard disables the tool. run_tasks tolerates the same input
by falling back to the default template; the report path now does too.
2. load_templates guarded the per-file read (an undecodable template
must not disable valid types) but not the os.listdir that feeds it.
A directory that cannot be listed -- permissions changed, a race
with a delete -- raised out of load_templates and out of execute,
while the tool description degraded. It now degrades the same way
and keeps the built-in types.
Adds tests for both: a parametrized non-string subagent_type (int,
bool, list; single and tasks forms) answers the actionable unknown-
type error, and an unlistable templates dir keeps execute working.
A preview fetched from a configured Skill Hub registry stages the skill
together with the fields the registry returned (category,
display_description, icon, version) in the staging directory's
skills_config.json. commit_staged forwards only source and display_name,
and the staging directory is discarded right after the confirm, so the
confirm silently dropped the rest: no icon, no version, no display
description, and category fell back to "skill" -- while the plain
install action records the caller's meta through record_install.
The confirm path now reads the staged entry before the staging is gone
and records it the same way, for a single-skill install. Adds a
regression test that drives preview setup and confirm through the
console endpoint and asserts the live entry keeps the fields.
build_voice_to_text is not failure-safe: an unknown vendor raises from
the voice factory, edge/elevenlabs answer None because their voiceToText
is an unimplemented no-op, and a vendor network error escapes. On the
VOICE branch of _generate_reply the call was bare, so the exception
travelled to _fail_callback (log only) and the user lost the turn, and a
None answer crashed on reply.type. The TTS sibling was guarded by
_safe_text_to_voice (#3684); this adds the mirror _safe_voice_to_text and
answers an ERROR Reply when transcription is unavailable.
Also corrects the stale claim in _safe_text_to_voice's docstring that
voice_to_text already degraded gracefully -- it did not, for exceptions
or None.
Adds tests/test_chat_channel_stt_fallback.py: a raising vendor and a
None-answering vendor both answer an ERROR Reply instead of losing the
turn, and the happy path still hands the transcript to the text branch.
The lint job runs `ruff check <changed files> --select F`, and
scheduler_tool.py carried five unused imports on master (typing.Any,
typing.Dict, bridge.context.ContextType, bridge.reply.Reply,
bridge.reply.ReplyType). Any PR touching this file therefore fails the
gate on pre-existing debt, so remove them: nothing in the module uses
them and nothing imports them from here (only SchedulerTool is).
_decorate_reply called super().build_text_to_voice() unprotected. The
call is not failure-safe: an unknown text_to_voice vendor raises a bare
RuntimeError from voice/factory.py, and custom (OpenAI-compatible)
vendors answer with an ERROR Reply when their endpoint or model is
misconfigured. The exception bubbled up to the worker future and
_fail_callback, which only logs, so the user lost the whole turn; the
ERROR Reply instead replaced the answer with a generic error message,
because the real text sat in voice_reply_text, which _send_reply only
reads for VOICE replies.
Wrap the call in _safe_text_to_voice: on exception or a non-VOICE
reply, log the reason and fall through so the answer is still
delivered as text, prefix/suffix included. voice_to_text already
degrades gracefully in _generate_reply; TTS now does too.
_create_task accepted a once task with a past run_at, stored the past
next_run all the same and still answered with the run time as the
"next execution". On the service side that past next_run is due
immediately: within the 10-minute catch-up window the task fires late,
and beyond it _is_task_due logs a warning and deletes the one-time
task - the reminder never fires and only a server-side log says why.
The user is told "created" either way.
Refuse the creation with an actionable error instead, mirroring the
service's own _calculate_next_run, which answers a past run_at with no
next run at all. The comparison runs through scheduled_reference_time,
so it happens in the task's own mode (aware UTC for a declared zone,
legacy naive local otherwise) and a zoned task cannot raise a
naive/aware TypeError.
_yaml_scalar quoted a value only when it matched _YAML_NEEDS_QUOTING
(indicator characters, ": ", " #", a trailing ":"), so the whole family
of YAML 1.1 plain scalars that resolve to a non-string slipped through
unquoted. A skill with description "no" was written as
`description: no`, read back by PyYAML as False, normalised to '' and
then dropped from the listing as "has no description" - the entire
skill silently disappeared. "yes"/"on"/"true" came back as "True",
"1:30" as "90" and "0x1f" as "31": user content was silently rewritten.
_yaml_scalar now also quotes when _plain_scalar_survives_yaml() reports
the value would not round-trip. The probe parses the scalar with
yaml.safe_load - the same loader the frontmatter parser uses - and
requires the same string back; a regex mirroring PyYAML's implicit
resolvers covers environments without PyYAML and errs towards quoting.
Round-trip fidelity beats the shortest possible frontmatter, because
descriptions and voice texts are user content.
Deleting a Weixin instance now removes that instance's credential file (weixin_credentials.{instance_id}.json) instead of leaving it on disk.
The cloud control plane removes instances through three paths -- instance_delete, instance_update with enabled=N, and channel_sync. Before this change only the legacy single-login path (_handle_channel_delete) cleared credentials, and even then only the default path. Per-instance files were never removed, so deleting and re-adding an instance silently re-logged it into the old account from the token left behind.
_remove_weixin_credentials now takes instance_id and the three deletion paths pass it through, guarded to weixin/wx so feishu/telegram deletions are untouched. The legacy default path is still cleared when called with no instance_id.
Tests: tests/test_cloud_weixin_credentials_cleanup.py (fail before, pass after).
ToolManager keeps five parallel registries for MCP state.
_teardown_mcp_server is the only place a server is retired, and it
cleared four of them -- the client, its pool entry, its tool instances and
its status -- but not _mcp_tool_vectors.
That index is keyed by tool name, and its only writer fills MISSING names:
missing = [name for name in current if name not in self._mcp_tool_vectors]
So when a reload moves a tool to a different server -- mcp.json edited,
server alpha replaced by beta publishing the same tool name with a
different description -- the tool is republished under a name whose vector
came from the old description, and is never re-embedded. Retrieval then
ranks it against a description it no longer has: the tool is live and
callable, but the one turn that needed it does not get it in the schema,
and nothing reports an error.
The cache also only grew. Three reload cycles with one renamed tool left
[1, 2, 3] entries; a loader that failed part way through publishing leaves
an entry nothing can attribute to a server, so _ensure_mcp_tool_vectors
also drops names that no longer have a tool.
_resolve_media_path fetches a URL reply into the agent's managed tmp dir --
the convention every channel follows through common.state_dir.tmp_dir(),
since a bare /tmp resolves against a different drive depending on where
the process was launched. The voice branch in this same file already
removes what it fetched:
for path in {file_path, amr_file, *files}:
try:
os.remove(path)
except OSError:
pass
The file branch did not, and nothing anywhere sweeps that directory. So
every remote document reply left one file of up to MAX_FILE_BYTES
(100 MB) behind, permanently.
_resolve_media_path now returns (path, downloaded) and _send_file removes
what it downloaded in a finally. A file:// path or a bare local path
belongs to the caller and is left alone, so the agent's own output
survives the send.
A team conversation keeps one transcript per participant -- every Agent's
rows live in the one shared sqlite file, told apart by agent_id -- and
clear_session scopes its DELETE to the handle's own id:
DELETE FROM messages WHERE agent_id = ? AND session_id = ?
clear_context has fanned out across participants since it was written,
with the reason in its own docstring:
A team conversation keeps one transcript per participant, so every
participant is cleared; otherwise the rest replay it on the next turn.
delete_session never did, and _teammates() was already there. So deleting
a conversation removed the initiator's copy, reported success, and left
the other participants still listing it with their transcript intact --
ready to be replayed into the model's context.
Measured on the current code, after delete_session: alpha 0 rows,
beta 2 rows and still listed. clear_context on the same roster clears
both.
The fan-out takes fanout=False, as _clear_teammate does: _teammates is
symmetric, so re-entering it would bounce between the participants
forever. _delete_for_teammate is best-effort, so one unreachable
participant cannot fail a delete the initiator already committed.
A peer in another process stays out of reach here -- removing that copy
needs a transport mode of its own, the way clearing one does.
_download_remote_media puts reply media into managed tmp storage so a bare
name never lands in the process CWD, which a packaged desktop build does
not control. The callback path's base64 loader removes what it downloaded
in a finally; the three long-connection senders did not, and neither did
the files they derive:
_send_image download -> _ensure_image_format -> _compress_image
_send_file download
_send_voice download -> any_to_amr
So every remote media reply left one to three files in <workspace>/tmp, up
to MAX_FILE_BYTES each, and nothing anywhere cleans that directory. An
agent answering with a remote file once per turn fills the disk. Only
files this channel created are removed -- a file:// path or a bare local
path belongs to the caller and must survive.
The same download also had no wall-clock budget. The socket timeout only
bounds the gap between two chunks, which common.media_download's own
docstring spells out, and send() runs on the shared reply thread, so a
stalled download blocked every later message on that channel. Every other
channel that downloads reply media passes an explicit max_seconds.
mcp_client reads a server's response body in three places. Two carry a total
deadline; _sse_send did not:
_sse_discover_endpoint :484 iterates lines, checks a deadline
_read_sse_response :810 iterates lines, checks a deadline
_sse_send :522 one blocking resp.read()
urlopen()'s timeout bounds a single socket read and every arriving byte
resets it, so one read() waits as long as the server keeps trickling. The
file already states the consequence above _SSE_DISCOVERY_TIMEOUT:
Because the loader walks its servers serially on one background thread,
that stalls every server queued behind it: they stay "pending" and
their tools are silently missing.
_sse_send is the only channel of initialize / list_tools / call_tool for the
sse transport, and unlike stdio it runs no _call_lock, so nothing else
bounds it either. Read in bounded chunks against self._timeout now, with a
ceiling on the accumulated body.
passive_reply claims a conversation before handing it to produce():
channel.running.add(from_user)
channel.produce(context)
and the claim is cleared from the Future done-callback, which
WechatMPChannel reaches through _success_callback / _fail_callback. It is
also the only gate in front of the next task:
if (channel.cache_dict.get(from_user) is None
and from_user not in channel.running) or ...
Two independent paths leave that state behind.
produce() answers and returns before queueing anything for /cancel,
/steer and a conversation bound to a disabled agent. No queue entry means
no Future, and the done-callback is the only other thing that would have
released the claim -- so from then on every message takes the 'still
thinking' branch, sleeping before it says the agent is still working.
_release_claim gives those three paths the same release, and
WechatMPChannel defines it once for the callbacks to share.
cache_dict is a defaultdict(list), so reading a user with nothing cached
inserted an empty list. The IndexError that followed returned without
deleting the entry it had just created, and every cleanup path needs a
non-empty list first -- after which cache_dict.get(...) is no longer None
for that user. Read with .get and answer instead.
The update handler folds the whole request body into the stored action so
scheduler metadata survives an unrelated edit, and the trusted-recipient
lookup sat behind a comparison of channel_type / instance_id / receiver:
changed = (channel_type != old_channel
or new_instance != ...
or new_receiver != original_action.get("receiver"))
so a request that left those three alone kept whatever notify_session_id,
receiver_name and is_group it had sent -- with the directory never
consulted. notify_session_id is not cosmetic: integration.execute_task
reads it as the session the output is written into, and it reaches
remember_scheduled_output -> _persist_messages, which appends a
user/assistant pair to that conversation's persistent history and its
in-memory context buffer.
Re-pointing still needs the directory, and still refuses an untrusted
target. The identity is now re-derived on every IM edit: from the store
when it knows the recipient, which is what create already does, and from
what was stored when it does not -- a recipient can leave the directory
while a task still points at it, and refusing the edit would leave the
user unable to even rename the task. Either way the body has no say.
The workspace-write cases need a directory that no writable set covers,
and _normalize_roots always adds tempfile.gettempdir() to that set --
which also rules out tmp_path, and Path.home() because conftest points
HOME inside temp. The fixture anchored on the filesystem root, which is
not writable for an unprivileged user: CI failed with
PermissionError: [Errno 13] Permission denied: '/cow_permission_test_area'
Redirecting tempfile.gettempdir moves the trusted root instead of the
workspace, so the fixture stays writable everywhere. Same premise check,
same verdicts, and it no longer rebuilds a directory per case.
delete_agent already swept the two sibling stores, and both document
exactly why:
project_store.forget_agent
"an Agent later created with the same id would inherit them"
delete_agent
"the project store keys its bindings the same way and needs the
same sweep"
The store holding the conversation bodies was the one left out. Every
Agent's sessions, messages, artifacts and scheduled runs live in ONE
sqlite file -- the default Agent's memory/long-term/index.db -- scoped by
an agent_id column (conversation_store.get_conversation_store). That file
is not under the deleted Agent's workspace, so the rmtree never reached
it. Measured on the current code, deleting an Agent left all five rows
behind and an Agent recreated with the same id read back its old
transcript, its artifact index and its scheduled run output.
_forget_agent_conversations deletes by agent_id across the four tables,
which also keeps the default Agent's rows safe since its id is empty.
Both bash gates decide by splitting the line with _parse_segments and
classifying each segment. The segmenter understands $(...) because ( and
) are already separators; the other two spellings shlex does not know went
through unchecked.
A backquote is not a quoting character in posix mode, so
echo `rm -rf x`
lexed as the single token '`rm' and the rm inside was never classified.
Process substitution produced a plain '<(' token, so cat <(rm -rf x) left
the rm as arguments of cat. Both run real commands:
read-only echo $(rm -rf ../outside) deny
echo `rm -rf ../outside` allow
`rm` now becomes its own segment, and so does the body of a <(...) or
>(...), including a nested one. An escaped backquote stays a literal and
a substitution the outer command merely receives becomes a placeholder
that is neither a command name nor a path.
create_backup writes the manifest last, so a copy that fails part way
through leaves a directory holding a partial payload and no manifest.
restore_backup refuses such a directory outright:
if not manifest_path.exists():
logger.warning(f"[Evolution] Backup not found: {backup_id}")
return False
It was never a backup. _prune_old_backups counted directories rather than
snapshots, so the debris took one of the _MAX_BACKUPS slots and evicted
one that could still be undone. With ten good snapshots plus one failure,
ten directories survived but only eight were restorable.
The failure path now removes what it started, and the pruner only spends a
slot on a directory that has a manifest -- which also clears debris left
by a crash between mkdir and the manifest write.
The stalled-stream-cancel test read the cancel entry from inside run_chat,
so the assertion depended on the worker thread reaching that line before
the main thread finished draining. On a slower interpreter the worker had
not got there yet and the test failed on 'must be registered as
cancellable' instead of on behaviour.
_stream_completion registers the entry and starts the worker before it
returns the generator, and run_chat stays parked in released.wait(), so
reading it from the driving thread is deterministic. The drain now also
releases the worker in a finally block.
Same red proof on unfixed code: the entry exists but is unset.
707b3a45 pinned the per-message dedup hashes to the flush outcome: claimed
on dispatch, committed once the summary landed. The day-level hash that
create_daily_summary dedups on was never moved with them:
if content_hash == self._last_flushed_content_hash:
return False
self._last_flushed_content_hash = content_hash # recorded first
return self.flush_from_messages(...) # may never land
flush_from_messages returning True only means a worker was dispatched, and
that worker releases the message hashes when no summary could be produced.
The day hash was already gone by then. The daily timer does not retry within
the day, and Deep Dream distils daily files -- so an empty one left the
whole day's conversation unreachable by any later flush, dream or search,
with nothing above WARNING to say why.
The day-level hash now uses the same two sets as the message hashes: claimed
under the existing lock so a re-entrant tick cannot double-summarise, and
committed by the worker on every path that actually landed -- including the
"nothing worth recording" sentinel, which is an answer rather than a failure.
A failed dispatch or a failed write releases it, so the day stays retryable.
_last_flushed_content_hash still tracks the last day that landed.
Three channels cache a lone image/file and wait for the user's follow-up
question, attaching the local path to the next prompt. They share one
_prepare_fn -> file_cache.add pipeline, but only feishu checks that the
download landed:
if not os.path.exists(feishu_msg.content):
raise FileNotFoundError(feishu_msg.content)
The check matters because every _prepare_fn swallows its own download
error -- download_file logs and returns, _download_media returns "". So
the channel's `except` never fires for a failed download, and
file_cache.add stores whatever path content was left holding.
For wechat_kf that path was assigned *before* the write, so a body that
dies mid-stream (CDN reset, disk full, over MAX_FILE_BYTES) cached a file
that does not exist. The next question about that file pulled a
"[文件: ...]" reference into the prompt and sent the agent looking for
something never written -- or answering from what the filename suggested.
A non-200 left the provisional media_id path in place the same way.
wechat_kf now assigns content only after save_response returns, and both
channels check existence before caching. weixin's file/video/voice
downloads get the same treatment the image branch already had: a failed
download clears content instead of leaving the placeholder path.
Also retargets one assertion in test_inbound_media_download_bounds: it
checked Path(msg.content).exists(), and Path("") is "." -- which exists.
It now asserts the tmp directory is empty, which is what it meant.
_stream_completion bounded only the wait for the first event:
_FIRST_EVENT_TIMEOUT_SECONDS = 30
first_item = output.get(timeout=_FIRST_EVENT_TIMEOUT_SECONDS)
Every later frame came off a bare output.get(). Once the role frame was
out, a model call that never returned left the generator parked on that
get forever: the client got no error and no stream end, the finally block
that sets closed and calls _cancel_agent_request never ran, and the worker
stayed inside run_chat still holding _SESSION_LOCKS[hash(session_id) % 64]
-- so the next request for that session blocked on the same lock.
The frames loop now bounds the wait like the web SSE stream in
channel/web/core/channel.py does (600s idle, 60s after a cancel), reports
an error event instead of holding the socket open, and leaves completed
False on that path so the cleanup still cancels the run. The budget is
deliberately far above the 30s first-event one: a tool-heavy turn is
legitimately slow between chunks.
/api/sessions and /api/history hand page / page_size straight to
ConversationStore, which clamped page but never page_size:
page = max(1, page)
so page_size=-1 reached SQLite as LIMIT -1 -- which SQLite reads as *no
limit*, dumping every session in the table -- and page_size=0 divided by
zero while locating the until_seq page. HistoryHandler wraps its body in
`except Exception`, so that ZeroDivisionError surfaced as HTTP 200 with
{"status": "error"} and the console's "load earlier messages" never worked.
Both paginated methods now go through one _page_window() helper that
clamps the pair to [1, MAX_PAGE_SIZE], mirroring MemoryService.list_files
and sharing its ceiling. /api/sessions clamps at the handler too, because
scope=all multiplies page * page_size and slices the merged list itself
before the store is involved.
page_size also used to be echoed back unclamped, so the response described
a window the query had not actually run.
The streaming card is finalized on a worker thread so a slow DingTalk card
API cannot block the agent, and that worker is joined under a budget. The
budget was swallowed: _submit() returned nothing, so agent_end set
context["dingtalk_streamed"] = True even when join() timed out with the
final PUT still in flight. dingtalk_channel.send() reads that flag to
decide whether it still owes the user a webhook reply, so the user was
left looking at a card stuck on PROCESSING with no answer at all.
_submit() now reports whether a waiting submit was actually applied, and
the delivery claim follows that answer. The budget also becomes a named
constant so it is tunable, matching the drain-then-claim ordering the
Feishu streamer already uses.
The sentinel that retires the worker also left the dead thread
referenced, so _start_worker()'s guard believed a live consumer still
existed and every later update on that streamer was queued into a void.
The worker now clears its own reference on the way out.
The scan tests take the process-wide PluginManager singleton, replace its
registry with two stand-ins, and put the original back afterwards. They were
putting back an *empty* one: a freshly constructed singleton carries no
plugins, because the bundled ones are only registered by the scan inside
`load_plugins()`. Every test that ran later in the same process and touched
the registry then failed on `KeyError: 'COW_CLI'` / `'GODCMD'` / `'BANWORDS'` /
`'KEYWORD'` -- five of them, on both CI jobs.
Build the baseline by importing the bundled plugin modules directly, skipping
the stand-ins and evicting any cached copy first (a cached `import_module`
returns without re-running the `@register` decorator, so a second call
registered nothing and the baseline came back empty on every test but the
first). Replace the registry rather than emptying it: `plugins` is a
SortedDict whose heap is maintained by `__delitem__`, which `dict.clear()`
does not go through.
The four files CI flagged now pass alongside these: 36 passed.
`scan_plugins` reloads every plugin it has already imported, and reloading
re-runs the plugin's `@PluginManager().register(...)` decorator, which builds
a brand new class object and writes it into the registry. The scan then
decided which plugins were new with `list(set(news) - set(raws))` over the
classes themselves -- an identity comparison that the reload guarantees to
differ.
So every scan reported every plugin it had just reloaded as newly installed.
`#scanp` (`plugins/godcmd/godcmd.py:389`) hands that list straight to the user
as "发现新插件", so asking what had just been installed produced an answer
naming the entire plugins directory, and the same thing happened on every
subsequent scan.
Compare the registry keys instead, which is what identity means here:
`register()` writes `name.upper()` into the registry, and a reload writes the
same key it wrote before.
`list_files` turns `page` and `page_size` straight into a Python slice. That is
harmless while the web handler is the only caller, since it parses both itself
-- but `on_memory` forwards the cloud console's payload to `dispatch`
unchanged, and the console sends these fields as strings. They reach the slice
with whatever they arrived as, and Python's slicing then applies its own
semantics rather than rejecting them:
- `page=0` -> `files[-2:0]` -> an empty `list` next to `total: 5`, so the
console reports "no memory files" while holding five of them
- `page=-1` -> `files[-4:-2]` -> the middle of the listing, under a page
number that names no such page
- `page_size=-1` -> `files[0:-1]` -> every file but the last, silently
- a non-numeric page -> an arithmetic TypeError surfaced as a `500`
All of these answer `200` with a body that looks like a normal listing, so the
caller cannot tell it is not looking at the page it asked for.
Clamp both values before slicing, cap a single page, and coerce in `dispatch`
so a bad value is a `400` naming the fields -- the same shape the sibling
`on_history` handler (`common/cloud_client.py:1622-1631`) already returns for
these same two fields.
The parallel-safe calls of a turn are started all at once and then collected
with a bare `future.result()` per call, which blocks until the slowest tool
finishes. The loop's cancel checkpoint sits after `_run_parallel_calls`, not
inside it, so a cancel pressed while the prefetch was still waiting could not
be observed until every worker had already run to completion -- the user asked
to stop and the turn stayed open for as long as the slowest tool took.
The same fan-out in `agent/subagent/runner.py` already bounds its wait and sets
each worker's cancel event. This applies the equivalent here: wait in short
slices so a cancel is noticed while the slow calls are still running, give up
on the budget as a backstop against a tool that never returns, and report the
calls that were still running as error results rather than leaving holes in the
map the loop looks each id up in.
_resolve_media_path downloads a reply's media into the managed tmp dir so
upload_media_to_cdn has a local file to read, and nothing ever removed it
afterwards: the class had no os.remove for media, and no other component
prunes state_dir.tmp_dir(). Every reply carrying an http(s) URL therefore left
a file behind for the life of the install -- unbounded, and a 100 MB cap per
file rather than a total.
feishu_channel already wraps its own downloads in `finally: os.remove` (three
sites), so the two channels were handling the same action two different ways.
The cleanup cannot simply delete whatever path it was handed: a reply may also
name a file the user owns, and that has to survive the send. _resolve_media
reports whether the path was downloaded, and only the three senders that got
that flag remove it -- in a `finally`, so a failed upload or a failed send
call does not leak either.
_resolve_media_path keeps its old signature and behaviour for callers that
just want a path.
An Agent's avatar bytes live in <shared root>/avatars/<agent_id><ext>, which
is outside its workspace -- shared_root() is the default Agent's workspace, not
the deleted one's. delete_agent sweeps the roster entry, the workspace
directory, the session-prefs store and the project store, so the id ends up
reusable and clean everywhere except that one file.
Two consequences, and the second is the sharper one:
- delete_agent documents itself as removing an Agent "for good, files and
all", and the residue is unbounded: up to MAX_AVATAR_BYTES (2 MiB) per
deletion, accumulating in a directory nothing prunes.
- AgentAvatarHandler.GET resolved the file by id alone while POST checked the
roster first, so /api/agents/<deleted id>/avatar kept serving the deleted
Agent's picture to anyone who held the URL. The console would not render it
(views/agents.js only shows a picture when the roster says avatar ==
"image"), which is why it went unnoticed -- the endpoint itself did not care.
The id is the threat both times, exactly as the two store sweeps already
document: their comments say a new Agent reusing the id would inherit the
leftovers. The avatar sweep joins them, wrapped in its own try so one failing
store cannot swallow another's cleanup.
delete_avatar_files() is now the single place that knows how avatar files are
laid out; the upload handler reuses it for the "one Agent, one extension"
sweep it already did inline.
The read-only allowlist holds commands that "write only through a redirect"
-- but several of them write through a flag, which the redirect scan cannot
see. Only `sed -i` was special-cased, so a session the user explicitly
locked to read-only could still delete and overwrite files:
find . -type f -delete
find . -type f -exec rm {} ;
find . -type f -fprint stolen.txt
sort -o out.txt in.txt
xxd -r dump.hex out.bin
sed -e 'w written.txt' in.txt
awk 'BEGIN{system("rm -f victim")}'
`find -exec` and `awk system()` are worse than a stray write: they run a
command that never went through the allowlist at all.
The same commands are missing from the workspace-write path-mutating set, so
`sort -o /etc/out.txt` and `find /etc -delete` wrote outside the workspace
that mode promises to confine writes to.
Adds _READ_ONLY_WRITE_FORMS (the flag-driven forms per command) plus the sed
`w` script and awk `system()` cases, and shares the detection with the
workspace-write path so both modes refuse the same commands. The reading
forms are untouched: `find . -name '*.log'`, `sort -k2 in.txt`,
`xxd dump.hex`, `sed -n '1,10p' in.txt` and `awk '{print $1}' f.txt` all
still run.
upload_media is the one entry every reply type goes through — image, video,
voice and file — and its http(s) branch reached download_to_file, which bounds
the size and nothing else. The URL is whatever the Agent's reply carried, and
the bytes are then uploaded to DingTalk, so a prompt or a page it read could
point the fetch at this host:
http://127.0.0.1:8080/secret.pnghttp://169.254.169.254/latest/meta-data/iam/security-credentials/http://10.0.0.5/internal.png
All three were requested. Requests also defaults to following redirects, so a
public URL could 3xx inward even with a host check in place.
Fetch it the way the WeCom bot's reply path now does: through the shared
guard, with guard="always" because a delivery path is not a tool reaching a
dev server on request and web_security_ssrf_protection being off (the default)
must not turn it into one. The declared length is still checked before the
first chunk, and the body is still bounded as it arrives with a deadline, so
the cap that #3300 asked for is unchanged.
Co-Authored-By: XiaoChen <1326713348@qq.com>
Three senders grow their own http(s) branch — _send_image, _send_file,
_send_voice — and all three hand the URL to _download_remote_media, which went
to download_to_file. That bounds the size and nothing else. The URL is whatever
the Agent's reply carried, and the bytes go back out to WeCom, so a prompt or a
page it read could point the fetch at this host:
http://127.0.0.1:8080/secret.pnghttp://169.254.169.254/latest/meta-data/iam/security-credentials/http://10.0.0.5/internal.png
All three were requested. Requests also defaults to following redirects, so a
public URL could 3xx inward even with a host check in place.
url_safety already had the redirect-aware helper this needs, but its check is
opt-in: it defers to web_security_ssrf_protection, which is off by default so
that tools can reach a local dev server or a LAN MCP host. A delivery path is
not that — a global setting being off should not turn it into one — so add
assert_public_url, the same unconditional check the guard wanted, and let
safe_get take guard="always" to use it. The tool-facing default is unchanged.
The declared length is still checked before the first chunk, which
download_to_file did and save_response does not; the body is then bounded as it
arrives, and the file still lands atomically.
Co-Authored-By: XiaoChen <1326713348@qq.com>
A Feishu reply can name a remote image two ways, and only one of them checked
where it was going. Markdown images go through resolve_markdown_images, which
hands the URL to download_public_image: that resolves the hostname, refuses a
loopback / link-local / cloud-metadata address, and re-checks the resolved
address of every redirect hop with auto-redirect off.
ReplyType.IMAGE_URL went straight to download_bytes, which bounds the size and
nothing else. The URL is whatever the Agent's own reply carried, and the bytes
came back as an image_key — so anything a prompt or a page it read could steer
it into naming was fetched from the host it runs on and handed to the chat:
http://127.0.0.1:8080/secret.pnghttp://169.254.169.254/latest/meta-data/iam/security-credentials/http://10.0.0.5/internal.png
All three were requested and uploaded, while the Markdown path refused the same
three. Requests also defaults to following redirects, so a public URL could 3xx
itself inward even with a scheme-and-host check in place.
Use the guard that was already written for this. Name the multipart part by the
content type the server actually sent rather than by the URL's extension, which
a redirect chain need not have preserved, and share that mapping with the
Markdown uploader instead of keeping a second copy of it.
Co-Authored-By: XiaoChen <1326713348@qq.com>
The guard meant to keep the transcript and the speaker chunks in agreement
called CloudClient._alias_agent_id directly. That is a staticmethod over the
*global* registry, so the roster the test built was never the one under
comparison: on a machine whose default Agent is not the fixture's, the chunk
exit answered "main-agent" and the assertion failed. Wrapping the import in
try/except hid the other half of the problem — where the optional linkai stack
is absent the body never ran at all, and the guard passed without testing
anything.
Point the global at the roster under test, and skip only the import when the
optional stack is genuinely missing. The comparison now covers a
non-default Agent and the empty id too, not just the one value.
The rule itself moves into its own test that needs no optional import, so the
transcript's spelling is pinned even where the cross-module check cannot run.
CloudClient reports the default Agent to remote callers under the reserved
alias, because that is how they address it — AgentRegistry.get_addressed turns
a bare "default" back into whichever Agent really holds that id. The sender that
does this covers the chunks naming a speaker:
_SPEAKER_CHUNKS = ("speaker", "peer_start", "peer_end")
The shared transcript does not travel that way. _run_on_peer puts it on the
InvokeRequest itself, so it never passes through send_chunk_fn and keeps the
real id:
entry["agent_id"] = message.get("agent_id") or owner.id
The far side then reads a conversation in which the same Agent appears under both
names. Its speaker chunk says "default" while its history says "main-agent", and
it ends up attributing part of its own past to somebody who is not on its roster.
Fold the author the same way the chunk sender does, with the same rule: the
default Agent, and only the default Agent. A roster that cannot answer leaves
the stored author alone rather than losing the speaker.
Co-Authored-By: XiaoChen <1326713348@qq.com>
AgentRegistry.get_addressed treats the bare id "default" as the default Agent
whatever it was really given, and CloudClient._alias_agent_id reports the
default Agent to remote callers under exactly that id. So a delegation chain
that already ran here comes back naming that Agent as "default".
serve_invoke folds the aliases the caller supplied onto the local id before it
compares the chain, but the set it folds is seeded from addressed_id — only the
id this one request happened to be addressed by. When the target is named by its
real id the reserved alias is not in that set, and "default" survives into the
trace as if it were somebody else:
trace=["helper", "default"], addressed "main-agent"
-> served, and the trace handed to the next hop reads
["helper", "default", "main-agent"]
Two entries for one Agent. The cycle guard compares strings, so it sees a chain
that has already been here as a fresh one, and the roster that is supposed to
name who may be handed work carries the same ghost. The default Agent can hand
off to itself, one alias deeper, and max_depth is spent on a hop that never went
anywhere.
Add the reserved alias to the fold whenever the target is the default Agent. The
same helper covers the roster, since members go through local() as well.
Co-Authored-By: XiaoChen <1326713348@qq.com>
A delegated turn runs under a synthetic session id that has no row in
session_prefs, so the session-scoped permission mode cannot be looked up for it.
A local hand-off already handles that by putting the mode on the context it
hands to agent_reply:
inherited_mode = self._inherited_permission_mode(source.id)
if inherited_mode:
delegated_context["delegated_permission_mode"] = inherited_mode
and agent_reply reads exactly that key back:
agent = self.get_agent(..., permission_mode=context.get("delegated_permission_mode"))
A hand-off to a teammate in another process had no such line at any of the four
hops. InvokeRequest had no field, the wire body omitted it, and serve_invoke
built a context without it — so the far side read None and fell back to the
global mode. A caller running read-only handed its teammate a full-access turn,
which is the one thing a delegation must not widen. The two context key sets
were otherwise identical: comparing them left delegated_permission_mode as the
only difference, and it is the only one agent_reply reads.
Add the field, fill it from the same helper the local path uses, put it on the
wire only when there is something to say, and set it on the receiving side under
the key agent_reply expects.
Co-Authored-By: XiaoChen <1326713348@qq.com>
project_store and session_prefs are siblings: same key scheme, same atomic
write, same "an unreadable file falls back rather than crashing" story. They
differ in how they treat a file that parses but holds the wrong shape.
session_prefs checks it::
if not isinstance(data.get("sessions"), dict):
data["sessions"] = {}
project_store only setdefaults — which fills a key that is *absent* and leaves
a present one alone whatever its type. So a file carrying ``"sessions": []``
keeps the list, and every caller then calls .get() / .pop() / .items() on it:
get_project_map ValueError: dictionary update sequence element #0 ...
get_project_dir AttributeError: 'list' object has no attribute 'get'
delete_project AttributeError: 'list' object has no attribute 'items'
forget_session TypeError: pop expected at most 1 argument, got 2
set_project_dir TypeError: list indices must be integers or slices
The last two are the calls that would repair the file, so a bad shape also takes
away the way out. The project picker's GET catches the exception and answers
{"status": "error"}, which leaves the user with no project selector and no
session grouping until the JSON is fixed by hand.
Check each of the four keys against the shape its readers assume, the way the
sibling store already does for its one. A file that is not even an object gets
the same empty store. Unparseable JSON still falls back to empty and is still
left on disk untouched for the user to look at.
Co-Authored-By: XiaoChen <1326713348@qq.com>
A session leaves three places, not one. The conversation store is the obvious
one; the project binding and the pinned model/permission live in
``project_store`` and ``session_prefs``, which namespace their rows by Agent.
SessionDetailHandler.DELETE has always swept all three. SessionService — the
object behind ``dispatch("delete_session")``, which the cloud console drives
over the history channel — swept only the first, so a delete from there reported
success and left the other two behind. Neither leftover is reachable from the
conversation store, so clearing that one does not touch them.
Both outlive the session: a stale project binding keeps counting it in the
"spaces in use" figure the session list is grouped by and keeps the Agent's file
tools pointed at a project the user removed, and a stale model pin answers for
that Agent the next time the id is reused.
Sweep both, each on its own so a failure in one still lets the other be cleaned.
The Agent is resolved through the registry first, which is what the rows are
keyed by, and an unresolvable one skips the sweep rather than guessing at
``default::`` the way an unscoped call would.
Co-Authored-By: XiaoChen <1326713348@qq.com>
project_store and session_prefs both namespace their rows by Agent:
def _session_key(session_id, agent_id):
return f"{agent_id or 'default'}::{session_id}"
The DELETE handler already knows which Agent the session is on — it reads
agent_id from the request and uses it for the conversation store, the cancel key
and the queue key — but called forget_session(session_id) without it, so both
lookups resolved to ``default::<id>`` and dropped a row that does not exist.
The real row survives, and it is not inert. A surviving project binding keeps
the deleted session counted in the "spaces in use" figure the session list
groups by, and keeps pointing that Agent's file tools at a project the user
removed. A surviving model pin keeps answering for it, so the next run on a
freshly recreated session id silently uses a model the user never went back for.
Pass the Agent through, the same way every neighbouring call already does. The
unscoped form stays correct for the default Agent, which is what it was silently
assuming all along.
Co-Authored-By: XiaoChen <1326713348@qq.com>
DelegationPolicy.from_config holds timeout_seconds inside [0.01, 600] and
raises otherwise — a deliberate bound on how long a hand-off may block. The
sending side honours it by always passing the policy's own value, both on the
wire and into lock.acquire().
serve_invoke() took the timeout off the payload instead, so the clamp only ever
bound the value the *caller* picked and never the one it received. That value
is what _relay_lock(session_id).acquire() blocks on, and holding that lock is
what serialises hands-off to a relay session: a hand-off naming 100000 parked
the serving thread on the lock for close to a day, with the policy's ceiling
never consulted.
Cap the claimed wait at the policy's own timeout, which is also what the
sending side can send. A caller may still ask for less than the policy allows.
The `or` fallback is gone with it, so a payload naming 0 means "do not wait"
rather than silently becoming the policy value.
Co-Authored-By: XiaoChen <1326713348@qq.com>
DelegationPolicy.max_depth is meant to bound how many agents may sit between a
user's request and the Agent that answers it. The sending side grows its depth
one hop at a time — it reads the depth it was handed and adds one — so it can
only ever reflect the chain that actually arrived.
serve_invoke() read the same field straight off the wire instead, falling back
to ``len(trace) - 1`` only when the payload omitted it. The trace already *is*
the chain, so a present-but-wrong depth overrode the one value that could be
trusted, and was never checked for sanity. Two payload values defeat max_depth:
- a negative depth passes ``depth > max_depth`` outright (``-100 > 3`` is
False) and is then stamped into the delegated context, so every later hop's
``+1`` stays negative and the guard stops bounding the chain at all;
- an understated depth is taken at face value and becomes the base the next
hop builds on, so a long chain keeps counting from near zero.
Take the depth from the trace. A payload that omits it already got this value;
one that carries it now agrees with the chain or loses.
Co-Authored-By: XiaoChen <1326713348@qq.com>
serve_invoke() is the mirror of AgentDelegateTool's hand-off, and it re-derived
three of the four policy guards from the local config: max_message_chars, the
cycle check and max_depth. The allowlist was the one it skipped, so the ACL only
ran in the process that sent the hand-off — which makes it a property of the
caller rather than of the Agent being driven.
With `allowed_targets` configured, a payload naming a source that the ACL does
not list is served anyway, including one claiming to be this Agent itself
(`allows()` refuses source == target even with no allowlist set). The MODE_CLEAR
branch returned before the policy was read at all, so any source could drop this
Agent's transcript for an arbitrary conversation.
Move the policy read above the MODE_CLEAR branch and apply allows() to both
paths. The refusal names both ids, matching the tool's wording on the sending
side.
Co-Authored-By: XiaoChen <1326713348@qq.com>
`stream` is the client's choice and defaults to `False`, so
`_non_stream_completion` is the *default* path of the OpenAI-compatible
endpoint. It called `run_chat` without registering the run, while
`_stream_completion` does (`openai_compat.py:231`):
cancel_key, scoped_session_key = _request_cancel_scope(completion_id, session_id)
registry.register(cancel_key, session_id=scoped_session_key)
Cancellation goes through `CancelRegistry.cancel_session`, which reads the
per-session index that only `register` fills -- and that is what the console's
`/cancel` fast path calls (`channel/web/core/channel.py:1051`). So for a
non-streaming run it found nothing and told the user there was nothing to
cancel, while the agent kept working.
The client has no other way to stop it either: this surface returns a
`request_id` only once the reply is complete, and the non-streaming path has no
equivalent of `_FIRST_EVENT_TIMEOUT_SECONDS` to bound the run.
Register the run the way the streaming path does, and unregister it in a
`finally` so neither a completed nor a failed run leaves a cancellable entry
behind.
`_qr_login` fetches a QR code twice, and only the first fetch checks the
result:
qrcode = qr_resp.get("qrcode", "")
qrcode_url = qr_resp.get("qrcode_img_content", "")
if not qrcode:
logger.error("[Weixin] No QR code returned from server")
return {}
The refresh on expiry had no such check. `fetch_qr_code` only calls
`raise_for_status()`, so an HTTP 200 carrying an error payload comes back with
no `qrcode` at all -- and the good code already in hand was overwritten with
nothing. From there:
* `poll_qr_status("")` queries `get_qrcode_status?qrcode=`, which cannot
succeed, and
* `poll_qr_status` turns a `Timeout` into `{"status": "wait"}`
(weixin_api.py:321), so an invalid code looks exactly like a slow one.
`QR_MAX_REFRESHES` does not bound it: the counter only advances on an *expiry*
event, which a dead code never reports. The user watches a QR that will never
scan until the wall-clock timeout ends the loop, and the cloud console keeps
offering the same picture because `_current_qr_url` was set to it.
Apply the guard the first fetch already has, and clear the stale URL on the way
out so the console stops offering a code that cannot be scanned.
`WeChatMPMessage.__init__` parses three types and raises NotImplementedError for
the rest, so both callbacks have to filter before constructing. Each carried its
own copy of that list:
if msg.type in ["text", "voice", "image"]:
and the fallback logged `暂且不处理` -- not the type, not the sender.
WeChat MP delivers more than those three. `parse_message` hands these handlers
`link`, `location`, `miniprogrampage`, `shortvideo`, `video` and `unknown`, so a
shared link, a location or a video clip produces a 200 with body `success` (the
platform must not retry) and nothing else: no reply, no reaction, and one log
line that cannot distinguish an unsupported message from a mis-parsed one.
Declare the set on the class and gate on that, the way `FeishuMessage` already
does -- `SUPPORTED_TYPES` at feishu_message.py:29, consumed by
`feishu_channel.py:774` with a comment explaining why:
# Types we cannot parse (e.g. interactive cards posted by other bots in a
# group) get no reply either way; skip them here instead of raising, so
# they do not show up in the log as handler errors.
Now the two lists cannot drift: a type added to the parser becomes reachable
without editing the callbacks, and the log says what was dropped and whose.
Also drops a pre-existing unused `import time` that the repo's ruff F-class
gate flags on any PR touching this file (the gate is scoped to changed files).
`service.normalize_transport` is the single decision for what a server's
transport is: strip whitespace, fold case, map the four streamable-http
aliases onto one key, reject anything not in `_VALID_TYPES`, and default an
absent type by whether a url is present. `validate_server` returns that
normalized value and `save_servers` stores it, so anything written through the
console is already canonical.
`McpClient.__init__` re-implemented a weaker version of the same decision and
kept its own copy of the alias set. A hand-edited mcp.json never passes
`validate_server` -- `ToolManager._normalize_mcp_configs` converts the list/dict
shape and fills in a *missing* type, nothing more -- so the client is the last
place the value is interpreted, and it folded only the bare aliases:
stored ' http ' -> self.transport ' http ' (validator: 'streamable-http')
stored '' -> self.transport '' (validator: 'sse')
`initialize()` dispatches on exactly `stdio` / `sse` / `streamable-http`, so
both land in its `else`: "Unknown transport type", `initialize()` returns False,
the server is registered as failed -- and since nothing rewrites mcp.json, it
fails the same way on every start. The console's own test-connection button
reports the same server as working, because that path does go through the
validator.
Import the decision instead of repeating it. An unrecognised type still falls
back to the raw value rather than raising: the constructor is also reached from
the background loader, where one bad entry should not take down every other
server, and `initialize()` already reports it.
A delegated turn gets a session id synthesised per (source, target, root):
return f"delegate_{source}_{target}_{sha256(root_session)[:16]}"
Nothing ever writes prefs under that id, so on the way back through
`AgentBridge.get_agent` -> `apply_session_prefs` the lookup misses and the
target is handed `apply_permission_mode(None)`, which leaves it on whatever
`global_mode()` says -- `full-access` by default. A conversation the user had
set to read-only could therefore hand a task to a teammate, and the teammate
would answer with full write access.
`agent/subagent/runner.py:131-136` already does the equivalent for sub agents,
and says why:
# Same working directory and same permission mode as the parent: the tool
# copies already point at the parent's cwd, and delegating to a sub agent
# must not become a way around the session's permissions.
and `apply_session_prefs`' own docstring states the intent: "Permission stays
shared either way, because it bounds what this conversation may change
regardless of who is speaking."
Read the mode in force with `peek_agent` (which does not build an Agent, so
asking never spins up MCP connections and skills), put it on the delegated
context, and let `apply_session_prefs` prefer it. It is threaded through
`get_agent(permission_mode=...)`, defaulted to None so every other caller keeps
reading prefs.
Note the read-only gate is left as it is on purpose. `subagent` is in
`_KNOWN_TOOLS` and so is allowed under read-only -- correctly, because the mode
is copied onto the child. `agent_delegate` was allowed too, but by omission:
absent from the table, and `_MUTATING_NAME_RE` does not match the verb. Now
that the mode travels, both are allowed for the same reason, and tightening the
gate instead would break legitimate read-only delegation without closing
anything.
Creating a scheduled task validates `action.type` against a whitelist:
if action_type not in ("send_message", "agent_task"):
return json.dumps({"status": "error", "message": "unsupported action type"})
Editing one did not. The update route merged the patch and cleaned up
per-type fields, so a task the user legitimately owns as `send_message` could be
edited into a `tool_call` -- and the scheduler runs those through
tool = ToolManager().create_tool(tool_name)
result = tool.execute(tool_params)
which is not the agent's tool-calling path. It never reaches
`check_tool_call`, so the session's permission mode does not apply and nothing
is recorded in its audit. A `read-only` conversation that was careful never to
run `bash` could have a task do it on a timer. The trusted-recipient check on
the update route does not help: it only looks at receiver / instance /
channel_type, and changing `type` does not mark the action as changed.
Hoist the whitelist to `_ALLOWED_ACTION_TYPES` and apply it on both routes, so
a type a user cannot create is also a type they cannot edit a task into.
`FeishuRegisterHandler` keeps the registration session in a class-level dict.
Starting one replaces `_state` and sets the previous worker's `cancel_event` --
but the `lark.register_app` call already in flight still returns, and still runs
its callbacks.
One write site checked that it still owned the session; the other four did not:
if key in self.cache_dict: # _on_qr
if lark_install.needs_download(): # "downloading"
except ImportError: # "error"
cls._state["status"] = "done" # success, with the credentials
So a user who retried registration while the first attempt was still polling
could have the abandoned attempt's app_id/app_secret written into the new
session. The console reads `status=done` and persists them, which both signs
the channel with an app the user cancelled and discards whatever the live
session had recorded. The error branch's own comment already stated the rule:
# 仅当当前 state 仍属于本次 worker 时才写入，避免覆盖更新的会话
Extract that check into `_owns_session(cancel_event)` and apply it at all five
sites, so the rule lives in one place and the next write cannot forget it.
`_session_settings_state` reported `source: "session"` whenever the session
prefs carried a model, while `AgentBridge.apply_session_prefs` only applies
that pin when the conversation is owned and has no members:
is_group = bool(prefs.get("members"))
if owns_conversation and not is_group:
model.set_session_override(prefs.get("provider"), prefs.get("model"))
else:
model.set_session_override(None, None)
The docstring already promised the two resolve alike -- "the model resolves the
same way the runtime does" -- and `session-settings.js:221` reads
`source === 'session'` as a live pin, so a team conversation was shown a check
mark on a model no Agent answers with. Changing it does nothing, and nothing
says so.
Align the read side with the runtime's rule and report `pin_ignored`, so the
stored pin is visible as stored rather than presented as active. The `members:
[]` case stays a solo conversation, matching the runtime's truthiness test.
`_fail_callback` logged "Undrained reply cached for {key}, dropping" and then
discarded only `running`. The cache entry itself stayed.
That entry is what closes the door on the user. `passive_reply.py:51-54` starts
a task only when the cache is empty *and* the user is not already running, so a
leftover segment plus a cleared `running` makes the gate read False forever.
The next message falls through to the drain path instead of `produce()`: the
user gets the tail of the answer that just failed, and their new question is
dropped. The wait loop at `:105-115` breaks out as soon as `running` is empty,
so the timeout branch that would have masked this is skipped too. Only after
the stale entry has been drained does the message after that one work.
A failed task is not rare -- an LLM stream cut off or a raising tool is enough,
and a multi-turn answer caches a segment per turn (`wechatmp_channel.py:174`),
so the window is wide.
The discard also releases cached media the way `passive_reply.py:245-248` does
for a segment it cannot render. Five of the six cache sites hold a media_id
uploaded to the permanent material store, so a leftover entry leaks it.
`_ensure_image_format` promises "Ensure image is JPG or PNG (the only formats
wecom supports). Convert if needed" — and its except path returned the
*original* path, which is the one value that promise rules out.
Both callers already guard on an empty return:
callback :991 if not formatted: return None
websocket :1136 if not local_path:
self._send_text("[Image format conversion failed]", ...)
so neither guard could fire, and the failure notice written for the websocket
path was unreachable. An image Pillow cannot open — a truncated download, a
corrupt or exotic format — was uploaded as-is or base64'd into the callback
packet unchanged, and WeCom rejects it platform-side: the image never appears
and nothing tells the user why.
Returning "" instead is what both call sites already expect.
`_launch_browser` starts the Playwright driver and *then* dispatches to the
mode-specific launcher, so a launcher that throws leaves both handles live on
the service. `_run_loop`'s failure path returned straight away, skipping the
`_shutdown_browser()` further down -- the only place `_playwright.stop()` and
`_chrome_launcher.close()` happen.
Nothing else covers it. `_start_thread`'s timeout calls `close()`, but `_ready`
is set on the way out so it never waits; and `close()` itself returns on
`if not self._alive`, which the failure path had just set to False. The comment
above `close()`'s stuck-thread branch already describes this exact hazard --
"would stay resident for the rest of the process's life" -- so the gap is the
launch that throws rather than the one that hangs.
Both processes then stay invisible: the model only sees "Browser is not
available", and the spawned Chrome holds the profile's lock, so the next launch
walks into the same failure.
The cleanup is wrapped so a teardown that itself fails cannot hide the launch
error or leave a caller blocked in `ready.wait()`.
`save_servers` writes `{"mcpServers": {}}` when the last server is removed in
the console — the PUT handler passes whatever list the browser posted, and an
empty one is valid. The two readers of that file then disagree about what
"empty" means:
* `service.load_servers` (what the console shows) tests the key with
`is None`, so `{}` stays an empty map and yields no servers;
* `ToolManager._load_mcp_configs` (what actually boots) chained `or`, and `{}`
is falsy, so it fell through to the whole document and read the wrapper key
as a server name.
Measured through both real loaders:
reader A (console) : []
reader B (runtime) : [{'name': 'mcpServers', 'type': 'stdio'}] <- no 'command'
The ghost also defeats the `if not mcp_servers_config` short circuit in
`_load_mcp_tools`, so every agent init forks a loader thread for a server that
cannot start, and `_mcp_status['mcpServers'] = 'failed'` is never popped — the
only pop is in `_teardown_mcp_server`, keyed by the real server name. Because
the file on disk keeps producing the ghost, it returns on every boot until the
file is edited by hand.
Testing for the key being absent, rather than the map being truthy, is what
the console reader already does.
When the service window says no, `time_check` lets a short allowlist of
configuration commands through so an admin can still change the hours and
reload. That allowlist is written out by hand and had drifted from the command
table it mirrors:
godcmd ADMIN_COMMANDS["reconf"]["alias"] -> ['reconf', '重载配置']
time_check allowlist -> #reconf, #更新配置
So the Chinese spelling of the command was dropped, while `#更新配置` -- which
is not a godcmd command at all but a reload handled by ten models/*_bot.py
implementations -- was let through. Measured through the decorator:
send '#reconf' -> reached handler: True
send '#重载配置' -> reached handler: False
send '#更新配置' -> reached handler: True
An admin who types the command the way the rest of godcmd's admin commands are
typed gets silence off hours: the message is dropped with only a log line, so
there is no reply to tell them the reload did not happen.
`#重载配置` now passes alongside `#reconf`. The test is written against godcmd's
own alias table rather than a copy of it, so the next alias added there fails
here instead of being silently dropped.
`reload_plugin` dropped the plugin's entry from the in-memory config on the way
in, so the fresh instance would re-read its settings. That only ever worked for
a plugin carrying its own `config.json` in its bundle: `Plugin.load_config`
falls back to that file and never looks at `plugins/config.json`, which is
where a plugin configured at runtime actually lives. For such a plugin the
entry was simply gone and nothing put it back.
Measured through the real `PluginManager` and the real `Godcmd`:
before reload : {'password': 's3cret', 'admin_users': ['alice']}
reload_plugin returned: True
live instance : password='' admin_users=[] temp='9623'
`#reloadp Godcmd` answered "插件配置已重载" while clearing the admin password
and the admin list, and logged a random four-digit temporary password in their
place - the admin has locked themselves out until the process restarts. The
removal also ran before the membership check, so `#reloadp <a name that is not
loaded>` erased that plugin's settings before discovering there was nothing to
reload.
Reloading a plugin's code is not the operation that reloads its configuration;
`#reconf` is, and it is what a user reaches for after editing config.json. So
the reload keeps the configuration it already has.
`remove_plugin_config` had no other caller, so its import goes with it. The
function itself is left alone in `config.py` in case something outside this
repository imports it.
Session ids are only unique within one Agent, so the cancel and steer
registries are keyed by a scoped id - `AgentBridge.scoped_session_key`, which
returns `"{agent_id}::{session_id}"` for anything but the default Agent. Every
submit-side caller already namespaces: `steer_session`, the web sessions API
(`sessions.py:417`) and `cloud_client.py:1538`. `agent/chat/service.py:389`
states the rule outright:
# Both the token and its session grouping are namespaced: two Agents
# serving the same session id must not cancel or steer each other.
`agent_reply` followed it for the cancel token and not for the steer inbox.
The scoped id was computed 130 lines earlier for the cancel registration and
then left; the steer registry was keyed by the raw `session_id`. So the two
halves disagreed:
* a steer aimed at a non-default Agent submitted to `"agent-a::s1"`, found an
empty bucket and came back `INACTIVE` - the run was told to change course
and never heard it;
* when the default Agent had a run under the same bare id, the bucket held two
inboxes and `submit` returned `AMBIGUOUS`, so neither run received it - and
with a single inbox the instruction landed in the other Agent's run.
`agent_reply` now registers and unregisters under the same scoped id it already
computes for the cancel token, which is hoisted out of the `if token_key` block
so it is defined whenever there is a session. The default Agent keeps the bare
key, so existing installs are unaffected.
`compact_context` took `messages_lock` twice with a slow LLM summarize call in
between: it snapshots the history, releases the lock, calls
`flush_mgr._summarize_messages` (a network round-trip), then re-acquires the
lock to write the compacted history back as a whole new list.
A turn that completes inside that window writes `self.messages` the same way
(`agent.py:869`, `self.messages = list(executor.messages)`), so the write-back
overwrote it. The user asked something, the Agent answered, and both messages
were gone - with the call still reporting `ok: True` and no log line.
`POST /api/sessions/<id>/compact_context` hands over the live agent from
`peek_agent` without checking whether a turn is running, and the console's
compact button sits next to the message box, so this needs nothing exotic.
The write-back now re-checks the snapshot it was built from. Messages that
arrived meanwhile are appended after the compacted history rather than dropped,
and if the history was rewritten rather than extended - an automatic trim does
that - the compaction declines with a new `history_changed` reason instead of
writing back a summary of a history that no longer exists.
Holding the lock across the summarize call would have been simpler but would
block every message append for the duration of an LLM call.
`ChatChannel._send_reply` sends an image reply's caption as a text bubble and
then hands the same reply object to `send()`:
```python
elif reply.type == ReplyType.IMAGE_URL and hasattr(reply, 'text_content') and reply.text_content:
text_reply = Reply(ReplyType.TEXT, reply.text_content)
self._send(text_reply, context)
time.sleep(0.3)
self._send(reply, context)
```
DingTalk's image branch sent the caption again on its way to the picture, so
every image the Agent produced arrived with its explanation printed twice.
The channels that get this right show which of the two sends counts: feishu
(`:922`), qq (`:581`) and wecom_bot (`:1045`) only upload the image and leave
`text_content` alone, because the base class already said it. DingTalk was the
only one repeating the work.
`send()` is also called directly, without going through `_send_reply` — the
scheduler forwards a FILE / IMAGE_URL reply untouched
(`agent/tools/scheduler/integration.py:1122`). So the caption cannot simply be
dropped: `_send_reply` now records that it has sent it and DingTalk checks that,
the same way the voice path already coordinates with feishu's streaming via
`context["voice_reply_text"]` / `context["feishu_streamed"]`. The FILE and
VIDEO branch keeps sending the caption itself, since the base class does not
pre-send one for those types.
A SKILL.md is hand-written (or delivered by the cloud skill market), so the
`install:` entries in its frontmatter are whatever the author typed. An empty
`kind:` parses as None, and `parse_metadata` called `.lower()` on it:
```python
kind = spec_raw.get('kind', spec_raw.get('type', '')).lower()
if not kind:
continue
```
The `if not kind` guard is the "skip an entry with no kind" intent, but the
`.lower()` one line above raises `AttributeError: 'NoneType' object has no
attribute 'lower'` before it can run. `kind: 1` reaches the same line.
The blast radius is the whole skill library rather than the one bad file,
because the parse happens while `load_all_skills` is still building entries:
* at construction, `agent/protocol/agent.py:171` swallows it into
`skill_manager = None`, so that Agent has **no skills at all** and the
`<available_skills>` section of the system prompt is permanently empty;
* mid-session, `agent.py:297` rebuilds the prompt every run and
`agent.py:324` swallows it again, falling back to the cached prompt — so
the tool list, runtime info and permission block all stop updating.
Only machines with PyYAML installed are affected (the non-YAML fallback parser
never reaches this branch), which makes it present as an environment problem
rather than a configuration one.
`kind` now goes through `_normalize_kind`, matching how the same module already
handles its sibling fields: `bins`/`os` through `_normalize_string_list`, and
`always`/`extract` through `parse_boolean_value`. The lower-casing and
trimming the original `.lower()` was there for is kept.
The channels page renders a credential as a mask and posts that mask straight
back on the next save, so every save path has to recognise the mask and leave
the stored secret alone. Those three guards required four consecutive stars,
while `_mask_secret` emits one star per hidden character - so a credential of
9 to 11 characters came back as a one-to-three star mask that the guard read as
a real secret. The save then persisted the mask over the original, the next
connect attempt used a credential the user never typed, and the UI still only
showed the mask, so the original was unrecoverable.
The three call sites now share one predicate that recognises the shape
`_mask_secret` produces instead of counting stars, which is what let the mask
and its guard drift apart in the first place.
DeepSeek, Doubao and Moonshot all read the failure body the same way:
response = res.json()
error = response.get("error", {})
logger.error(f"... status_code={res.status_code}, msg={error.get('message')}")
Two of those assumptions are not the caller's to make.
The body is not always JSON. A gateway in front of the API -- Cloudflare, an
nginx error page -- answers a refusal with HTML, so `res.json()` raises
`ValueError`. Qianfan reads the same shape through `_error_result`, which
guards it, and `models/openai/openai_http_client._parse_response` falls back to
`{"raw": resp.text}`; these three were missed.
And the body does not always carry an `error` key. Moonshot read
`response.get("error")` with no default, so a FastAPI-style
`{"detail": "..."}` left `error` as `None` and `error.get("message")` raised
`AttributeError`.
Either exception lands in the method's own `except Exception`, which does
`time.sleep(3)` and calls `reply_text` again -- re-sending the request that was
just refused, twice -- and then answers 我现在有点累了，等会再来吧 instead of the
401/429 wording the branch exists to produce.
Doubao's `reply_text` also dereferenced `args` unconditionally
(`body = args.copy()`), while `agent.chat.session_service.generate_session_title`
and `optimize_prompt` call `bot.reply_text(session)` with no args at all. Every
other provider guards it (`dict(args) if args else dict(self.args)`,
`if args is None: args = self.args`, `args.copy() if args else {}`), so a
Doubao install never generated a session title and never optimised a prompt:
both raised before the request, retried twice, slept 3s each time, logged a
traceback and fell back to the truncated user message.
Measured, with the provider answering 401 and an HTML body:
* `reply_text`: 3 requests, answered 我现在有点累了，等会再来吧
-> 1 request, answered 授权失败，请检查API Key是否正确
Measured, with Doubao's `reply_text(session)` called the way session_service
calls it:
* before: 0 requests, answered 我现在有点累了，等会再来吧
* after: 1 request, answered with the model's output
`tests/test_provider_error_body.py` covers the three providers and both
requests.
The Wenxin chat endpoint carries its credential in the query string:
url = ("https://aip.baidubce.com/rpc/2.0/ai_custom/v1/wenxinworkshop/chat/"
+ session.model + "?access_token=" + access_token)
and requests writes the whole URL, query included, into the message of the
exception it raises when the call fails ("Max retries exceeded with url:
...?access_token=..."). `reply_text` then does two things with that message:
logger.warn("[BAIDU] Exception: {}".format(e))
result = {..., "content": "出错了: {}".format(e)}
so a single network blip writes a live credential into run.log -- which the
console serves as a complete file download -- and shows it to the user, because
a zero-token result becomes `Reply(ReplyType.ERROR, reply_content)`.
`get_access_token` has the same shape, with `client_secret` in the query, and
its failure reaches the same two places.
The channels were scrubbed for this (wechatcom, dingtalk, wechat_kf); the model
providers were missed. `common.utils.scrub_secrets` is that scrub, shared, and
`reply_text` masks the message once before either use -- mask, not drop, so the
failure stays readable to whoever is debugging it.
Measured, with the request failing the way requests fails:
* before: 出错了: HTTPSConnectionPool(...) ... chat/ernie?access_token=BAIDU_... (Caused by ...)
* after: 出错了: HTTPSConnectionPool(...) ... chat/ernie?access_token=*** (Caused by ...)
`tests/test_baidu_wenxin_token_not_logged.py` covers both surfaces and both
requests.
LinkAI answers a refusal in more than one shape, and `_chat` and `reply_text`
only knew OpenAI's:
response = res.json()
error = response.get("error")
logger.error(f"... msg={error.get('message')}, type={error.get('type')}")
LinkAI's own body is `{"code", "message"}`; a misconfigured base gets FastAPI's
`{"detail": "Not Found"}` (`_explain_linkai_error` names that very case); and a
proxy in front of either answers with an HTML page that is not JSON at all. For
all three, `response.get("error")` is None, so `error.get("message")` raised
`AttributeError` -- into the method's own `except Exception`, which sleeps two
seconds and re-runs the request that was just refused. A rejected call reached
the user as 请再问我一次吧, after the retry bound and two extra round trips, and
the 429/409 wording the branch exists to produce was unreachable.
Measured, against a 400 carrying `{"code": 40001, "message": "余额不足"}`:
* `_chat`: 3 requests, answered 请再问我一次吧 -> 1 request, 提问太快啦
* `reply_text`: 2 requests, answered 请再问我一次吧 -> 1 request, 提问太快啦
* a 502 HTML page: still retried, still 3 requests, still 请再问我一次吧
`_linkai_error_body` is the read `_handle_linkai_sync_response` already used
(`.get("error", {})` with a `res.text` fallback); it now serves all three, so a
caller that only wants to log a failure has one place to read from.
`tests/test_linkai_error_envelope.py` covers both branches and the retry control.
LinkAI answers a refused call -- out of credits, a blocked prompt, a bad
parameter -- with HTTP 200 and a non-zero `code` in the body, so the status
alone never says whether the call was accepted. Each module that talks to it
reads that envelope twice (`if res.status_code == 200` and then
`if res.get("code") == 200`), and each got one branch wrong.
`MJBot.generate` and `MJBot.do_operate` only returned inside `if code == 200`.
A non-zero code fell off the end of the method, so `process_mj_task` stored
`None` in `e_context['reply']` and a `$mj` on a refused call produced no message
at all -- where a non-200 HTTP status does answer with `图片生成失败`.
`LinkSummary.summary_chat` and `_parse_summary_res` had the mirror image: their
error branch called `res.json()` unguarded. The body of an error page is not
JSON (a gateway's HTML 502, say), so reporting a failure raised `ValueError` out
of `summary_url` / `summary_file`.
`Util.parse_linkai_response` now applies the "status and body" rule in one place
and always hands back a dict for `data`; `fetch_app_plugin` goes through it too,
where `res.json().get("data").get("plugins")` raised `AttributeError` on the
same envelope and the blanket `except` swallowed it, answering `False`.
Measured before -> after, with the API answering HTTP 200 and
`{"code": 40001, "message": "余额不足"}`:
* `generate` / `do_operate`: returned `None`, so no reply was reachable ->
now return the same `Reply(ReplyType.ERROR, ...)` a non-200 status produced
* `_parse_summary_res` with a 502 HTML body: raised ValueError -> returns None
* a good `{"code": 200, "data": ...}` body still parses as before
`tests/test_linkai_response_envelope.py` covers the three.
Incidental: `plugins/linkai/utils.py` carried an unused `except ... as e`; the
F-class gate scans whole files, so it had to go for this file to stay green.
`FileCache.cache` is a process-wide dict keyed by the inbound sender id, and
its TTL was only enforced inside `get()`. A batch whose session never sent the
follow-up text was never revisited again, so it stayed in the cache for the life
of the process. `cleanup_expired()` — the only global sweep — had no caller
outside the tests.
`add()` now sweeps before it touches the requested batch, so any batch that went
stale beside it is reclaimed on the next write. `cleanup_expired()` accepts an
optional clock so the sweep can be tested without sleeping.
WeCom answers these callbacks with HTTP 200 whether or not it took the
content, so errcode in the body is the only signal -- the same envelope the
long-connection replies in this file already read. _send_via_response_url
claimed the stream up front and then only logged the reply at info level, so
a refused push left url_sent set and the next poll finished with empty
content: the answer was dropped and the log said it had been sent.
The cloud client and the feishu one-click setup rewrite config.json in
place, so the truncation that opens the write happens before the new
bytes exist; json.dump streams, and a failure partway through leaves a
partial config.json that the next load_config reads as corruption. Both
now write through the shared write_json_atomic helper.
DingTalk answers HTTP 200 with a non-zero `errcode` when it rejects a send, so
the status alone is not an acknowledgement. `send_group_message` and
`_send_file_message` were fixed for this, and `_upload_media` and
`_reply_markdown_or_text` in the same file already read the body, but the two
image paths stayed on the status-only check.
`send_image_with_media_id` never parsed the body; `send_image_message` parsed it
into `result` and only used it in the error log. Both returned True for a
rejected image, so the `if not success:` branch in `send()` -- the one that tells
the user "抱歉，图片发送失败" -- stayed dead and the dropped image went
unreported.
Verification:
- 4 new cases in tests/test_dingtalk_image_errcode.py fail before, pass after
(HTTP 200 with errcode=40035 must be False; errcode=0 must stay True).
- tests/ -k dingtalk: 67 passed.
- ruff check --select F --ignore F403,F405 on both files: All checks passed.
- git diff --check: clean.
Each channel that receives an attachment pulls it from the platform by
`media_id`, and each of these call sites open-coded the same three lines:
`response = client.media.download(...)`, `if response.status_code == 200`,
then `f.write(response.content)`. `weixin_api.download_media_from_cdn` did the
same thing one layer down, decrypting `resp.content` in one piece. Every one of
them buffered a body of any size into memory whole and then wrote it with no
cap at all, so an oversized or endless answer landed on disk before anything
could object.
The rest of the tree already reads through `common.media_download` -- feishu,
dingtalk, qq and slack on the inbound side, wechatmp and wechat_kf on the
sending side -- so this is the last group still doing it by hand.
`wechatcomapp`, `wechatmp` and `wechat_kf` inbound voice / image / file now go
through `save_response` with the same `MAX_FILE_BYTES` / `MAX_IMAGE_BYTES` the
other channels use. `weixin` uses it on the GET its own `_resolve_media_path`
opens (`stream=True`), and the CDN download reads its body with `read_response`
before the AES step -- that also moves the read ahead of the key parsing, so a
malformed key no longer leaves the connection open.
Each call site shrank to a couple of lines, and the failure is logged and
swallowed the way the neighbouring adapters do it: `prepare()` has no handler
of its own above `_prepare_fn`, so an exception there would abort the turn
instead of degrading it.
Measured against a 2 KiB cap in the new tests, before -> after:
* wechatcomapp / wechatmp / wechat_kf: each wrote the oversized body into the
tmp dir -> every one now writes no file and logs the failure
* weixin `_resolve_media_path`: resolved to a 2048-byte tmp path -> now
returns "" and leaves no `.download_*` temp file behind
* weixin CDN: decrypted the whole body -> now refuses before the AES step
`tests/test_inbound_media_download_bounds.py` covers the three adapters and the
CDN path; the weixin URL case joins the tmp-dir module it already had.
Incidental: `channel/weixin/weixin_api.py` carried an unused `struct` import
and an f-string with no placeholder; the F-class gate scans whole files, so
both had to go for this file to stay green.
Windows Notepad / PowerShell write UTF-8 with a BOM by default, and the JSON /
txt files CowAgent asks users to edit by hand are exactly the ones that get
re-saved that way. Plain `utf-8` turns the leading BOM into a `\ufeff`
character, and what happens next depends on the reader:
- `plugins/*/config.json` (godcmd's is created on first run for the operator
to fill in): `json.load` raises "Unexpected UTF-8 BOM", the plugin's
`__init__` fails, and `PluginManager.activate_plugins` answers a failed init
by disabling the plugin and **persisting** `enabled=false` -- so it stayed
off across restarts even after the file was put right.
- `banwords.txt`: the first line is read as `\ufeffword`, `str.strip()` does
not remove U+FEFF (it is not whitespace), so the word is registered with the
BOM glued to it and can never match. No error -- the first word of the list
is silently unfiltered.
- `skills_config.json`: the read helper swallows the error and answers `{}`,
and `cow skill list` rebuilds the file assuming `enabled: True` -- a skill
the user had turned off comes back on, silently.
- `config.json` in `cow backup`: same swallow, so every caller treats the
install as unconfigured and `cow backup` archives `~/cow` instead of the
configured `agent_workspace`. `restore` hits the same read on the copy inside
the archive, where it is not caught and aborts the restore.
- `mcp.json`: `_read_mcp_file` wraps it as `McpConfigError("mcp.json is not
valid JSON: ...")`, which names the JSON rather than the BOM, so the file
looks fine in an editor.
Apply the convention `agent/admin.py` already documents -- "utf-8-sig tolerates
a UTF-8 BOM (e.g. config.json edited with Windows Notepad / PowerShell)" -- at
every one of those read sites, replacing what were five separate PRs (#3419,
#3421, #3422, #3423) with one change.
`Plugin.load_config` also restores the `if os.path.exists(plugin_config_path)`
check inside the `if not plugin_conf` block that assigns `plugin_config_path`:
pulled out of it, the check ran on a name that was never assigned whenever
`pconf()` already returned a config, i.e. for every plugin with an entry in
`plugins/config.json`, and `load_config` raised `UnboundLocalError` -- the same
"plugin gets disabled" outcome the change was meant to avoid.
`tests/test_plugin_config_defaults.py` covers that case; it fails on the
previous revision with `UnboundLocalError: cannot access local variable
'plugin_config_path' where it is not associated with a value`.
Three places replayed a log tail with `readlines()` and then sliced the result:
the web logs view (last 200 lines), the chat `cow logs` command (up to 50) and
`cow logs` on the CLI (50). `run.log` is appended to for as long as CowAgent
runs and is never rotated, so all three read every line ever written to display
a handful of them -- and the chat command does it inside the bot process, where
a long-lived instance gets OOM-killed by its own help command.
Add `common.utils.tail_lines`, which walks the file backwards in 8 KiB blocks
and stops once enough lines have been collected, and use it from all three:
- `channel/web/api/logs.py` for the initial replay,
- `plugins/cow_cli/cow_cli.py` for `cow logs` in chat,
- `cli/commands/process.py::_print_last_lines`, imported on use like the CLI's
other project imports (the CLI ships as its own package and only puts the
project root on `sys.path` when it needs something from it).
Measured on a 20000-line / 1.0 MB run.log: the three views went from reading
100% of the file to 1.6%, 0.8% and 0.8%, with no `readlines()` call left.
Also drops the two pre-existing pyflakes findings in `cli/commands/process.py`,
since the CI lint gate requires the files a PR touches to be F-clean: the dead
`flag` assignment in `_kill_pid` (the `/F` flag is appended directly) and the
`f` prefix on a string with no placeholders.
A timed-out `sendMessage` used to answer with a synthetic `{"ret": 0, "msgs": []}`,
so a reply whose response never arrived looked delivered to every caller that
reads `ret` to decide whether anything went wrong.
The two timeout kinds mean different things, so handle them apart instead of
treating them alike:
- `ConnectTimeout` (along with `SSLError` / `ConnectionError` /
`ChunkedEncodingError`) means the request never reached the peer: nothing was
delivered, so it stays retryable and is raised once the retries run out;
- `ReadTimeout` means the request *did* reach the peer and only the response was
lost, so a send raises right away. Retrying would risk delivering the same
message twice, and would leave the caller waiting out the backoff first;
- `get_updates` keeps reading a read timeout as an empty poll, logged at debug
level as before: the server holds the long poll for up to
`DEFAULT_LONG_POLL_TIMEOUT` seconds, so no answer within that window means no
message arrived, not that the request failed.
Also drops the two pre-existing pyflakes findings in this file, since the CI
lint gate requires the files a PR touches to be F-clean: the unused `struct`
import and the `f` prefix on a string with no placeholders.
`common/media_download.py` already streamed and counted a body, but only
behind its own `requests.get`, so the POST-based TTS calls could not reuse it
and each one open-coded the same stream/count/delete loop -- the same work as
#3333, #3334, #3335, #3338 and #3339. Add public helpers that take a response
the caller already holds, and switch every call site to them.
`common/media_download.py`
* `save_response(response, path, max_bytes, max_seconds)` streams an open
response into a temp file next to `path` and `os.replace`s it into place
only once the whole body arrived within the cap, so a failed or oversized
download never leaves a partial file behind or clobbers an existing one.
Closes the response on the way out, so callers need no `finally`.
* `read_response(response, max_bytes, max_seconds)` is the same accounting
for callers that need the bytes in memory.
* `download_to_file` / `download_bytes` now build on them.
Call sites, each of which shrank to a couple of lines: the custom / openai /
linkai / zhipuai / ali TTS paths and the vision image download. MiniMax keeps
its own loop -- the audio arrives as hex-encoded SSE frames rather than one
body, so it cannot call the helpers -- but caps against the same shared
`MAX_FILE_BYTES`.
Measured against the same oversized endpoint answers, before -> after:
* custom / openai / linkai / zhipuai / ali: each wrote a 104,857,601-byte
file into the temp dir -> every one now refuses with a `ReplyType.ERROR`
(ali returns `None`) and writes no file at all.
* vision: built a 27,962,050-char base64 data URL -> now raises
`VisionAPIError`.
* minimax: buffered a 1 MiB-per-frame stream without limit and wrote it out
-> now refuses with an ERROR once the decoded audio passes the cap.
`tests/test_media_download.py` covers the helpers plus one case per caller, so
there is no new test module per backend.
8d3138a9 rewrote the MCP guide and introduced the "Connection test" wording,
which is also what turned the docs contract test red on master. That test has
since dropped its doc wording assertions (dae6bc6d), so this only keeps the
guide in line with the label both consoles use: mcp_test is "Test connection"
in the web console (channel/web/static/js/core/i18n.js:1461) and in the desktop
app (desktop/src/renderer/src/i18n.ts:1393).
## Problem
Both WeCom kf endpoints take the credential as a query parameter:
url = f"{KF_API_BASE}/sync_msg?access_token={self.client.access_token}"
`requests` puts the entire URL -- query string included -- into the message
of the exception it raises when the call fails ("Max retries exceeded with
url: ..."). The two HTTP wrappers log that raw exception, so a single failed
sync or send writes a live access token into run.log. The web console serves
run.log as a complete file download (LogsDownloadHandler), turning any
routine network blip into a credential leak. wechatcom and dingtalk already
scrub these URLs; wechat_kf was missed.
## Change
Add `_scrub_secrets()` which replaces every `access_token=...` value with
`***`, and wrap the exception messages in the `sync_msg` and `send_msg`
wrappers with it before logging. Non-credential text is passed through
unchanged, and the masked token is returned in the error payload so callers
still see a useful failure reason.
## Verification
- fail-before: on un-fixed upstream, 2 leak tests fail (token reaches the log) and 2 helper tests error (the new `_scrub_secrets` helper does not exist yet) -> 4 failed.
- fail-after: 4 passed (2 leak tests green + 2 helper tests green).
- ruff: `ruff check channel/wechat_kf/wechat_kf_channel.py tests/test_wechat_kf_token_not_logged.py --select F --ignore F403,F405` -> All checks passed.
- `git diff --check` -> clean.
`ChatChannel.consume` is the message pump every channel subclass starts in
`__init__`, and its `while True` body is bare. Any exception raised while
looking at one session therefore unwinds out of the loop and kills the pump
for the rest of the process lifetime: messages keep piling up in the
per-session queues and the bot goes silent while the process stays up.
Two ways to raise in there today:
* `assert len(self.futures[session_id]) == 0` — an assert used as a runtime
check, which also disappears under `python -O`;
* `self.futures[session_id]` is only created once a context has been
submitted, so a session that never got one raises `KeyError` on that line
before the assert is even reached.
Move the per-session work into `_consume_session`, log failures and carry on
with the next session, and turn the assert into a warning that keeps the
session (and releases the semaphore it took) instead of ending the loop.
The refresh thread started by `_active_refresh` is the only thing that
renews the WeCom-app access token, and its loop wrapped no error handling
at all: one failed `fetch_access_token` call -- a DNS blip, a gateway 500,
a wrong `secret` -- unwound out of `refresh_loop` and killed the thread for
the rest of the process lifetime. The token then stayed expired (WeCom
issues them for two hours) and every later API call failed until CowAgent
was restarted, with nothing in the log pointing at the real cause.
Move the loop body into `_refresh_once_if_needed` and log transient
failures instead of propagating them, so the next 60s tick retries.
`Channel.NOT_SUPPORT_REPLYTYPE` defaults to `[VOICE, IMAGE]`, and every channel
that can do better overrides it: the ten that send images set `[]`, and the two
that still cannot send voice (terminal, web console) narrow it to `[VOICE]`.
`QQChannel` never overrode it, so it inherited `[VOICE, IMAGE]` while shipping
`_send_image` (URL and local file) and `_send_media`.
`ChatChannel._decorate_reply` therefore rewrote every `ReplyType.IMAGE` reply
into `ReplyType.ERROR` -- "不支持发送的消息类型: ReplyType.IMAGE" -- before
`send` ever saw it. IMAGE_URL is a separate member and was not in the list, so
images that arrive as a URL were delivered while images produced locally were
refused.
Declare `[VOICE]`, which is what this channel actually cannot send: no voice
branch exists in `send`, so a voice reply would otherwise fall through to the
text fallback and hand the user a tmp path.
Once the polling loop reported success, `_long_text_synthesis` downloaded
`audio_address` -- the URL the task API hands back -- with
`requests.get(...).content` and wrote it into the managed tmp dir. Nothing
capped either the read or the write, so an endless or multi-gigabyte response
was buffered whole and written out before anything could object.
Download through `common.media_download.download_to_file` with
`MAX_FILE_BYTES`, the counted download every other media fetch in the repo
uses. It also writes through a temp file, so an oversized or truncated
download leaves nothing behind. A failed download now returns an ERROR Reply
instead of falling through to a file that was never written.
DashScopeVoice._download_audio buffered the whole response body with no size
limit, so a hostile or broken TTS audio URL could fill disk (probe P4 —
unbounded media download). Route it through the size-capped
common.media_download.download_to_file (100 MB cap); the import is aliased
because this module already defines its own MAX_FILE_BYTES for ASR input.
The method already degrades to None on any download failure, so an oversized
response is refused the same way. Adds a regression test.
Co-Authored-By: WorkBuddy <noreply@workbuddy.ai>
SkillService._download_file buffered the whole response body with no upper
bound, so a hostile or broken URL could fill disk. Route it through the
size-capped common.media_download.download_to_file (100 MB cap) instead,
matching the other download paths already hardened the same way.
Adds a regression test asserting an oversized response is refused with
MediaTooLargeError and nothing is written to disk.
Co-Authored-By: WorkBuddy <noreply@workbuddy.ai>
upload_media() fetched HTTP(S) URLs with a bare requests.get and
f.write(response.content), so an oversized link could exhaust memory/disk.
Reuse the shared download_to_file helper, already used by Feishu (#3300) and
dingtalk_message.download_image_file.
QQ's private copy of the filename guard drifted from the canonical one in
channel/chat_message.py: it never capped the name at 180 chars. An over-long
sender filename makes _download_attachment's download_to_file() raise
OSError(ENAMETOOLONG) and the attachment is silently replaced with a
"[... download failed]" placeholder. Re-sync the copy (also harden basename
handling for backslashes and "."/"..").
Uploading a remote video used a bare requests.get + file.write(response.content)
with no byte cap (P4). Route the HTTP branch through download_to_file(...,
MAX_FILE_BYTES) so a huge or endless response is refused, matching the
image/file paths in the same file.
chat_channel.py hands an agent-produced video to channels as
Reply(ReplyType.VIDEO, "file://" + path). WechatKfChannel.send dispatches
ReplyType.VIDEO_URL but never ReplyType.VIDEO, so a locally generated video
fell through to the trailing else and was only logged as
[wechat_kf] unsupported reply type: VIDEO
send() is the whole outbound contract for this channel and _send_reply() has
no per-type fallback, so the user received nothing at all -- not even the raw
file:// path the other channels used to leak before they were fixed.
The asymmetry is unique to wechat_kf: every sibling that dispatches VIDEO_URL
also dispatches VIDEO (weixin, wechatcom, wechatmp, wecom_bot, qq), and the
three channels that used to share this gap got the same one-line treatment
(#3289 discord, slack/telegram). _send_video() already exists and the video
branch already reads through _read_media(), which loads a file:// path from
disk and size-caps a genuine http(s) download, so no new plumbing is needed.
Tests: a file:// VIDEO reply is uploaded as "video" and handed to
_send_video(), and requests.get is never called for it; a remote VIDEO_URL
reply keeps its existing download-and-send behaviour.
The sibling is now `.skills_config.json.<pid>.<tid>.tmp` instead of
`skills_config.json.tmp`. agent/evolution/executor.py lists the dot-prefixed
shape in _WATCH_IGNORE_PREFIXES, so the workspace scanner does not read the
in-flight replacement as a real change, and a per-writer name keeps two
overlapping saves from renaming each other's partial file.
The test assertion that named the old temp path would have passed vacuously
after this, so it now checks the directory contents instead.
c020627
