Skip to content

fix: verify audio magic bytes before extension-based short-circuit in convert_audio_format - #9651

Open
xiaoyuyu6420 wants to merge 4 commits into
AstrBotDevs:masterfrom
xiaoyuyu6420:fix/9594-amr-audio-magic-bytes
Open

fix: verify audio magic bytes before extension-based short-circuit in convert_audio_format#9651
xiaoyuyu6420 wants to merge 4 commits into
AstrBotDevs:masterfrom
xiaoyuyu6420:fix/9594-amr-audio-magic-bytes

Conversation

@xiaoyuyu6420

@xiaoyuyu6420 xiaoyuyu6420 commented Aug 12, 2026

Copy link
Copy Markdown

Problem

When a platform adapter (e.g. NapCat via aiocqhttp / OneBot v11) saves QQ voice messages, the actual audio encoding is AMR (magic bytes #!AMR) but the local file is given a .wav extension. The convert_audio_format() function short-circuits purely on file extension:

if audio_path.lower().endswith(f".{output_format}"):
    return audio_path   # ← skips ffmpeg, returns raw AMR bytes

When output_format is wav, the extension matches and the raw AMR byte stream is returned unchanged. Downstream STT providers (e.g. Whisper-compatible APIs) receive malformed WAV data and reject it with HTTP 400.

Reported in #9594.

Fix

Reuse the existing _get_audio_magic_type() helper (already used by ensure_wav()) to verify the file content actually matches the target format before short-circuiting:

if audio_path.lower().endswith(f".{output_format}"):
    detected = _get_audio_magic_type(audio_path)
    if not detected or detected == output_format:
        return audio_path
    # Extension/content mismatch → proceed with ffmpeg conversion

Three cases:

  • Extension matches AND magic bytes confirm the format → short-circuit (preserves the optimization).
  • Extension matches BUT magic bytes detect a different format → proceed with ffmpeg conversion (fixes the bug).
  • Extension matches BUT format is unrecognised → short-circuit (preserves old behaviour for formats we cannot detect, avoids unnecessary ffmpeg overhead).

Tests

5 new tests added to tests/test_media_utils.py, all passing:

Test Scenario Expected
amr_with_wav_extension_does_not_short_circuit AMR content, .wav ext, target wav ffmpeg invoked
real_wav_with_wav_extension_short_circuits WAV content, .wav ext, target wav no conversion
unknown_content_with_matching_ext_short_circuits unrecognised content, .wav ext, target wav no conversion
real_amr_with_amr_extension_short_circuits AMR content, .amr ext, target amr no conversion
wav_with_ogg_extension_does_not_short_circuit WAV content, .ogg ext, target ogg ffmpeg invoked

Full suite: 55 passed. ruff format / ruff check clean.

Closes #9594.

Summary by Sourcery

Validate audio content against its detected format before skipping conversion based on the file extension.

Bug Fixes:

  • Prevent audio files whose extensions do not match their detected magic-byte format from bypassing conversion and reaching downstream consumers with invalid data.

Enhancements:

  • Preserve conversion short-circuiting for matching, detectable formats and missing files while covering extension/content mismatch scenarios with focused tests.

Tests:

  • Add coverage for matching formats, mismatched extensions, AMR/WAV detection, unknown content, and missing audio files.

@dosubot dosubot Bot added size:S This PR changes 10-29 lines, ignoring generated files. area:core The bug / feature is about astrbot's core, backend labels Aug 12, 2026

@sourcery-ai sourcery-ai Bot left a comment

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.

Hey - I've left some high level feedback:

  • The ffmpeg subprocess mocking logic (including _FakeFFmpegProcess and the fake_exec setup) is duplicated across several tests; consider extracting a reusable helper or fixture to keep the test suite DRY and easier to maintain.
  • In _FakeFFmpegProcess, the stderr parameter is accepted but never used; either wire it into communicate() or remove the parameter to avoid confusion about its purpose.
Prompt for AI Agents
Please address the comments from this code review:

## Overall Comments
- The ffmpeg subprocess mocking logic (including `_FakeFFmpegProcess` and the `fake_exec` setup) is duplicated across several tests; consider extracting a reusable helper or fixture to keep the test suite DRY and easier to maintain.
- In `_FakeFFmpegProcess`, the `stderr` parameter is accepted but never used; either wire it into `communicate()` or remove the parameter to avoid confusion about its purpose.

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.

xiaoyuyu6420 added a commit to xiaoyuyu6420/AstrBot that referenced this pull request Aug 13, 2026
Address Sourcery review feedback on AstrBotDevs#9651:
- Extract _patch_ffmpeg() helper to deduplicate the fake_exec setup
  across the three conversion-expected tests.
- Remove the unused stderr parameter from _FakeFFmpegProcess.
@xiaoyuyu6420

Copy link
Copy Markdown
Author

Addressed both Sourcery notes:

  • Unused stderr param — removed in b5e2b31.
  • Duplicated ffmpeg mockingb5e2b31 extracted _patch_ffmpeg for the invoked-ffmpeg path and dropped stderr; ac7b2d6 finishes the job by collapsing the three duplicated "never-called" fake_exec stubs into a shared _patch_ffmpeg_not_called(monkeypatch, reason) helper (per-scenario reason strings preserved).

Verified: pytest tests/test_media_utils.py -k convert_audio_format — 5 passed; full file — 54 passed, 1 pre-existing Windows-only failure in test_file_uri_to_path_* (unrelated to this change, fails on upstream baseline too); ruff check clean.

xiaoyuyu6420 and others added 4 commits August 28, 2026 19:15
… convert_audio_format

When a platform (e.g. NapCat) saves AMR-encoded audio with a .wav
extension, the extension-only short-circuit returned the raw AMR file
unchanged. Downstream STT providers then received malformed WAV data
and returned HTTP 400.

The fix reuses the existing _get_audio_magic_type() helper to verify
the file content actually matches the target format before skipping
ffmpeg conversion. When the detected format differs from the output
format, conversion proceeds normally. Unrecognised content still falls
back to extension matching to preserve existing behaviour.

Closes AstrBotDevs#9594.
Address Sourcery review feedback on AstrBotDevs#9651:
- Extract _patch_ffmpeg() helper to deduplicate the fake_exec setup
  across the three conversion-expected tests.
- Remove the unused stderr parameter from _FakeFFmpegProcess.
Collapse the three duplicated fake_exec stubs in the short-circuit
tests into _patch_ffmpeg_not_called(monkeypatch, reason), finishing
the Sourcery review note on duplicated ffmpeg mocking.
…ication

The core fix for AstrBotDevs#9594 was already merged in AstrBotDevs#9612 (detect audio format
from file content). This commit adapts the test suite to match that
implementation:

- Update unknown-content test: master converts unrecognised formats
  rather than short-circuiting (safer behavior).
- Add missing-file test: master returns the path as-is when the file
  does not exist yet (NapCat race condition handling).
- Update section header to reference both AstrBotDevs#9594 and AstrBotDevs#9612.
@xiaoyuyu6420
xiaoyuyu6420 force-pushed the fix/9594-amr-audio-magic-bytes branch from ac7b2d6 to 394d4a6 Compare August 28, 2026 11:19
@dosubot dosubot Bot added size:XS This PR changes 0-9 lines, ignoring generated files. and removed size:S This PR changes 10-29 lines, ignoring generated files. labels Aug 28, 2026

@sourcery-ai sourcery-ai Bot left a comment

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.

Sourcery assessment

Approved.

@xiaoyuyu6420

Copy link
Copy Markdown
Author

Friendly ping @Soulter — rebased onto current master. The core fix for #9594 was already merged in #9612; this PR now provides test coverage for the magic-byte verification logic. Sourcery approved. CI awaits fork-PR approval.

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 size:XS This PR changes 0-9 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug] QQ语音消息(AMR)因 convert_audio_format 后缀短路导致 STT 失败 (HTTP 400)

1 participant