fix: skip empty quoted image placeholders

[VectorPeak]

Closes #9176.

This PR prevents expired or unavailable quoted image placeholders from aborting the agent request with No valid file or URL provided.

Modifications / 改动点

What Problem This Solves

When a user quotes an expired or unavailable image message, the adapter can still provide a Reply component whose embedded chain contains an Image placeholder, but that placeholder may have no usable file or url value.

build_main_agent() previously treated every embedded quoted Image as usable and called convert_to_file_path() immediately. For an empty placeholder, Image.convert_to_file_path() raises ValueError("No valid file or URL provided"), so the whole agent request can abort before the quoted-message fallback has a chance to fetch by reply id.

Changes

• skip quoted Image placeholders whose file and url values are empty or whitespace-only before calling convert_to_file_path()
• keep has_embedded_image false for skipped placeholders so the existing reply-id fallback extraction path can still run
• include the reply id and quoted-chain index in the warning log so skipped placeholders are easier to trace
• add regression coverage for empty file, empty url, both-empty, and whitespace-only quoted image placeholder shapes

This intentionally does not change Image.convert_to_file_path() itself. Valid quoted images still use the existing convert/compress/attachment path, and quoted audio/file/video handling is unchanged.

• This is NOT a breaking change. / 这不是一个破坏性变更。

Evidence

The regression test simulates the reachable issue shape from #9176:

Reply(
    id="reply-1",
    chain=[Plain(text="quoted text"), Image(**image_kwargs)],
)

It now covers these invalid placeholder variants:

{"file": ""}
{"file": None, "url": ""}
{"file": "", "url": ""}
{"file": "   ", "url": None}
{"file": None, "url": "   "}

Before this change, those placeholders could reach convert_to_file_path() and raise ValueError("No valid file or URL provided").

After this change, the empty placeholder is skipped, has_embedded_image remains False, and the existing fallback extractor can still provide the quoted image reference:

assert result.provider_request.image_urls == ["fallback-quoted-image-ref"]
mock_extract_quoted_images.assert_awaited_once()

Possible call chain / impact

user quotes an expired/unavailable image message
  -> platform adapter provides Reply(chain=[Image(file="", url="")])
  -> build_main_agent() quoted message attachment loop
  -> previous behavior: convert_to_file_path()
  -> ValueError("No valid file or URL provided")
  -> agent request aborts before fallback extraction can run

With this PR:

user quotes an expired/unavailable image message
  -> empty quoted Image placeholder is skipped
  -> has_embedded_image stays False
  -> extract_quoted_message_images(...) fallback can run
  -> valid fallback image refs are appended to provider_request.image_urls

Scope notes:

• affects only empty quoted Image placeholders in the main-agent quoted attachment loop
• does not change normal user image attachments
• does not change valid quoted image conversion/compression
• does not change quoted audio, file, or video handling
• does not change quoted-image fallback limits, dedupe, parser settings, or Image.convert_to_file_path()'s direct conversion contract

Screenshots or Test Results / 运行截图或测试结果

uv run ruff check astrbot/core/astr_main_agent.py tests/unit/test_astr_main_agent.py
All checks passed!

uv run ruff format --check astrbot/core/astr_main_agent.py tests/unit/test_astr_main_agent.py
2 files already formatted

git diff --check
# passed

uv run pytest tests/unit/test_astr_main_agent.py::TestBuildMainAgent::test_build_main_agent_skips_empty_quoted_image_and_uses_fallback tests/unit/test_astr_main_agent.py::TestBuildMainAgent::test_build_main_agent_skips_caption_when_main_provider_supports_images tests/unit/test_astr_main_agent.py::TestBuildMainAgent::test_build_main_agent_does_not_caption_quoted_image_twice tests/unit/test_astr_main_agent.py::TestBuildMainAgent::test_build_main_agent_does_not_retry_quoted_image_caption_when_empty -q
........                                                                 [100%]
8 passed, 1 warning in 3.21s

Note: uv run pytest tests/unit/test_astr_main_agent.py -q currently has two pre-existing Windows-only path separator expectation failures in video attachment tests (/path/to/... expected, \\path\\to\\... observed). The focused quoted-image tests above pass and cover this PR's behavior change.

---

Checklist / 检查清单

• 😊 If there are new features added in the PR, I have discussed it with the authors through issues/emails, etc.
  / 如果 PR 中有新加入的功能，已经通过 Issue / 邮件等方式和作者讨论过。

• 👀 My changes have been well-tested, and "Verification Steps" and "Screenshots" have been provided above.
  / 我的更改经过了良好的测试，并已在上方提供了“验证步骤”和“运行截图”。

• 🧸 I have ensured that no new dependencies are introduced, OR if new dependencies are introduced, they have been added to the appropriate locations in requirements.txt and pyproject.toml.
  / 我确保没有引入新依赖库，或者引入了新依赖库的同时将其添加到 requirements.txt 和 pyproject.toml 文件相应位置。

• 😷 My changes do not introduce malicious code.
  / 我的更改没有引入恶意代码。

[sourcery-ai]

Hey - I've found 1 issue, and left some high level feedback:

• In test_build_main_agent_skips_empty_quoted_image_and_uses_fallback, consider avoiding the hard-coded "/tmp/fallback-quoted.jpg" value and using a temporary path or a more generic sentinel value so the test is less tied to a specific filesystem layout.
• For the new warning log when skipping an empty quoted image, you might want to include more context (e.g., the index in the chain or some representation of reply_comp) to make it easier to trace which placeholder was skipped during debugging.

Prompt for AI Agents

Please address the comments from this code review:

## Overall Comments
- In `test_build_main_agent_skips_empty_quoted_image_and_uses_fallback`, consider avoiding the hard-coded `"/tmp/fallback-quoted.jpg"` value and using a temporary path or a more generic sentinel value so the test is less tied to a specific filesystem layout.
- For the new warning log when skipping an empty quoted image, you might want to include more context (e.g., the index in the chain or some representation of `reply_comp`) to make it easier to trace which placeholder was skipped during debugging.

## Individual Comments

### Comment 1
<location path="tests/unit/test_astr_main_agent.py" line_range="1590-1603" />
<code_context>
+    ):
+        """Quoted image placeholders without a file or URL should not abort the request."""
+        module = ama
+        mock_reply = Reply(
+            id="reply-1",
+            chain=[Plain(text="quoted text"), Image(file="")],
+            sender_nickname="",
+            message_str="quoted text",
+        )
+        mock_event.message_obj.message = [Plain(text="Hello"), mock_reply]
+
+        mock_context.get_provider_by_id.return_value = None
</code_context>
<issue_to_address>
**suggestion (testing):** Cover the case where quoted Image has an empty url instead of an empty file

The test currently covers `Image(file="")` (with `url=None`), but the production logic skips quoted images whenever both `reply_comp.url` and `reply_comp.file` are falsy. That also includes shapes like `Image(url="", file=None)` or `Image(url="", file="")`. Since the adapter may emit `Image(file="", url="")`, please add a second case (or parameterize this test) using an empty `url` to verify the skip behavior for both possible image shapes.

```suggestion
    @pytest.mark.asyncio
    @pytest.mark.parametrize(
        "image_kwargs",
        [
            {"file": ""},
            {"url": ""},
        ],
    )
    async def test_build_main_agent_skips_empty_quoted_image_and_uses_fallback(
        self, mock_event, mock_context, mock_provider, image_kwargs
    ):
        """Quoted image placeholders without a file or URL should not abort the request."""
        module = ama
        mock_reply = Reply(
            id="reply-1",
            chain=[Plain(text="quoted text"), Image(**image_kwargs)],
            sender_nickname="",
            message_str="quoted text",
        )
        mock_event.message_obj.message = [Plain(text="Hello"), mock_reply]

```
</issue_to_address>

---

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

• X
• Mastodon
• LinkedIn
• Facebook

_{Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.}

[gemini-code-assist]

Code Review

This pull request introduces a check to skip quoted images that lack a file or URL, logging a warning and continuing, along with a corresponding unit test to verify this behavior. The reviewer suggested improving the robustness of this check by stripping whitespace from the url and file fields to prevent potential downstream failures.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

[gemini-code-assist]

The current check not (reply_comp.url or reply_comp.file) is a great addition to prevent crashes on empty placeholders. However, it can be bypassed if the url or file fields contain whitespace-only strings (e.g., " "), which would still cause convert_to_file_path() to fail later.

Using .strip() with a fallback to an empty string ensures robust defensive handling against any whitespace-only values.

Additionally, when implementing similar functionality for different cases (such as direct vs. quoted attachments), we should refactor the logic into a shared helper function to avoid code duplication. Please also ensure that this new attachment handling functionality is accompanied by corresponding unit tests.

if not ((reply_comp.url or "").strip() or (reply_comp.file or "").strip()):
    logger.warning(
        "Skip quoted image without file or URL for umo=%s, reply_id=%s",
        event.unified_msg_origin,
        getattr(comp, "id", None),
    )
    continue

References

1. When implementing similar functionality for different cases (e.g., direct vs. quoted attachments), refactor the logic into a shared helper function to avoid code duplication.
2. New functionality, such as handling attachments, should be accompanied by corresponding unit tests.

[VectorPeak]

Addressed in dd86be3. The quoted-image guard now strips url/file before deciding whether the placeholder is usable, so whitespace-only values are skipped before convert_to_file_path(). The warning log also includes the quoted chain_index for easier tracing. I kept this scoped to the quoted-message placeholder path rather than adding a shared helper because this PR does not change direct attachment handling.

[VectorPeak]

Reviewer feedback addressed in dd86be3.\n\nChanges made:\n- Parameterized the empty quoted-image regression test across empty file, empty url, both-empty, and whitespace-only placeholder variants.\n- Replaced the /tmp-specific test fallback value with a generic sentinel string.\n- Hardened the quoted-image guard by stripping url/file values before deciding whether to skip.\n- Added quoted chain_index to the skip warning log for easier tracing.\n\nValidation:\n- uv run ruff check astrbot/core/astr_main_agent.py tests/unit/test_astr_main_agent.py - passed\n- uv run ruff format --check astrbot/core/astr_main_agent.py tests/unit/test_astr_main_agent.py - passed\n- git diff --check - passed\n- focused quoted-image pytest command - passed, 8 tests\n- GitHub CI after the update - all checks passed