Skip to content

fix: cannot resolve voice when using aiocqhttp adapter#8523

Merged
Soulter merged 1 commit into
AstrBotDevs:masterfrom
tjc6666666666666:patch-5
Jun 3, 2026
Merged

fix: cannot resolve voice when using aiocqhttp adapter#8523
Soulter merged 1 commit into
AstrBotDevs:masterfrom
tjc6666666666666:patch-5

Conversation

@tjc6666666666666
Copy link
Copy Markdown
Contributor

@tjc6666666666666 tjc6666666666666 commented Jun 2, 2026

Refactor file handling to resolve sources more effectively, including decoding file URIs and handling base64 data. Update methods to ensure compatibility with different file formats and paths.

Modifications / 改动点

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

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


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.txtpyproject.toml 文件相应位置。

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

Summary by Sourcery

Improve audio record file handling by unifying source resolution across file, URL, path, and base64 inputs.

New Features:

  • Add helper methods to decode file:/// URIs and resolve the most appropriate file source from multiple possible locations.

Enhancements:

  • Update record-to-file and record-to-base64 conversion to rely on centralized file source resolution and proper file URI decoding.
  • Adjust temporary file generation for decoded base64 audio data to use a .wav extension.

Refactor file handling to resolve sources more effectively, including decoding file URIs and handling base64 data. Update methods to ensure compatibility with different file formats and paths.
@dosubot dosubot Bot added size:M This PR changes 30-99 lines, ignoring generated files. area:core The bug / feature is about astrbot's core, backend labels Jun 2, 2026
Copy link
Copy Markdown
Contributor

@sourcery-ai sourcery-ai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey - I've found 2 issues, and left some high level feedback:

  • In _resolve_file_source, the self.url branch has redundant and partially unreachable logic (self.url.startswith("file:///") is checked twice and the and os.path.exists(self._decode_file_uri(self.url)) clause will never be hit), so consider restructuring this block to clearly handle file:/// (decode + exists), http, and direct filesystem paths separately.
  • _resolve_file_source is declared async but does not perform any await or I/O, which adds unnecessary complexity to call sites; consider making it a synchronous helper (or adding a comment if you anticipate future async operations there).
  • Both convert_to_file_path and convert_to_base64 now rely on file_source but still report errors using self.file (e.g., raise Exception(f"not a valid file: {self.file}")), which can be misleading when the URL/path is actually coming from url or path; it may be clearer to include the resolved source or all candidate fields in the error message.
Prompt for AI Agents
Please address the comments from this code review:

## Overall Comments
- In `_resolve_file_source`, the `self.url` branch has redundant and partially unreachable logic (`self.url.startswith("file:///")` is checked twice and the `and os.path.exists(self._decode_file_uri(self.url))` clause will never be hit), so consider restructuring this block to clearly handle `file:///` (decode + exists), `http`, and direct filesystem paths separately.
- `_resolve_file_source` is declared `async` but does not perform any `await` or I/O, which adds unnecessary complexity to call sites; consider making it a synchronous helper (or adding a comment if you anticipate future async operations there).
- Both `convert_to_file_path` and `convert_to_base64` now rely on `file_source` but still report errors using `self.file` (e.g., `raise Exception(f"not a valid file: {self.file}")`), which can be misleading when the URL/path is actually coming from `url` or `path`; it may be clearer to include the resolved source or all candidate fields in the error message.

## Individual Comments

### Comment 1
<location path="astrbot/core/message/components.py" line_range="174-179" />
<code_context>
+                return self.file
+
+        # 2) 尝试 url(可能是 file:/// 或 http 链接)
+        if self.url:
+            if (
+                self.url.startswith("file:///")
+                or self.url.startswith("http")
+                or os.path.exists(self.url)
+                or (
+                    self.url.startswith("file:///")
+                    and os.path.exists(self._decode_file_uri(self.url))
</code_context>
<issue_to_address>
**issue (bug_risk):** The `os.path.exists(self._decode_file_uri(self.url))` branch is effectively unreachable due to the preceding `self.url.startswith("file:///")` condition.

Because the `self.url.startswith("file:///")` check appears earlier in the `or` chain, Python short-circuits when it’s true and never evaluates the `and os.path.exists(self._decode_file_uri(self.url))` part.

If you want to only accept `file:///` URLs when the decoded local path exists, consider restructuring as:

```python
if self.url.startswith("file:///"):
    decoded = self._decode_file_uri(self.url)
    if os.path.exists(decoded):
        return self.url  # or decoded
elif self.url.startswith("http") or os.path.exists(self.url):
    return self.url
```

This ensures the existence check is actually applied and removes dead logic.
</issue_to_address>

### Comment 2
<location path="astrbot/core/message/components.py" line_range="156" />
<code_context>
+        path = urllib.parse.unquote(path)
+        return path
+
+    async def _resolve_file_source(self) -> str:
+        """选择可用的文件源。
+
</code_context>
<issue_to_address>
**issue (complexity):** Consider simplifying the new file resolution logic by making `_resolve_file_source` synchronous, extracting reusable helpers for source validation/classification, and removing unreachable branches so both conversion methods share the same streamlined flow.

The new `_resolve_file_source` / `_decode_file_uri` layer does centralize logic, but there are a few concrete spots where you can reduce complexity and fix a subtle bug without changing behavior.

### 1. `async` helper without `await`

`_resolve_file_source` is `async` but never awaits, which forces `convert_to_file_path` / `convert_to_base64` into async even for pure in‑memory decisions. You can make it synchronous and keep the public API unchanged:

```python
@staticmethod
def _decode_file_uri(uri: str) -> str:
    path = uri.removeprefix("file:///")
    return urllib.parse.unquote(path)

def _resolve_file_source(self) -> str:
    # 1) Prefer file if it is already usable
    if self.file and self._is_usable_source(self.file):
        return self.file

    # 2) Fallback to url
    if self.url and self._is_usable_source(self.url):
        return self.url

    # 3) Fallback to path
    if self.path and os.path.exists(self.path):
        return self.path

    # 4) Last resort: raw value for error messages
    return self.file or self.url or ""
```

Then call it synchronously:

```python
async def convert_to_file_path(self) -> str:
    file_source = self._resolve_file_source()
    ...
```

### 2. Redundant / unreachable condition

In the current `_resolve_file_source`:

```python
if self.url:
    if (
        self.url.startswith("file:///")
        or self.url.startswith("http")
        or os.path.exists(self.url)
        or (
            self.url.startswith("file:///")
            and os.path.exists(self._decode_file_uri(self.url))
        )
    ):
        return self.url
```

The last branch `self.url.startswith("file:///") and ...` is never reached if `url.startswith("file:///")` is true, because the first `self.url.startswith("file:///")` already short‑circuits the `or`. This can be simplified and fixed by handling the decoded `file:///` case explicitly:

```python
def _is_usable_source(self, value: str) -> bool:
    if value.startswith("file:///"):
        decoded = self._decode_file_uri(value)
        return os.path.exists(decoded)
    if value.startswith(("http", "base64://")):
        return True
    return os.path.exists(value)
```

Then `_resolve_file_source` just delegates to `_is_usable_source` (as in the previous snippet), avoiding nested conditionals and unreachable code.

### 3. Reduce duplication between `convert_to_file_path` and `convert_to_base64`

Both methods still repeat the same “what kind of source is this?” branching. You can unify that into a small helper that returns a normalized “kind + value”, while keeping the async download intact:

```python
def _classify_source(self, source: str) -> tuple[str, str]:
    if source.startswith("file:///"):
        return "path", self._decode_file_uri(source)
    if source.startswith("http"):
        return "http", source
    if source.startswith("base64://"):
        return "base64", source.removeprefix("base64://")
    return "path", source  # bare filesystem path
```

Then both conversions become simpler and consistent:

```python
async def convert_to_file_path(self) -> str:
    source = self._resolve_file_source()
    if not source:
        raise Exception(f"not a valid file: {self.file}")

    kind, value = self._classify_source(source)

    if kind == "http":
        file_path = await download_image_by_url(value)
        return os.path.abspath(file_path)
    if kind == "base64":
        image_bytes = base64.b64decode(value)
        file_path = os.path.join(
            get_astrbot_temp_path(), f"recordseg_{uuid.uuid4()}.wav"
        )
        with open(file_path, "wb") as f:
            f.write(image_bytes)
        return os.path.abspath(file_path)

    # kind == "path"
    if os.path.exists(value):
        return os.path.abspath(value)
    raise Exception(f"not a valid file: {self.file}")
```

```python
async def convert_to_base64(self) -> str:
    source = self._resolve_file_source()
    if not source:
        raise Exception(f"not a valid file: {self.file}")

    kind, value = self._classify_source(source)

    if kind == "http":
        file_path = await download_image_by_url(value)
        bs64_data = file_to_base64(file_path)
    elif kind == "base64":
        bs64_data = value
    else:  # kind == "path"
        if not os.path.exists(value):
            raise Exception(f"not a valid file: {self.file}")
        bs64_data = file_to_base64(value)

    return bs64_data.removeprefix("base64://")
```

This keeps all existing behaviors (file/url/path precedence, `file:///` decoding, HTTP download, base64 handling) while:

- making `_resolve_file_source` single‑responsibility and synchronous,
- removing the unreachable `file:///` branch,
- and centralizing the “what kind of source is this?” logic so both call sites stay in sync.
</issue_to_address>

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

Comment on lines +174 to +179
if self.url:
if (
self.url.startswith("file:///")
or self.url.startswith("http")
or os.path.exists(self.url)
or (
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

issue (bug_risk): The os.path.exists(self._decode_file_uri(self.url)) branch is effectively unreachable due to the preceding self.url.startswith("file:///") condition.

Because the self.url.startswith("file:///") check appears earlier in the or chain, Python short-circuits when it’s true and never evaluates the and os.path.exists(self._decode_file_uri(self.url)) part.

If you want to only accept file:/// URLs when the decoded local path exists, consider restructuring as:

if self.url.startswith("file:///"):
    decoded = self._decode_file_uri(self.url)
    if os.path.exists(decoded):
        return self.url  # or decoded
elif self.url.startswith("http") or os.path.exists(self.url):
    return self.url

This ensures the existence check is actually applied and removes dead logic.

Comment thread astrbot/core/message/components.py
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request improves file path resolution and decoding for audio records in astrbot/core/message/components.py by introducing _decode_file_uri and _resolve_file_source helpers. However, several improvements are recommended: first, avoid using download_image_by_url for audio files as it incorrectly saves them with a .jpg extension; second, simplify a redundant condition in the _resolve_file_source method; and third, refactor convert_to_base64 to reuse convert_to_file_path to eliminate duplicated logic.

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.

Comment thread astrbot/core/message/components.py
Comment thread astrbot/core/message/components.py
Comment thread astrbot/core/message/components.py
@Soulter Soulter changed the title Enhance file source resolution and decoding methods fix: cannot resolve voice when using aiocqhttp adapter Jun 3, 2026
Copy link
Copy Markdown
Member

@Dt8333 Dt8333 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

我是无情的赞同机器

Comment thread astrbot/core/message/components.py
Comment thread astrbot/core/message/components.py
Comment thread astrbot/core/message/components.py
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:core The bug / feature is about astrbot's core, backend lgtm This PR has been approved by a maintainer size:M This PR changes 30-99 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants