_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.
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.
The four in-place writers in this plugin truncated a live config file before
writing it, so a crash, a kill, or a full disk partway through left it
half-written. For config.json that is fatal on the next start: load_config
re-raises on a parse error unless the packaged desktop build is running, and
the desktop-only recovery path exists precisely because of "a bad write
during a previous update". For skills_config.json, uninstall swallowed the
exception and still reported success while every record was gone.
Serialise into a dot-prefixed sibling and os.replace it into place, matching
what agent/skills/manager.py and channel/web/core/_common.py already do for
these same two files.
Problem
-------
plugins/banwords and plugins/godcmd both repair a config.json that exists
but cannot be used -- ``{}``, or only one of the two required keys -- and
write the repaired document back with ``open(..., "w")``, which truncates
the file before the new bytes are there. The repair runs precisely because
the file on disk is already unusable, so a write that dies halfway (a full
disk, the process being killed) turns "empty" into "unparseable" instead
of fixing anything.
Reachability
------------
The comments next to these blocks already spell out what an unparseable
config costs: Plugin.load_config cannot read it, ``__init__`` raises on the
missing keys, and ``PluginManager.activate_plugins`` answers a plugin that
fails to initialise by disabling it and *persisting* ``enabled=false`` --
so the plugin stays off across restarts "even after the file was put right
again". For godcmd the destroyed keys include the admin password.
Precedent
---------
Writing through a sibling file is what the rest of the storage layer
already does: plugins/plugin_manager.py:77 (plugins.json),
agent/skills/manager.py:131 (skills_config.json),
agent/tools/scheduler/task_store.py:71, models/model_catalog.py:232,
channel/web/core/_common.py:114 (config.json).
Fix
---
Give each plugin a local ``_write_config_atomically`` that serialises into
``config.json.tmp``, fsyncs and os.replace()s it into place, and call it
from the repair path. The helper stays per-plugin on purpose: a plugin
directory is meant to be copyable on its own, which is also why the two
files each carried their own copy of the repair.
Scope
-----
Only the repair write changes. The trigger conditions, the merge with
DEFAULT_CONFIG, the ``except OSError`` handler and the "a read-only plugin
dir must not take the plugin down" behaviour are untouched.
plugins/keyword/keyword.py writes config.json only inside
``if not os.path.exists(config_path)``, so it can never truncate an
existing file and is deliberately not touched.
Testing
-------
tests/test_plugin_config_defaults.py: 2 failed, 6 passed against the
unfixed sources (JSONDecodeError on the truncated ``{"half``), 8 passed
with the fix. 60 passed across the plugin/linkai/qianfan/console suites.
No new ruff debt (9 pre-existing findings before and after).
Problem
-------
skills_config.json is the installed-skill registry: both `cow skill`
(``cli.utils.load_skills_config``) and the runtime SkillManager read it.
All five writers in cli/commands/skill.py rewrote it in place with
``open(..., "w")``, which truncates before the new bytes are there, so a
save that fails halfway -- a full disk, the process being killed, a power
cut -- leaves a half-written document behind.
Reachability
------------
cli.utils.load_skills_config() (cli/utils.py:84) cannot parse that
document and answers ``{}``, so the very next
``_register_installed_skill`` / ``_set_enabled`` / ``uninstall`` /
``_merge_builtin_into_config`` starts from an empty baseline and, when it
saves, drops every other skill's entry -- source, enabled,
display_name -- from the registry. The runtime side reads the same path:
SkillManager._load_skills_config answers None for an unreadable file and
_sync_skills_config then falls back to its own view, so the user's
``enabled: false`` choices are reset as well.
Precedent
---------
agent/skills/manager.py:131 already writes this same file through a
sibling file and documents why: "Written to a temporary file and renamed
into place, so a concurrent reader sees either the old file or the new
one, never a partial write." Its reader docstring goes further -- the
None return exists "so the caller never mistakes it for an empty config
and writes the defaults over the user's choices". The CLI writers are the
copies that picked up neither half of that convention.
Fix
---
Add one module-level ``_write_skills_config`` that serialises into
``skills_config.json.tmp``, fsyncs and os.replace()s it into place, and
route all five call sites through it.
Scope
-----
Only the write path changes: the readers, the merge semantics and the
best-effort error handling at each call site are untouched. The same file
is also written by plugins/cow_cli/cow_cli.py (the inline console); that
entry point is left for a separate change so this diff stays focused on
cli/commands/skill.py.
Testing
-------
tests/test_skills_config_atomic_write.py: 2 failed, 1 passed against the
unfixed source (JSONDecodeError on the truncated ``{\n "half``), 3
passed with the fix. Existing skill suites unchanged (see the PR body).
`EdgeVoice.textToVoice` and `ElevenLabsVoice.textToVoice` were the only two
providers under `voice/` with neither an ERROR branch nor any exception
handling -- every other module there has both (`ali`, `azure`, `baidu`,
`custom`, `dashscope`, `google`, `linkai`, `mimo`, `minimax`, `openai`,
`pytts`, `tencent`, `xunfei`, `zhipuai`).
A raise travels from `textToVoice` up through
`Bridge().fetch_text_to_voice` (bridge/bridge.py:157) and
`ChatChannel.build_text_to_voice`, and `ChatChannel._handle` has no try around
the decoration step, so it ends at `ChatChannel._fail_callback`, which only
logs (channel/chat_channel.py:453). Because the conversion happens on the way
out of an already generated text reply, the user loses **both** the voice
answer and the text answer -- a normal question gets no reply at all.
Both are documented, user-selectable engines (`config.py:148` lists
`elevenlabs` and `edge` among the `text_to_voice` values), and both failures
are ordinary: edge-tts raises `NoAudioReceived` when Microsoft's endpoint
refuses the request, and ElevenLabs raises on an expired key or an exhausted
quota.
Both entry points now match the shape `ali_voice.py` uses: log the error and
return `Reply(ReplyType.ERROR, "抱歉，语音合成失败")`. The success path is
unchanged.
`AzureVoice.textToVoice` is left alone: it does have the ERROR branch, but it
only reaches it when the SDK returns a non-success `reason`, so a raise from
the SDK call itself still escapes. That is a separate change to a provider
that at least handles the documented failure mode.
Testing: `tests/test_tts_failure_becomes_error_reply.py` pins the failure path
for both providers (failing `generate`, failing `save`, failing synthesis) plus
a success control for each. By reverting the two source hunks the four failure
cases report `RuntimeError` / `OSError` escaping `textToVoice` while the two
success cases still pass.
Deep Dream replaces both files in place: `_read_main_memory` is the only
source it distils from, and the result is the user's only long-term memory --
nothing keeps a copy of it. Writing straight into the file truncates it
first, so a failure halfway through (a full disk, the process being killed)
leaves a partial memory and the previous content is unrecoverable. The dream
diary has the same shape: a re-run of the same day replaces it wholesale.
Both writes now build the text beside the file and swap it in, which is the
pattern the rest of the repo already uses for single-copy user data
(`TaskStore._write_atomic`, `model_catalog`, `plugin_manager.save_config`).
A failed save leaves whatever was there before.
Files with the same name but created only when missing (`get_today_memory_file`
with `ensure_exists`, `create_memory_files_if_needed`, `ensure_daily_memory_file`)
are deliberately left alone: they never overwrite existing content.
xunfei_tts sends the text and then waits for the audio frames, closing only when the server reports the last one, and it started run_forever with neither a timeout nor a ping - so nothing was ever sent that could fail and a provider that accepted the connection and then stopped answering blocked the voice turn for good, with no reply and no error.
Ping every WS_PING_INTERVAL seconds and give up after WS_PING_TIMEOUT without a pong, and record what the stream ended with: run_forever hands errors to on_error and then simply returns, so without that the caller cannot tell a stream that produced audio from one that gave up. Raise when it failed, which XunfeiVoice.textToVoice already turns into an ERROR reply.
Plugin.save_config wrote both copies of a plugin's configuration straight into the destination, which truncates it before the new bytes are there, so a save that fails part-way leaves a half-written store. PluginManager._load_all_config reads plugins/config.json with a bare json.load whose failure is only logged, so one damaged write silently drops every plugin's configuration.
Stage the document in a sibling file, fsync and os.replace - the shape PluginManager.save_config already uses for plugins.json - and drop the temporary file when the write fails.
retire_legacy rewrote a live config.json with Path.write_text, which opens the destination for writing and cuts it short before the new bytes are there. A write that stops part-way (full disk, container killed) therefore leaves a partial file that load_config reads as corruption: the desktop client quarantines it and reinitialises from config-template.json, taking every API key and channel credential with it, and a source deployment raises instead of starting.
Save through a sibling file, fsync and os.replace - the shape team.write() already uses for team.json - and unlink the temporary file if the write fails.
LinkAI was the only voice provider that returned None on a failed
transcription or synthesis (voice/linkai/linkai_voice.py, 4 sites).
Every sibling (ali, openai, mimo) returns Reply(ReplyType.ERROR, ...),
so the user hears/sees a failure message. A None bubbles up through
bridge.fetch_voice_to_text with no Reply at all and the turn goes silent
(or the caller dereferences a None reply).
Replace the four `return None` with ReplyType.ERROR replies, reusing the
same Chinese strings the other providers use ("抱歉，语音识别失败" /
"抱歉，语音合成失败").
Tests: the four failure exits (ASR/TTS, HTTP error / exception) now
return an ERROR reply instead of None.
The agent bridge hands local files to channels as `file://` URLs
(bridge/agent_bridge.py::_create_file_reply). wechat_kf's send() fed
those straight into requests.get (IMAGE_URL/VIDEO_URL) or open() (FILE),
so a local-file reply raised InvalidSchema / FileNotFoundError and never
reached the user. Unwrap the prefix and read from disk; only genuine
http(s) URLs hit the network.
A FILE reply also carries the agent's summary in reply.text_content;
send it as a text bubble first, matching the sibling weixin channel's
send() (channel/weixin/weixin_channel.py).
Tests: a file:// image is read from disk (requests.get is never called),
a http(s) image still downloads, a file:// file opens locally, and a
FILE reply's text_content is delivered as a text message.
`_write_env_file` rebuilds ~/.cow/.env from an in-memory dict on every
`set`/`delete` and writes it straight into the file. `open(path, "w")`
truncates before a single byte is written, so anything that goes wrong after
that point leaves the file empty -- a value the codec cannot encode, a full
disk, the process being killed. The next read then reports "nothing is
configured" rather than an error, and every key is already gone.
Reproduced: set OPENAI_API_KEY, then set a second key whose value is a lone
surrogate (what a model emits for a broken emoji; json.loads hands it through
and utf-8 refuses to encode it). The tool reports "EnvConfig tool error", and
the file is down to its two header lines -- list() returns {}.
This file is the agent's only copy of its API keys. bridge/agent_initializer.py
and agent/tools/bash/bash.py read it to decide which providers and skills are
usable at all, so losing it takes the whole configuration with it, and there is
nothing to rebuild it from.
A dozen other stores in this tree already build their document beside the file
and `os.replace` it in; TaskStore is right next door and its `_write_atomic`
docstring describes this exact failure. Do the same here, which also makes the
layout of the file independent of how the write went.
Also restrict the file to its owner. The other credentials file in ~/.cow,
mcp_oauth.json, is written 0o600 for that reason; this one was left at the
process umask, 0o644 on a typical install. The mode is applied on every use as
well as on every save, so a file an earlier version created is narrowed too.
`voice/baidu` and `voice/custom` were the only providers calling `requests.*`
without a timeout. A stalled service leaves such a request waiting forever:
the socket stays open, nothing is raised and nothing is logged, so the turn
never finishes and the user never gets a reply.
Baidu's long-text synthesis already caps its polling loop at 100 rounds, but
the unbounded POST inside that loop means the loop can never advance past a
hang — the cap never gets a chance to fire.
Use the same (connect, read) shape as the other providers: (5, 60) for baidu,
matching openai/zhipuai, and (5, 120) for custom, whose ASR upload and TTS
synthesis are the two long endpoints mimo and linkai already allow 120s for.
`_execute_skill_call` re-wrapped every agent reply as TEXT using only
`reply.content`. When the agent ends its turn by sending files, `agent_reply`
returns a FILE / IMAGE_URL reply whose `content` is the media URL while the
prose rides in `text_content` (`bridge/agent_bridge.py::_create_file_reply`).
A scheduled skill that produced a report therefore delivered a bare
`file:///...` path: the file never reached the user and the answer was
dropped, while the log still reported "skill result sent".
Forward such a reply untouched, as the sibling `_execute_agent_task` in the
same module already does; `agent_delegate.delegated_result_text` documents the
same trap. A `result_prefix` now lands on `text_content`, and plain text
replies are still wrapped exactly as before.
Also drops the pre-existing unused `Optional` import, which the F-class lint
gate flags on any change to this file.
`Vision._download_to_data_url` used a plain `requests.get`, which follows
redirects automatically, while the SSRF guard only checked the URL the model
supplied. A public image URL that 3xx-redirects into a loopback / link-local /
cloud-metadata address was therefore fetched unchecked, and the internal body
was base64-embedded into the LLM request.
Move the redirect-aware fetcher out of `WebFetch._safe_get` into the shared
`agent.tools.utils.url_safety` module as `safe_get`, and route vision through
it. `WebFetch` keeps a thin wrapper, so its own headers and timeout are
unchanged.
WechatComAppChannel.send() branched on text, voice and images only, so a
FILE reply -- what AgentBridge returns for every document, video and audio
the agent produces (bridge/agent_bridge.py:1918, :1929) -- matched no branch
and fell off the end of the method: no upload, no message, not even a log
line. The channel declares NOT_SUPPORT_REPLYTYPE = [], so ChatChannel does
not downgrade the reply to a text notice either, and the user simply never
receives the file. Every other channel routes files (wechat_kf:249, qq:578,
weixin:822, wecom_bot:997, dingtalk:919, feishu:907, discord:479, slack:487,
telegram:704).
FILE, VIDEO and VIDEO_URL now upload the media and send it, as a video when
the reply is one. A file reply carries a file:// path or a URL, so both are
resolved to a local file first, fetching a URL into the agent's managed tmp
dir; a file that cannot be resolved is reported instead of uploaded. The
prose the agent wrote beside the attachment (reply.text_content) is sent
first, and any remaining reply type falls back to text rather than vanishing.
Every other provider under voice/ turns a failure into Reply(ReplyType.ERROR,
...) -- azure, baidu, custom, dashscope, google, mimo, minimax, tencent, xunfei
and zhipuai all wrap their calls. Aliyun was the one that did not: textToVoice
and voiceToText called straight through, so a raise travelled to
ChatChannel._fail_callback, which only logs (channel/chat_channel.py:453) and
sends the user nothing at all. Both methods already had the ERROR branch -- it
was reachable only when the call returned None, never when it raised:
fileName = text_to_speech_aliyun(self.api_url_text_to_voice, text, self.app_key, token_id)
if fileName:
reply = Reply(ReplyType.VOICE, fileName)
else:
reply = Reply(ReplyType.ERROR, "抱歉，语音合成失败") # never reached on a raise
Raising was easy to reach. get_valid_token parsed the token response with a bare
json.loads and indexed ["Token"]["Id"], so a rejected key or an exhausted quota
-- both of which answer {"Message": ..., "Code": ...} with no Token field --
raised KeyError, and a gateway error page raised JSONDecodeError:
>>> json.loads('{"Message": "InvalidAccessKeyId.NotFound", "Code": "..."}')["Token"]
KeyError: 'Token'
The response body appeared nowhere in either failure, so a bad key could not be
told apart from an outage.
get_valid_token now separates "the response was not a token" from "we could not
ask", raising a RuntimeError carrying the first 200 characters of the body, and
both entry points turn any failure into the ERROR reply they already had. The
cached-token path is untouched: a live token is still reused without another
lookup.
ali_api.py also left all three outbound calls unbounded -- requests.post into
the synthesis endpoint, requests.get for the token, and a bare
http.client.HTTPSConnection for recognition. Six of the nine files under voice/
already pass a timeout and all six point at voice/openai's
REQUEST_TIMEOUT = (5, 60), so the value follows the package rather than being
invented.
Covered by tests/test_ali_voice_failure_replies.py: a token response with no
Token field, a gateway error page, and a synthesis call that raises must each
come back as Reply(ReplyType.ERROR, ...) from both entry points, and the
working path must still return VOICE with the cached token reused. A scan over
ali_api.py asserts every outbound call carries a timeout, with a guard so it
cannot pass by matching nothing.
Deliberately not done: speech_to_text_aliyun closes its connection on the paths
it handles, but a raise out of conn.request still leaks it. Fixing that means a
try/finally around the recognition call, which is a different change from the
one here.
_write_store wrote straight into the overlay file, and _strip_legacy_config_key
straight into config.json, so anything that failed partway through -- a full
disk, an update interrupted mid-copy -- left a document that no longer parses.
The two fail in opposite directions, and both are bad.
The overlay store fails silently. _read_store swallows the decode error on
purpose:
except (OSError, ValueError) as e:
logger.warning(f"[ModelCatalog] failed to read {path}: {e}")
raw = None
...
return {"providers": providers} # no providers at all
so a truncated store reads back as "the user has no overrides" instead of
raising. Keeping one bad file from taking the whole models view down is the
right call -- but it makes the loss invisible, and save_catalog then makes it
permanent:
store = _read_store() # read fresh so a concurrent edit isn't clobbered
...
_write_store(store)
It reads the store back before rewriting it, so the empty baseline it just read
is what gets written. The user's custom providers are gone, and nothing logged
an error at the moment they were lost.
config.json fails loudly instead. _strip_legacy_config_key rewrites the user's
entire config to drop provider_model_catalog once the migration has run, and a
truncated config.json is exactly what load_config treats as corruption: on the
desktop client the file is quarantined and replaced with config-template.json,
taking every API key and channel credential with it, while a source deployment
raises and never starts.
Both now go through a single helper beside _store_path that builds the result in
a sibling .tmp file and os.replace()s it in -- the shape task_store.py:87
(_write_atomic), recipient_store.py:67 (_save_unlocked),
channel/weixin/weixin_channel.py:80 (_save_credentials) and
channel/web/api/agents.py:422 already use. A failure removes the temp file and
re-raises, leaving the previous file exactly as it was.
Covered by tests/test_model_catalog_atomic_write.py: a failed store write and a
failed config.json cleanup must both leave the previous file byte-identical and
no .tmp behind, a successful write must land intact, and save_catalog must keep
the providers already in the store -- the control for the silent-loss path.
save_config wrote straight into plugins.json, so anything that failed partway
through -- a full disk, an update interrupted mid-copy -- left a truncated
document where the plugin states used to be. load_config then read it with a
bare json.load and no guard:
with open(cfg_path, "r", encoding="utf-8") as f:
pconf = json.load(f)
pconf["plugins"] = SortedDict(...)
A truncated file raises JSONDecodeError there; a file that still parses but lost
its "plugins" key raises KeyError on the very next line. Either one escapes
load_plugins, which app.py:252 calls on first start:
if first_start:
if DESKTOP_MODE:
threading.Thread(target=PluginManager().load_plugins, daemon=True).start()
else:
PluginManager().load_plugins()
On a source deployment that is a failed start. On the desktop client the same
raise happens on a daemon thread, so it is never caught and never logged: the
plugin system simply never comes up -- /status, /help and the rest stop
answering -- with nothing in the log to say why. And the store is not obscure:
enabled and priority live only there, and six call sites write it (enable,
disable, set priority, install, uninstall, first scan), so a routine click in
the console is enough to put it at risk.
save_config now builds the store in a sibling .tmp file and os.replace()s it in
-- the shape task_store.py:87 (_write_atomic), recipient_store.py:67
(_save_unlocked), channel/weixin/weixin_channel.py:80 (_save_credentials) and
channel/web/api/agents.py:422 already use. A failure removes the temp file and
re-raises, leaving the previous store exactly as it was.
load_config keeps the fallback it already documents ("prefer the writable copy,
fall back to the one shipped in the resource dir") but stops choosing between
the two with os.path.exists. A damaged file passes that check, which is exactly
the case the fallback exists for, so the fallback was unreachable in the one
situation that needed it. Both copies now go through _read_plugin_store, which
answers None for "not there" and for "there but unusable": a bad writable copy
falls through to the shipped default, and when neither is usable the load starts
from an empty store instead of aborting. Whenever the writable copy was not the
source, a usable one is written back so the next start does not fall back again.
_read_plugin_store also rejects a store whose "plugins" is missing or not a
mapping -- valid JSON is not the same as a valid store, and that shape is what
turned into the KeyError above.
Also drop the unused conf import in this module: the lint gate only inspects
changed files, so touching this file would otherwise turn the build red.
Covered by tests/test_plugin_store_atomic_write.py: a truncated store and one
that parsed without its "plugins" mapping must both load, a damaged writable
copy must fall back to the shipped default and be repaired, a save that dies
partway must leave the previous file byte-identical and no .tmp behind, and a
healthy store must load without being rewritten.
A `requests` call with no timeout has no upper bound: if the peer accepts the
connection and then stops talking, the socket stays open, nothing is raised and
nothing is logged. The turn never completes and the user never gets a reply --
and because the process looks perfectly healthy in the meantime, the stall is
invisible until someone notices a request that has been "in progress" for ten
minutes. That is the one failure a timeout exists to prevent, and 14 calls in
this package were missing it:
models/baidu/baidu_unit_bot.py:21,33 token fetch, text generation
models/baidu/baidu_wenxin.py:94,119 text generation, token fetch
models/chatgpt/chat_gpt_bot.py:383,389,408 Azure DALL-E submit + poll
models/claudeapi/claude_api_bot.py:157,350,492,585 messages (incl. stream)
models/doubao/doubao_bot.py:111 chat/completions
models/linkai/link_ai_bot.py:494 media download
models/moonshot/moonshot_bot.py:154 chat/completions
Every sibling backend already bounds its calls -- deepseek (4 sites, 180),
doubao (4), moonshot (4), minimax (4), mimo (4), modelscope (5), qianfan (2),
gemini (3) and linkai (7). So this is not a missing convention, it is nine
backends that follow it and seven that were never brought along.
Each value follows the file it lands in rather than being invented:
doubao (120), moonshot (120) same call shape as the bounded sites already
in those files (doubao_bot.py:426,
moonshot_bot.py:476 -> timeout=120)
claude, baidu, chatgpt (180) no timeout in those files yet, so the package
default for text generation -- deepseek,
mimo and linkai all use 180 there
linkai download (5, 60) a connect/read pair like the existing
linkai_bot.py:368 (5, 10); 60s read matches
the download convention used by dingtalk and
voice/openai
The test scans the whole package with ast rather than listing today's backends,
so a backend added tomorrow, a new endpoint on an existing one, or a rewritten
call cannot quietly ship without a bound. It comes with a guard on the scan
itself (>20 calls, both verbs seen) so the check cannot pass by matching
nothing.
Three files also carried pre-existing F debt that the lint gate would surface
the moment this PR touches them: an unused `need_retry` write in
baidu_wenxin.py (assigned twice, never read -- F841), unused `json` and
`openai_error` imports in chat_gpt_bot.py, and an unused `base64` import in
claude_api_bot.py (F401).
Every console save reads config.json, changes a few keys and writes the whole
dict back -- straight into the file, with `open(config_path, "w")`. That
truncates it before the new bytes exist, and json.dump streams, so anything
that goes wrong partway through (a value json cannot encode, a full disk, the
process being killed) leaves a partial config.json where the user's settings
used to be:
>>> f = open("config.json", "w")
>>> json.dump({"web_password": "hunter2", "bad": object()}, f, indent=4)
TypeError: Object of type object is not JSON serializable
>>> open("config.json").read()
'{\n "web_password": "hunter2",\n "bad": '
Nothing downstream can recover from that. load_config treats an unparseable
user config as corruption, and config.py:511 says what happens next -- on the
desktop client the file is quarantined and replaced with config-template.json,
taking every API key, channel credential and custom provider with it, while a
source deployment raises and never starts:
# Self-heal a corrupted user config.json instead of crashing on startup --
# but ONLY for the packaged desktop client (COW_DESKTOP=1). A truncated or
# invalid file (e.g. a bad write during a previous update) would otherwise
# make json.loads raise and strand the desktop app on "Initialization
# Failed" forever [...]
Six call sites share the pattern: four in the channels view (save credentials,
connect, disconnect, and pruning a legacy channel_type) and one each in the
settings view and the models view. They now go through a single helper next to
the existing _read_config_file_for_write, which builds the result in a sibling
.tmp file and os.replace()s it in. That is the shape the rest of the tree
already uses: channel/web/api/agents.py:422, channel/weixin/weixin_channel.py:80
(_save_credentials), agent/tools/scheduler/task_store.py:87 (_write_atomic, whose
docstring spells out this exact failure) and recipient_store.py:67
(_save_unlocked). A failure now removes the temp file and re-raises, leaving the
previous config exactly as it was.
The helper takes the path instead of resolving it, so each view keeps deriving
config.json from its own get_data_root -- which is what the existing web tests
patch to keep a save inside tmp_path.
Also drop the unused parse_custom_bot_type import in models.py: the lint gate
only inspects changed files, so touching this one would otherwise turn the
build red.
Covered by tests/test_web_config_atomic_write.py: both a serialisation failure
and a simulated ENOSPC must leave the previous file byte-identical, exercised
through all three entry points, plus a positive save so the guard cannot pass by
never writing at all.
`_upload_image_url` handled the HTTP case by downloading the image into a
uuid-named file and uploading that file. Every image-URL reply takes this
path, and three things were wrong with it:
- The name was bare, so the file was created in the process CWD. That is not
where the packaged desktop build starts, and not necessarily writable there.
- `os.remove` ran inside `with open(temp_name, "rb") as file:`, i.e. while the
file was still open. On Windows that fails (WinError 32), and it fails
before the `return`, so no image key was produced and the reply was dropped
-- after the image had already been uploaded to Feishu. The file was left
behind too. Verified on Windows that the identical `os.remove` succeeds once
the handle is closed, so the open handle is the whole cause; POSIX unlink
does not care, which is why CI (Linux) stays green while Windows fails.
- `requests.get(img_url)` had no timeout and its status was never checked, so
a non-200 left no file and the `open` below raised FileNotFoundError. That
skips the caller's documented
`if not reply_content: logger.warning("upload image failed")` path.
The download now goes straight into the multipart body, which removes the
staging file, the CWD dependence and the delete-before-close at once, and both
legs are bounded at (5, 30) / (5, 15) like the newer
`feishu_static_card.upload_public_image_to_feishu`, which already uploads bytes
instead of a file. A non-200 is reported and returns None; a non-zero API
`code` now returns None as well instead of raising AttributeError on a missing
`data`.
Also dropped `import uuid`, whose last use was this temp name, and two other
pre-existing findings in the file that the CI lint gate reports once the file
is touched: the unused `ws_client` local in `stop()` and an f-string with no
placeholder at :984.
The keyword plugin turns a matched URL into a file reply by downloading it
inline, on the thread handling the message, and neither the request nor the
reply has any failure path:
- requests.get is called with no timeout, so a file host that accepts the
connection and then stalls holds the turn open forever with nothing
raised and nothing logged. The linkai siblings already bound their
requests (midjourney.py timeout=8, utils.py timeout=(5, 10)); do the same
with a (5, 60) connect/read split, which is generous for a document and
still finite.
- Nothing inspects status_code, so a 404 or a 502 from an intermediary is
written to state_dir under the URL's basename and handed to the user as
their file: an HTML error page saved as `report.xlsx`.
- A network failure raises out of requests, and that exception reaches only
chat_channel._fail_callback, which logs it -- the keyword goes quiet
instead of answering.
Both the bad status and the exception now reply ERROR, so the failure is
visible. The exception text is not echoed back: requests embeds the whole
URL in it and a keyword URL may carry a token in its query string, so the
reply names the failure kind (e.g. `ConnectTimeout`) and the URL stays in
the log line.
Covered by two tests: a 404 that must not be written to the tmp dir, and an
unreachable host that must answer rather than raise. The existing xlsx test
now models status_code on its fake response, which is what the code reads.
banwords and godcmd read their required keys straight out of whatever
Plugin.load_config() returned, and only wrote a default config when the file
was missing -- so a config.json that exists but carries nothing ({} or one key
of the two) raised KeyError in __init__:
KeyError: 'action' plugins/banwords/banwords.py:39
KeyError: 'password' plugins/godcmd/godcmd.py:194
That is worse than a failed start. PluginManager.activate_plugins answers a
plugin that fails to initialise by calling disable_plugin, which persists
`enabled: false` to plugins.json -- so the plugin is off for good, and fixing
the config file does not bring it back.
Fall back to the defaults both plugins already documented, keep whatever keys
the file does carry, and write the repaired config out. A config with only a
password keeps it and gains admin_users.
Also apply ruff --fix to godcmd's four pre-existing lint errors (unused
import, f-string with no placeholder, two unused except targets): the lint
gate only inspects changed files, so touching this one would have turned the
build red.
save_tasks() opened the store with 'w', which truncates before anything is
serialised, and json.dump streams: a task holding a value json cannot encode
raised after the old contents were already gone, and load_tasks() swallows
the decode error and reports {} -- one failed save and every scheduled task
is lost.
>>> store.save_tasks({"bad": {"id": "bad", "when": {1, 2}}})
TypeError: Object of type set is not JSON serializable
>>> store.load_tasks()
{}
Write beside the file and swap it in instead, as recipient_store in this same
package already does (and team.py, project_store.py, session_prefs.py,
mcp_oauth.py, wechat_kf_cursor_store.py, weixin_channel.py). A failure now
leaves a .tmp file to clean up rather than a truncated task list.
The backup copy had the same problem from the other side: the store is
written as UTF-8 with ensure_ascii=False and read back as UTF-8, but the copy
used the platform's default codec. On Windows that decodes the file through
the ANSI codepage -- mojibake, or, where the bytes are not valid there, a
read() that raises after the destination was opened for writing and leaves a
zero-byte .bak where a usable backup used to be. Use UTF-8 for both sides.
Also drop the unused pathlib import: the lint gate only inspects changed
files, so touching this one would have turned the build red.
Context.__init__ took `kwargs=dict()`. That literal is evaluated once, when
the function is defined, so every Context built without an explicit dict
pointed at the same object -- and the scheduler's four delivery paths, the
evolution push and the cloud client's message push all build one that way
and then write keys through `context["..."] = ...`:
>>> a = Context(ContextType.TEXT, "hello"); a["session_id"] = "conversation-A"
>>> b = Context(ContextType.TEXT, "a different conversation")
>>> b["session_id"]
'conversation-A'
A bare `Context()` after a feishu task even came with that task's
`session_id` and `receiver`, which is the pair a channel uses to decide
where a proactive push goes. bridge/agent_bridge.py and
bridge/agent_event_handler.py read context.kwargs back, so a leaked value
is treated as belonging to the current turn.
Default to None and create a fresh dict per instance. An explicit dict is
still used as given; None is accepted too, where the old code would have
left `.kwargs` as None and turned a missing key into a TypeError.
The Baidu translator is the older of the two, and its request handling has
not kept up with the Youdao sibling:
- requests.post is called with no timeout, and the call sits in a retry
loop that runs up to three times, so a connection that accepts and then
stalls blocks the caller indefinitely -- three times over. Youdao passes
timeout=10; bound each attempt the same way. The loop used to be bounded
only by attempt count.
- r.json() runs before anything inspects the response, so an HTTP-level
failure (a proxy answering 502 with an HTML body) surfaced as "Expecting
value: line 1 column 1" instead of the status and body that explain it.
Youdao covers this with raise_for_status(); report the status and the
body directly. Both paths raise either way, so only the message improves,
and retry semantics are untouched: a JSON body with a retryable
error_code still retries exactly as before.
wechatmp, wechat_kf and wechatcomapp fetch ReplyType.IMAGE_URL /
VIDEO_URL media with requests.get(..., stream=True) and no timeout, then
iterate the body -- 7 sites in total. requests with no timeout waits
forever, and these run inside the channel's send() path, so a file host
that accepts the connection and then stalls leaves the send blocked for
good: the reply never arrives and nothing is logged, because no exception
is ever raised.
The sibling channels already bound the identical downloads (slack at 60,
dingtalk at 60 and 60*5) and wechat_kf already passes timeout=10 on its
own API calls in the same file; only the media fetches were missed. Use
timeout=60 to match them.
Touching these files also brings four dead imports in front of the CI
F-class gate, which inspects changed files only, so drop them here rather
than land red: json in wechat_kf, and sys / create_reply / subscribe_msg
in wechatcomapp (the latter two only ever appeared on commented-out
lines).
FileCache.add() only set `timestamp` when a session's entry was created, so
the 5-minute window was measured from the first file of a batch instead of
the most recent one.
The cache exists so that files sent on their own are attached to the user's
next text message (add() per inbound file, get() on the following TEXT
message, then clear()). A user sending several files in a row is building
one batch, so the window has to run from the last file. Counting from the
first meant a burst spanning more than the TTL expired wholesale before the
question arrived, silently dropping files that had landed seconds earlier.
Refresh the timestamp on every add, including a re-add of an already-cached
file -- that is still activity on the batch. The TTL itself is unchanged:
get() and cleanup_expired() still expire a session once nothing has been
added within the window.
c020627
