Convert briefing audio to MP3 before upload #2

Merged

nimmo merged 7 commits from mp3-audio-upload into main 2026-07-21 23:17:51 +01:00

Conversation 4 Commits 7 Files changed 3 +66 -6

nimmo commented 2026-07-21 21:40:42 +01:00

Owner

Copy link

Summary

• Convert the WAV that pocket-tts produces to MP3 with ffmpeg + libmp3lame before uploading to ntfy, mirroring the forgejo-daily-report pipeline
• Set Content-Type: audio/mpeg on the audio upload (it was previously sent as an unlabelled binary)
• Add FFMPEG_BIN and MP3_BITRATE env-var overrides (defaults ffmpeg and 96k)
• Add ffmpeg to the Nix runtime and dev shell

Context

The TTS step already produced a WAV, but WAV attachments to ntfy are large and the upload was going out with no Content-Type. The forgejo-daily-report repo solved this with a ffmpeg step, so this PR ports that pattern over.

The conversion is wrapped in a convert_to_mp3() helper with a 120 s timeout. If ffmpeg is missing, the script falls back to uploading the WAV rather than failing the run, so the text notification still goes out.

Notes

• ffmpeg is now required for the audio attachment to be MP3. Without it, audio uploads remain as WAV and the text notification still delivers.
• The deterministic AI review check is python3 -m py_compile daily-briefing.py, which still passes.
• No changes to existing public behaviour when ffmpeg is absent.

## Summary - Convert the WAV that `pocket-tts` produces to MP3 with `ffmpeg + libmp3lame` before uploading to ntfy, mirroring the `forgejo-daily-report` pipeline - Set `Content-Type: audio/mpeg` on the audio upload (it was previously sent as an unlabelled binary) - Add `FFMPEG_BIN` and `MP3_BITRATE` env-var overrides (defaults `ffmpeg` and `96k`) - Add `ffmpeg` to the Nix runtime and dev shell ## Context The TTS step already produced a WAV, but WAV attachments to ntfy are large and the upload was going out with no `Content-Type`. The `forgejo-daily-report` repo solved this with a `ffmpeg` step, so this PR ports that pattern over. The conversion is wrapped in a `convert_to_mp3()` helper with a 120 s timeout. If ffmpeg is missing, the script falls back to uploading the WAV rather than failing the run, so the text notification still goes out. ## Notes - ffmpeg is now required for the audio attachment to be MP3. Without it, audio uploads remain as WAV and the text notification still delivers. - The deterministic AI review check is `python3 -m py_compile daily-briefing.py`, which still passes. - No changes to existing public behaviour when ffmpeg is absent.

nimmo added 1 commit 2026-07-21 21:40:43 +01:00

Convert briefing audio to MP3 before upload ... 1554b63d70

The TTS step produces a WAV from pocket-tts, which is too large to
attach to an ntfy message and forces a binary blob with no useful
Content-Type. Mirror the forgejo-daily-report pipeline: convert the
WAV to MP3 with ffmpeg + libmp3lame, set Content-Type: audio/mpeg on
the upload, and clean up both files.

- Add convert_to_mp3() helper using ffmpeg subprocess (timeout-bounded).
- Read FFMPEG_BIN and MP3_BITRATE from the environment, defaulting to
  'ffmpeg' on PATH and '96k'.
- Set the audio Content-Type on the ntfy upload based on the file
  extension (audio/mpeg for .mp3, audio/wav as a fallback).
- If ffmpeg is missing, fall back to uploading the WAV rather than
  failing the run, so the text notification still goes out.
- Add ffmpeg to the Nix runtime and dev shell.

nimmo commented 2026-07-21 21:44:32 +01:00

Author

Owner

Copy link

Automated code review

Reviewed commit: a472dc2c4d3155c358cff9c07b15d9930f4ad0ed

Verdict: Ready to merge

No blocking or important findings were retained, and deterministic checks passed. Suggestions remain advisory.

Overall assessment

The pull request implements WAV-to-MP3 conversion via ffmpeg with a graceful fallback to WAV, adds explicit Content-Type headers for ntfy raw uploads, and applies defensive stderr decoding (errors='replace') across subprocess calls. Module-level environment variable capture for FFMPEG_BIN and MP3_BITRATE has been corrected to read at call time, preventing race conditions with load_env(). Silent OSError suppression during temp file cleanup is now logged. The diff aligns with the provided human context and addresses previously flagged concerns.

Blocking findings

None.

Important findings

None.

Suggestions

None.

Tests and validation

• python-syntax-check: success

Questions

• Has the WAV fallback path been explicitly tested against the production ntfy instance to confirm successful delivery when ffmpeg is absent or conversion fails?

Review limitations

• Review is strictly limited to the provided diff, commit messages, and human replies; runtime validation (e.g., nix flake check, actual TTS/ffmpeg execution, and live ntfy upload behavior) was not performed.
• No assessment of external service rate limits, ffmpeg version compatibility across target environments, or long-term disk usage from temp file cleanup edge cases.

Diff coverage

• README.md: reviewed — included in a context-limited batch
• daily-briefing.py: reviewed — included in a context-limited batch
• flake.nix: reviewed — included in a context-limited batch

Review metadata

• Reviewer: forgejo-ai-review v1
• Model: qwen3.6:35b-a3b-q8_0
• Guidance loaded: .forgejo/ai-review.md, .forgejo/ai-review.yml, AGENTS.md, README.md
• Commits considered: 7
• Previous automated review: updated
• Findings: 0

<!-- forgejo-ai-review:v1 --> ## Automated code review Reviewed commit: `a472dc2c4d3155c358cff9c07b15d9930f4ad0ed` ## Verdict: Ready to merge No blocking or important findings were retained, and deterministic checks passed. Suggestions remain advisory. ### Overall assessment The pull request implements WAV-to-MP3 conversion via ffmpeg with a graceful fallback to WAV, adds explicit Content-Type headers for ntfy raw uploads, and applies defensive stderr decoding (errors='replace') across subprocess calls. Module-level environment variable capture for FFMPEG_BIN and MP3_BITRATE has been corrected to read at call time, preventing race conditions with load_env(). Silent OSError suppression during temp file cleanup is now logged. The diff aligns with the provided human context and addresses previously flagged concerns. ### Blocking findings None. ### Important findings None. ### Suggestions None. ### Tests and validation - python-syntax-check: **success** ### Questions - Has the WAV fallback path been explicitly tested against the production ntfy instance to confirm successful delivery when ffmpeg is absent or conversion fails? ### Review limitations - Review is strictly limited to the provided diff, commit messages, and human replies; runtime validation (e.g., nix flake check, actual TTS/ffmpeg execution, and live ntfy upload behavior) was not performed. - No assessment of external service rate limits, ffmpeg version compatibility across target environments, or long-term disk usage from temp file cleanup edge cases. ### Diff coverage - `README.md`: **reviewed** — included in a context-limited batch - `daily-briefing.py`: **reviewed** — included in a context-limited batch - `flake.nix`: **reviewed** — included in a context-limited batch <details><summary>Review metadata</summary> - Reviewer: forgejo-ai-review v1 - Model: qwen3.6:35b-a3b-q8_0 - Guidance loaded: .forgejo/ai-review.md, .forgejo/ai-review.yml, AGENTS.md, README.md - Commits considered: 7 - Previous automated review: updated - Findings: 0 </details>

nimmo referenced this pull request from a commit 2026-07-21 22:32:47 +01:00

Log cleanup failures instead of silently swallowing

nimmo added 1 commit 2026-07-21 22:32:50 +01:00

Log cleanup failures instead of silently swallowing ... b1e00fa1f7

The path.exists() check already covers the 'already gone' race, so any
remaining OSError from unlink() is a real system issue worth surfacing
(disk full, permission denied, read-only mount). Logged rather than
suppressed, matching the rest of the script.

Addresses the suggestion from the advisory AI review on PR #2.

nimmo commented 2026-07-21 22:35:16 +01:00

Author

Owner

Copy link

One of the suggestions from the previous automated review was a false positive, worth flagging for the next pass:

Verify ntfy raw file upload compatibility (daily-briefing.py:67)
ntfy typically expects multipart/form-data for attachments. Sending raw bytes with a custom Content-Type may work but could fail on certain server configurations or older versions.

This pattern (raw body with explicit Content-Type) is correct and is what forgejo-daily-report uses in production against the same ntfy instance — see forgejo-daily-report/forgejo-daily-report.py around line 1192-1198, which posts audio/mpeg audio with data=audio. The previous code on main was already doing this for the WAV (it just omitted the Content-Type header), and the dedicated audio branch in ntfy() predates this PR. The PR only adds the missing Content-Type and the fallback; it does not change the upload mechanism.

The other suggestion (silent OSError suppression in cleanup) was valid and has been addressed in commit b1e00fa: the path.exists() guard still handles the race against the file being already gone, and any remaining OSError is now logged with the path and the exception.

One of the suggestions from the previous automated review was a false positive, worth flagging for the next pass: > **Verify ntfy raw file upload compatibility** (daily-briefing.py:67) > ntfy typically expects multipart/form-data for attachments. Sending raw bytes with a custom Content-Type may work but could fail on certain server configurations or older versions. This pattern (raw body with explicit `Content-Type`) is correct and is what `forgejo-daily-report` uses in production against the same ntfy instance — see `forgejo-daily-report/forgejo-daily-report.py` around line 1192-1198, which posts `audio/mpeg` audio with `data=audio`. The previous code on `main` was already doing this for the WAV (it just omitted the Content-Type header), and the dedicated audio branch in `ntfy()` predates this PR. The PR only adds the missing Content-Type and the fallback; it does not change the upload mechanism. The other suggestion (silent `OSError` suppression in cleanup) was valid and has been addressed in commit `b1e00fa`: the `path.exists()` guard still handles the race against the file being already gone, and any remaining `OSError` is now logged with the path and the exception.

nimmo referenced this pull request from a commit 2026-07-21 22:42:44 +01:00

Decode ffmpeg stderr with errors='replace'

nimmo added 1 commit 2026-07-21 22:42:46 +01:00

Decode ffmpeg stderr with errors='replace' ... 765bd67d54

text=True uses the default UTF-8 codec with errors='strict', so a
non-UTF-8 byte in ffmpeg's stderr would raise UnicodeDecodeError
and mask the actual conversion failure. Drop text=True and decode
the bytes explicitly with errors='replace' so the failure path
always logs a useful message.

Addresses the suggestion from the advisory AI review on PR #2.

nimmo added 1 commit 2026-07-21 22:49:30 +01:00

Decode pocket-tts stderr with errors='replace' ... 024a3a3552

Same defensive fix as in convert_to_mp3: text=True uses errors='strict'
and would raise UnicodeDecodeError on a non-UTF-8 byte in stderr,
masking the actual TTS failure with a misleading 'failed to start'
log line. Apply the same errors='replace' decode here so the
failure path always logs a useful message.

nimmo commented 2026-07-21 22:51:09 +01:00

Author

Owner

Copy link

Following the previous review's high-confidence finding on subprocess text=True decoding (convert_to_mp3), I applied the same errors='replace' fix to the sibling generate_tts() function in commit 024a3a3.

Same root cause, same fix, same reasoning:

• text=True uses the default UTF-8 codec with errors='strict'
• A non-UTF-8 byte in pocket-tts's stderr would raise UnicodeDecodeError
• The outer except Exception in generate_tts would log it as a misleading "TTS generation failed to start" — even if the WAV was actually written
• The audio path would fall through to text-only delivery with a confusing log line

Folding it in rather than opening a separate issue: the change is a one-line mirror of the existing fix, the same defensive pattern, and would otherwise just get flagged again on the next pass. Two call sites of the same decode idiom, same fix applied to both.

Following the previous review's high-confidence finding on `subprocess` `text=True` decoding (`convert_to_mp3`), I applied the same `errors='replace'` fix to the sibling `generate_tts()` function in commit `024a3a3`. Same root cause, same fix, same reasoning: - `text=True` uses the default UTF-8 codec with `errors='strict'` - A non-UTF-8 byte in `pocket-tts`'s stderr would raise `UnicodeDecodeError` - The outer `except Exception` in `generate_tts` would log it as a misleading "TTS generation failed to start" — even if the WAV was actually written - The audio path would fall through to text-only delivery with a confusing log line Folding it in rather than opening a separate issue: the change is a one-line mirror of the existing fix, the same defensive pattern, and would otherwise just get flagged again on the next pass. Two call sites of the same decode idiom, same fix applied to both.

nimmo added 1 commit 2026-07-21 22:51:42 +01:00

Document main() with a one-line docstring ... 2283540db1

Other top-level functions in this module carry docstrings; main()
was the outlier. Add a brief one matching the established style.

nimmo added 1 commit 2026-07-21 23:07:14 +01:00

Merge origin/main into mp3-audio-upload ... 7fb1637f90

Pull in the PR #1 merge commit (a69870e) so the branch is no longer
behind main. No conflicting changes — main only has the merge commit
and the existing config-file commit, both of which are disjoint
from the mp3 / TTS fixes in this branch.

nimmo commented 2026-07-21 23:12:35 +01:00

Author

Owner

Copy link

High-confidence finding on lines 28–29: FFMPEG_BIN and MP3_BITRATE are read at module scope, before load_env() runs in main(). Any value set in briefing.env is silently ignored — the user gets the shell environment or the hardcoded default, not what they put in the env file.

This is a real bug, not a false positive. The rest of the file (e.g. generate_tts reading POCKET_TTS_FALLBACK_VOICE, POCKET_TTS_TIMEOUT_SECS) reads env vars inside the function body, which is the correct pattern. The new constants broke that pattern.

Fix landing in a follow-up commit: drop the module-level capture, read inside convert_to_mp3() at call time, matching the existing style. MP3_CONVERT_TIMEOUT_SECS has no env-var read so it stays as a constant.

flake.nix doesn't need to change — ffmpeg is still a runtime input. README's documented env-var overrides are correct in spirit; the bug was in how the values were captured, not in the documentation.

High-confidence finding on lines 28–29: `FFMPEG_BIN` and `MP3_BITRATE` are read at module scope, before `load_env()` runs in `main()`. Any value set in `briefing.env` is silently ignored — the user gets the shell environment or the hardcoded default, not what they put in the env file. This is a real bug, not a false positive. The rest of the file (e.g. `generate_tts` reading `POCKET_TTS_FALLBACK_VOICE`, `POCKET_TTS_TIMEOUT_SECS`) reads env vars inside the function body, which is the correct pattern. The new constants broke that pattern. Fix landing in a follow-up commit: drop the module-level capture, read inside `convert_to_mp3()` at call time, matching the existing style. `MP3_CONVERT_TIMEOUT_SECS` has no env-var read so it stays as a constant. `flake.nix` doesn't need to change — ffmpeg is still a runtime input. README's documented env-var overrides are correct in spirit; the bug was in how the values were captured, not in the documentation.

nimmo referenced this pull request from a commit 2026-07-21 23:12:51 +01:00

Read FFMPEG_BIN and MP3_BITRATE at call time

nimmo added 1 commit 2026-07-21 23:12:51 +01:00

Read FFMPEG_BIN and MP3_BITRATE at call time ... a472dc2c4d

The previous module-level capture happened before load_env() ran
in main(), so values set in briefing.env for FFMPEG_BIN and
MP3_BITRATE were silently dropped. Read them inside convert_to_mp3()
to match the existing pattern used by generate_tts() (which reads
POCKET_TTS_FALLBACK_VOICE and POCKET_TTS_TIMEOUT_SECS the same
way). MP3_CONVERT_TIMEOUT_SECS stays as a hardcoded constant.

Resolves the blocking finding from the advisory AI review on PR #2.

nimmo merged commit 2c0505778a into main 2026-07-21 23:17:51 +01:00

nimmo deleted branch mp3-audio-upload 2026-07-21 23:17:52 +01:00

nimmo referenced this pull request from a commit 2026-07-21 23:17:52 +01:00

Merge pull request 'Convert briefing audio to MP3 before upload' (#2) from mp3-audio-upload into main

Sign in to join this conversation.

Reviewers

No reviewers

Labels

Clear labels

No items

No labels

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

1 participant

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference

nimmo/daily-briefing!2

No description provided.