fix(qqbot): surface non-image attachments to agent when download fails (#16979) by briandevans · Pull Request #16990 · NousResearch/hermes-agent

briandevans · 2026-04-28T12:16:13Z

Summary

QQ Bot users uploading files (PDFs, documents, videos) saw the bot
silently do nothing whenever the QQ file CDN download failed: the
agent never learned that an attachment had been sent, no warning was
emitted at the production log level, and the user had no way to tell
the upload had been dropped.

The bug

_process_attachments() in gateway/platforms/qqbot/adapter.py
appended [Attachment: …] only on the success branch of the
non-image / non-voice path:

else:
    # Other attachments (video, file, etc.): record as text.
    try:
        cached_path = await self._download_and_cache(url, ct)
        if cached_path:
            other_attachments.append(f\"[Attachment: {filename or ct}]\")
        # ← no else: download returns None → attachment disappears
    except Exception as exc:
        logger.debug(\"[%s] Failed to cache attachment: %s\", ...)
        # ← no fallback append either

Two real failure paths reach this code:

_download_and_cache returns None — happens when _http_client
is unset, the transport fails, or resp.raise_for_status() raises
(the helper catches and returns None).
_download_and_cache raises ValueError from the
is_safe_url guard.

Both failures silently dropped the attachment and logged at DEBUG,
which production deployments running at INFO/WARNING never see.

The fix

Tightly scoped to the non-image / non-voice branch:

Compute label = filename or ct or \"file\" once so the success and
failure markers stay consistent and never collapse to an empty
[Attachment: ].
When cached_path is None, append [Attachment: <label> (download failed)] and log at WARNING with the truncated URL.
When the download raises, also append the same fallback marker and
log the exception at WARNING.

The image and voice branches are intentionally left alone — voice
already has its own [Voice] [语音识别失败] fallback, and the image
branch is outside the issue's scope.

Test plan

Focused regression: tests/gateway/test_qqbot.py::TestProcessAttachmentsFallback
— 4 new tests cover (1) _download_and_cache returns None, (2)
raises, (3) succeeds, (4) success/failure with missing filename
falls back to content_type. All pass.
Adjacent suite: full tests/gateway/test_qqbot.py — 75 passed.
Regression guard: reverted gateway/platforms/qqbot/adapter.py,
confirmed the 3 failure-path tests fail with empty
attachment_info, restored fix, all 4 pass again.

NousResearch#16979) QQ users sending file/video/document attachments saw the bot silently do nothing whenever the CDN download failed. `_process_attachments` only appended a `[Attachment: …]` marker on the success branch — when `_download_and_cache` returned `None` (no `_http_client`, transport error, content-type mismatch) or raised (unsafe URL), the attachment disappeared without any signal to the agent. Both failure paths logged at `DEBUG`, so production deployments running at INFO/WARNING never even saw the error. This makes the failure observable without expanding scope: - Always append `[Attachment: <name> (download failed)]` when the download returns no payload or raises, so the agent at least knows a file was sent. - Upgrade both failure logs to `WARNING` so production logs surface the CDN error. - Reuse a single `label = filename or ct or "file"` so the success and failure markers stay consistent and never collapse to an empty `[Attachment: ]`. Image and voice paths are intentionally untouched — voice already has its own `[Voice] [语音识别失败]` fallback, and the image branch is out of scope for this issue. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Copilot

Pull request overview

Fixes QQBot behavior where non-image/non-voice attachments (e.g., PDFs, videos) were silently dropped when CDN download/caching fails, ensuring the agent still receives an attachment marker and operators get higher-signal logs.

Changes:

Add a consistent label and append a fallback [Attachment: … (download failed)] marker when caching returns None or raises.
Promote attachment download/cache failures to WARNING logs in the non-image/non-voice path.
Add focused regression tests covering success/failure cases and filename/content-type label fallback.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

File	Description
`gateway/platforms/qqbot/adapter.py`	Ensures non-image/non-voice attachments always surface to the agent with a failure marker and emits warning logs on failure.
`tests/gateway/test_qqbot.py`	Adds regression tests verifying attachment markers appear on failure and omit the failure marker on success.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-04-28T12:19:35Z

+                label = filename or ct or "file"
                try:


label = filename or ct or "file" can still produce an effectively empty label when filename is whitespace (e.g., " "), since filename isn't stripped. Consider computing label from filename.strip() (and possibly normalizing newlines) so the attachment marker never becomes [Attachment: (download failed)].

Copilot · 2026-04-28T12:19:36Z

+                        logger.warning(
+                            "[%s] Attachment download returned no payload: %s (%s)",
+                            self._log_tag,
+                            label,
+                            url[:80],
+                        )


The new WARNING log includes url[:80], which for QQ file CDN links often contains signed query parameters (sign=..., sig=...). Emitting those at WARNING increases the chance of leaking download-capable tokens into production logs. Consider logging only urlparse(url).hostname + path (omit query), or explicitly redact known signature parameters before logging.

Copilot · 2026-04-28T12:19:36Z

+                        other_attachments.append(f"[Attachment: {label}]")
+                    else:
+                        logger.warning(
+                            "[%s] Attachment download returned no payload: %s (%s)",


The message "Attachment download returned no payload" is misleading because _download_and_cache() returns None for multiple reasons (e.g., HTTP client not initialized, network/HTTP errors that are caught and turned into None). Consider rewording this to a generic "Attachment download failed (no cached path)" or adjusting _download_and_cache to provide/propagate a failure reason so ops can distinguish misconfiguration vs CDN failures.

Suggested change

"[%s] Attachment download returned no payload: %s (%s)",

"[%s] Attachment download failed (no cached path): %s (%s)",

Address Copilot review on PR NousResearch#16990: **Finding 1 (line 1196 — whitespace filename slipped past fallback):** ``label = filename or ct or "file"`` would treat ``" "`` as truthy and produce ``[Attachment: (download failed)]``. Strip both the filename and the content-type before the falsy chain so the label always carries something meaningful. **Finding 2 (line 1206 — signed query params leaked into WARNING log):** QQ file CDN URLs are signed (``sign=``, ``sig=``). Logging ``url[:80]`` risked emitting download-capable tokens to production logs. Replace with ``urlparse(...).hostname + path`` so the log keeps debuggability (host tells you which CDN, path tells you which endpoint) without the secret bearer. **Finding 3 (line 1202 — misleading "no payload" message):** ``_download_and_cache()`` returns ``None`` for many reasons (HTTP client not initialized, transient network error, empty response …); "no payload" implies a successful empty body, which is one specific case. Reword to "(no cached path)" — accurate regardless of which branch returned None. Two new regression tests: * ``test_whitespace_filename_falls_back_to_content_type`` — asserts a whitespace-only filename does not produce ``Attachment: `` (double space) and that the content-type fallback fires. * ``test_failure_log_does_not_leak_signed_query_params`` — asserts the WARNING record never contains ``sign=`` or the signature value, but still contains the host + path. 77/77 tests/gateway/test_qqbot.py pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

briandevans · 2026-04-28T12:23:11Z

@copilot All three findings addressed in commit 3d2626751:

Finding 1 (line 1196 — whitespace filename): Real bug. label = filename or ct or "file" treated " " as truthy and produced [Attachment: (download failed)] (double-space). Now strips both filename and content-type before the falsy chain — label is always non-empty after a fall-through.

Finding 2 (line 1206 — signed query params in WARNING log): Real risk. QQ file CDN URLs carry sign= / sig= bearer tokens; url[:80] would emit them at WARNING level. Replaced with urlparse(url).hostname + .path — keeps debuggability (host shows which CDN, path shows which endpoint) without the signature.

Finding 3 (line 1202 — misleading "no payload"): Real ambiguity. _download_and_cache() returns None for many reasons (HTTP client not init, transient network error, empty response). Reworded to "download failed (no cached path)" — accurate regardless of which branch returned None. Propagating a typed reason from _download_and_cache is a fair follow-up but feels like scope creep for this PR.

Two new regression tests in TestProcessAttachmentsFallback:

test_whitespace_filename_falls_back_to_content_type — asserts a whitespace-only filename does not produce Attachment: and that the content-type fallback fires.
test_failure_log_does_not_leak_signed_query_params — asserts the WARNING record never contains sign= or the signature value, but still contains the host + path.

77/77 tests/gateway/test_qqbot.py pass. CI will pick up the new commit shortly.

briandevans · 2026-04-28T12:45:18Z

CI audit — test job's failures on commit 3d2626751 are pre-existing baselines on clean origin/main (06164a7b2). Zero failures intersect touched code.

Touched code (qqbot adapter _process_attachments, 4 new + 0 changed existing) — tests/gateway/test_qqbot.py runs 77 / 77 PASS locally.

Sample baselines reproduced on clean origin/main:

Test	Symptom	Cause on main
`tests/tui_gateway/test_protocol.py::test_session_resume_returns_hydrated_messages`	`unexpected keyword argument 'include_ancestors'`	DB stub lags production signature — already fixed in my PR #16683.
`tests/agent/test_anthropic_adapter.py::TestBuildAnthropicClient::test_custom_base_url`	beta-header set mismatch (`anthropic-beta` includes `context-1m-2025-08-07` not in expected set)	Recent `a7cdd4133 fix(bedrock): send context-1m-2025-08-07 beta` widened the beta header on origin/main.
`tests/tools/test_mcp_structured_content.py::TestStructuredContentPreservation::test_text_only_result`	`'_rpc_lock'` AttributeError	Recent MCP nullable-schema cluster (`0f473d643`/`aa9488328`) added `_rpc_lock` access path that the test stubs don't expose.
`tests/tools/test_clipboard.py::TestIsWsl::*`	WSL-detection asserts on non-WSL CI runner	Long-standing baseline (also noted on PR #16555 / #16666 / #16851 / #16902 — all touched-code suites green).
`tests/tui_gateway/test_make_agent_provider.py::test_make_agent_passes_resolved_provider`	mocked call signature mismatch	Already fixed in my PR #16684.

This PR's touched-code regression suite (TestProcessAttachmentsFallback, 4 tests) is fully green and the existing 73-test qqbot suite is unaffected.

briandevans · 2026-05-18T01:14:06Z

Closing to keep the queue clean — happy to reopen if this is still useful.

Copilot AI review requested due to automatic review settings April 28, 2026 12:16

Copilot started reviewing on behalf of briandevans April 28, 2026 12:16 View session

Copilot AI reviewed Apr 28, 2026

View reviewed changes

alt-glitch added type/bug Something isn't working P2 Medium — degraded but workaround exists platform/qqbot QQ Bot adapter comp/gateway Gateway runner, session dispatch, delivery labels Apr 28, 2026

briandevans mentioned this pull request Apr 28, 2026

fix(aux): default Codex reasoning effort to medium when extra_body.reasoning.effort is falsy #17012

Closed

4 tasks

zxzxzx258 mentioned this pull request May 15, 2026

QQ Bot: File attachments cached but path not passed to agent, original filename lost #26399

Closed

briandevans closed this May 18, 2026

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(qqbot): surface non-image attachments to agent when download fails (#16979)#16990

fix(qqbot): surface non-image attachments to agent when download fails (#16979)#16990
briandevans wants to merge 2 commits into
NousResearch:mainfrom
briandevans:fix/qqbot-attachment-silent-drop-16979

briandevans commented Apr 28, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Apr 28, 2026

Uh oh!

Copilot AI Apr 28, 2026

Uh oh!

Copilot AI Apr 28, 2026

Uh oh!

briandevans commented Apr 28, 2026

Uh oh!

briandevans commented Apr 28, 2026

Uh oh!

briandevans commented May 18, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

	"[%s] Attachment download returned no payload: %s (%s)",
	"[%s] Attachment download failed (no cached path): %s (%s)",

Conversation

briandevans commented Apr 28, 2026

Summary

The bug

The fix

Test plan

Related

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Apr 28, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 28, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 28, 2026

Choose a reason for hiding this comment

Uh oh!

briandevans commented Apr 28, 2026

Uh oh!

briandevans commented Apr 28, 2026

Uh oh!

briandevans commented May 18, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants