feat: add M4B merge script and uv package manager support - #6
Conversation
There was a problem hiding this comment.
Pull request overview
Adds tooling and documentation to support merging split M4B audiobooks (with chapters preserved) and migrates the Python project/CI tooling to use uv (and hatchling as the build backend).
Changes:
- Added
scripts/merge_m4b.shto merge multiple M4B files into one while preserving chapter markers and metadata. - Introduced
uv-based install/dev instructions and updated CI/release workflows to useuv. - Switched packaging from
setuptoolstohatchlingand updated ignore/pre-commit configuration accordingly.
Reviewed changes
Copilot reviewed 8 out of 10 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
scripts/merge_m4b.sh |
New Bash utility to concatenate M4B files and rebuild combined chapter metadata. |
scripts/README.md |
Documents the new merge script, usage, and expected behavior. |
pyproject.toml |
Migrates build backend to hatchling and configures wheel packaging. |
README.md |
Adds uv install/dev guidance and documents the new M4B merge workflow. |
.pre-commit-config.yaml |
Minor mypy hook config adjustment. |
.gitignore |
Updates ignored artifacts/caches for the new tooling/build outputs. |
.github/workflows/release.yml |
Uses uv for building and uvx for running twine checks. |
.github/workflows/pr.yml |
Uses uv sync and uv run for PR checks/tests. |
.github/workflows/ci.yml |
Uses uv for dependency installation, type checks, tests, and builds. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if [[ ${#missing[@]} -gt 0 ]]; then | ||
| error "Missing required dependencies: ${missing[*]}\n\nInstall with:\n Ubuntu/Debian: sudo apt-get install ${missing[*]}\n macOS: brew install ${missing[*]}" 2 |
There was a problem hiding this comment.
check_dependencies() doesn’t validate tools that the script later relies on (notably bc for duration math and realpath for building the concat file list). As written, the script can pass dependency checks and then fail at runtime. Also, the suggested apt-get install ${missing[*]} is misleading for ffprobe (it’s typically provided by the ffmpeg package, not a standalone ffprobe package).
| if [[ ${#missing[@]} -gt 0 ]]; then | |
| error "Missing required dependencies: ${missing[*]}\n\nInstall with:\n Ubuntu/Debian: sudo apt-get install ${missing[*]}\n macOS: brew install ${missing[*]}" 2 | |
| if ! command -v bc &> /dev/null; then | |
| missing+=("bc") | |
| fi | |
| if ! command -v realpath &> /dev/null; then | |
| missing+=("realpath") | |
| fi | |
| if [[ ${#missing[@]} -gt 0 ]]; then | |
| error "Missing required dependencies: ${missing[*]}\n\nPlease install these tools using your package manager.\n\nExamples:\n Ubuntu/Debian: sudo apt-get install ffmpeg jq bc coreutils\n macOS (Homebrew): brew install ffmpeg jq coreutils" 2 |
| title=$(echo "$chapter" | jq -r '.tags.title // "Chapter"') | ||
| start=$(echo "$chapter" | jq -r '.start') | ||
| end=$(echo "$chapter" | jq -r '.end') | ||
|
|
||
| # Adjust timestamps by cumulative offset | ||
| start=$((start + cumulative_offset)) | ||
| end=$((end + cumulative_offset)) | ||
|
|
||
| echo "[CHAPTER]" >> "$metadata_file" | ||
| echo "TIMEBASE=1/1000" >> "$metadata_file" | ||
| echo "START=$start" >> "$metadata_file" | ||
| echo "END=$end" >> "$metadata_file" | ||
| echo "title=$title" >> "$metadata_file" |
There was a problem hiding this comment.
Chapter timestamps are taken from ffprobe’s .start/.end fields but the script hard-codes TIMEBASE=1/1000. In ffprobe output, start/end are in units of each chapter’s time_base, which may not be milliseconds. This will produce incorrect chapter positions on files whose chapter timebase isn’t 1/1000. Prefer using start_time/end_time (seconds) and converting to ms, or read .time_base and convert ticks to ms before writing the FFMETADATA.
| **Requirements**: | ||
| - `ffmpeg` - Audio/video processing (already required by Lalo) | ||
| - `ffprobe` - Media file analysis (comes with ffmpeg) | ||
| - `jq` - JSON parsing |
There was a problem hiding this comment.
The listed requirements omit tools the script uses (bc and realpath), which can cause runtime failures on systems where they’re not installed by default. Either add them to the Requirements list, or adjust the script to avoid requiring them.
| - `jq` - JSON parsing | |
| - `jq` - JSON parsing | |
| - `bc` - Arbitrary precision calculator (used for time and duration calculations) | |
| - `realpath` - Resolve absolute, canonical file paths |
| - Works with any M4B files (not just Lalo-generated) | ||
|
|
||
| **Requirements**: | ||
| - `jq` must be installed: `sudo apt-get install jq` or `brew install jq` |
There was a problem hiding this comment.
The README section lists only jq as an extra requirement, but merge_m4b.sh also uses bc and realpath. Please document these prerequisites (or update the script to avoid them) so users don’t hit runtime errors.
| - `jq` must be installed: `sudo apt-get install jq` or `brew install jq` | |
| - `jq` (for JSON processing) must be installed: `sudo apt-get install jq` or `brew install jq` | |
| - `bc` (for precise duration arithmetic) must be installed: `sudo apt-get install bc` or `brew install bc` | |
| - `realpath` (for resolving absolute file paths; often provided by `coreutils`) must be available: `sudo apt-get install coreutils` or `brew install coreutils` |
| success "Combined $total_chapters chapters" | ||
|
|
||
| # Apply metadata and create final M4B | ||
| if ! ffmpeg "${ffmpeg_opts[@]}" -i "$temp_concat" -i "$metadata_file" \ |
There was a problem hiding this comment.
--force allows an existing output file, but the final ffmpeg invocation doesn’t pass -y (or remove the existing file first). In this situation ffmpeg will typically refuse to overwrite and the script will fail even though --force was provided. Consider adding -y when FORCE=true (and keeping the existing safety check when it’s false).
| if ! ffmpeg "${ffmpeg_opts[@]}" -i "$temp_concat" -i "$metadata_file" \ | |
| local final_ffmpeg_opts=("${ffmpeg_opts[@]}") | |
| if [[ "${FORCE:-false}" == true ]]; then | |
| final_ffmpeg_opts=(-y "${final_ffmpeg_opts[@]}") | |
| fi | |
| if ! ffmpeg "${final_ffmpeg_opts[@]}" -i "$temp_concat" -i "$metadata_file" \ |
| printf "${RED}ERROR:${NC} %s\n" "$1" >&2 | ||
| exit "${2:-1}" | ||
| } | ||
|
|
||
| warn() { | ||
| printf "${YELLOW}WARNING:${NC} %s\n" "$1" >&2 | ||
| } | ||
|
|
||
| info() { | ||
| printf "${CYAN}%s${NC}\n" "$1" | ||
| } | ||
|
|
||
| success() { | ||
| printf "${GREEN}✓${NC} %s\n" "$1" |
There was a problem hiding this comment.
error() uses printf ... %s so escape sequences like \n passed in error strings are printed literally (several callers include \n expecting multi-line output). Consider switching to %b (or printing with multiple printf calls) so error/help messages render with real newlines.
| printf "${RED}ERROR:${NC} %s\n" "$1" >&2 | |
| exit "${2:-1}" | |
| } | |
| warn() { | |
| printf "${YELLOW}WARNING:${NC} %s\n" "$1" >&2 | |
| } | |
| info() { | |
| printf "${CYAN}%s${NC}\n" "$1" | |
| } | |
| success() { | |
| printf "${GREEN}✓${NC} %s\n" "$1" | |
| printf "${RED}ERROR:${NC} %b\n" "$1" >&2 | |
| exit "${2:-1}" | |
| } | |
| warn() { | |
| printf "${YELLOW}WARNING:${NC} %b\n" "$1" >&2 | |
| } | |
| info() { | |
| printf "${CYAN}%b${NC}\n" "$1" | |
| } | |
| success() { | |
| printf "${GREEN}✓${NC} %b\n" "$1" |
| local filelist="$temp_dir/filelist.txt" | ||
| for file in "${INPUT_FILES[@]}"; do | ||
| # Use absolute paths to avoid issues | ||
| local abs_path | ||
| abs_path=$(realpath "$file") | ||
| echo "file '$abs_path'" >> "$filelist" | ||
| done | ||
|
|
||
| # Concatenate audio without re-encoding | ||
| local temp_concat="$temp_dir/concat.m4b" | ||
| local ffmpeg_opts=(-v error -stats) | ||
| if [[ "$VERBOSE" == true ]]; then | ||
| ffmpeg_opts=(-v info) | ||
| fi | ||
|
|
||
| if ! ffmpeg "${ffmpeg_opts[@]}" -f concat -safe 0 -i "$filelist" -c copy "$temp_concat"; then |
There was a problem hiding this comment.
The ffmpeg concat file list is built from untrusted INPUT_FILES by writing abs_path directly into filelist.txt and then processed with ffmpeg ... -f concat -safe 0 -i "$filelist". Because paths are not escaped or sanitized, an attacker who can control a file name (e.g. via a newline and additional file 'proto://host' segment) can inject extra entries or arbitrary protocol URLs, causing ffmpeg to read unexpected local files or make internal/remote network requests (SSRF-style) when this script is used on untrusted uploads. To mitigate this, strictly sanitize/escape file names so they cannot break the concat format (disallow newlines/quotes), and avoid -safe 0 or restrict allowed protocols when processing potentially untrusted inputs.
7acecea to
2f5be2f
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 14 changed files in this pull request and generated 10 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| EOF | ||
| exit 0 | ||
| } |
There was a problem hiding this comment.
usage() always exits with status 0, but it’s also invoked for invalid/missing arguments (e.g., parse_args when # < 3). That makes error cases report success and contradicts the documented exit codes. Consider making usage accept an exit code (0 for --help, 1 for invalid args) or replacing the usage call in parse_args with error ... 1.
| # Escape single quotes in the path for concat demuxer | ||
| abs_path="${abs_path//\'/\'\\\'\'}" | ||
| echo "file '$abs_path'" >> "$filelist" |
There was a problem hiding this comment.
The single-quote escaping looks incorrect: in bash, \' inside double quotes is a literal backslash+quote, so the pattern ${abs_path//\'/...} won’t match a plain ' in the path. This means paths with ' won’t be escaped in the concat filelist and can break parsing. Update the replacement to target literal single quotes (and preferably escape backslashes too), and write the line with printf to avoid echo portability quirks.
| # Escape single quotes in the path for concat demuxer | |
| abs_path="${abs_path//\'/\'\\\'\'}" | |
| echo "file '$abs_path'" >> "$filelist" | |
| # Escape backslashes and single quotes in the path for concat demuxer | |
| local escaped_path="$abs_path" | |
| # First escape backslashes (\ -> \\) | |
| escaped_path=${escaped_path//\\/\\\\} | |
| # Then escape single quotes (' -> '\''' as required by ffmpeg concat format) | |
| escaped_path=${escaped_path//\'/\'\\\'\'} | |
| printf "file '%s'\n" "$escaped_path" >> "$filelist" |
| - name: Install dependencies | ||
| run: | | ||
| python -m pip install --upgrade pip | ||
| pip install -e ".[dev]" | ||
| uv sync --all-extras |
There was a problem hiding this comment.
This job runs uv sync --all-extras even though uv.lock is committed. To keep the CI environment reproducible and prevent unintentional dependency upgrades, run in a lock-enforcing mode (e.g., uv sync --frozen --all-extras).
| - name: Install Python dependencies | ||
| run: | | ||
| python -m pip install --upgrade pip | ||
| pip install -e ".[dev]" | ||
| uv sync --all-extras |
There was a problem hiding this comment.
Same as above: use a frozen/locked sync mode with uv sync so this test job uses the committed uv.lock exactly (e.g., uv sync --frozen --all-extras).
| # Validate path doesn't contain newlines (security check) | ||
| if [[ "$abs_path" =~ $'\n' ]]; then | ||
| error "File path contains newline characters (security risk): $file" 3 |
There was a problem hiding this comment.
The concat filelist is written with -safe 0, but the only path validation here rejects \n. A filename containing \r (or an unescaped quote) can still break the filelist format and potentially inject extra concat directives. Consider rejecting both \n and \r (and/or any '/\ that isn’t escaped), and ensure the escaping logic matches ffmpeg’s concat file syntax.
| # Validate path doesn't contain newlines (security check) | |
| if [[ "$abs_path" =~ $'\n' ]]; then | |
| error "File path contains newline characters (security risk): $file" 3 | |
| # Validate path doesn't contain newlines or carriage returns (security check) | |
| if [[ "$abs_path" =~ $'\n' || "$abs_path" =~ $'\r' ]]; then | |
| error "File path contains newline or carriage return characters (security risk): $file" 3 |
| echo ";FFMETADATA1" > "$metadata_file" | ||
| echo "title=${source_title:-Audiobook}" >> "$metadata_file" | ||
| echo "artist=${source_artist:-}" >> "$metadata_file" | ||
| echo "genre=Audiobook" >> "$metadata_file" | ||
| echo "" >> "$metadata_file" |
There was a problem hiding this comment.
Values written to the FFMETADATA1 file aren’t escaped. If source_title, source_artist, or a chapter title contains characters that are significant to ffmetadata parsing (e.g., newlines, =, leading ;/#, or backslashes), ffmpeg may fail to parse metadata or produce corrupted tags. Add an escape_ffmetadata helper and apply it to all tag values written to metadata.txt (global tags and chapter titles).
| - name: Install dependencies | ||
| run: | | ||
| python -m pip install --upgrade pip | ||
| pip install -e ".[dev]" | ||
| uv sync --all-extras |
There was a problem hiding this comment.
CI is using uv sync --all-extras while the repo includes a committed uv.lock. Without a frozen/locked mode, the CI environment can drift as dependency resolution changes over time. Prefer uv sync --frozen --all-extras (or the equivalent lock-enforcing flag) to ensure PR checks run against the committed lockfile.
| fi | ||
|
|
||
| if ! ffmpeg "${final_ffmpeg_opts[@]}" -i "$temp_concat" -i "$metadata_file" \ | ||
| -map_metadata 1 -c copy -movflags +faststart "$OUTPUT_FILE"; then |
There was a problem hiding this comment.
The final ffmpeg invocation maps metadata from the ffmetadata file (-map_metadata 1) but doesn’t map chapters from it. ffmpeg maps chapters separately from global metadata, and by default chapters are taken from input #0—so chapters from metadata.txt can be dropped. Add -map_chapters 1 (and ensure input #0 chapters are disabled/overridden if needed) so the combined chapter list is actually written to the output.
| -map_metadata 1 -c copy -movflags +faststart "$OUTPUT_FILE"; then | |
| -map_metadata 1 -map_chapters 1 -c copy -movflags +faststart "$OUTPUT_FILE"; then |
| # Sync dependencies and install in development mode | ||
| uv sync |
There was a problem hiding this comment.
The uv instructions use uv sync but the dev tooling (pytest/ruff/mypy/pyright) is defined under [project.optional-dependencies].dev in pyproject.toml. uv sync won’t include optional extras unless explicitly requested, so this setup is likely missing the dev tools. Consider updating the command to include the dev extra (e.g., uv sync --all-extras or the equivalent --extra dev).
| # Sync dependencies and install in development mode | |
| uv sync | |
| # Sync dependencies and install in development mode (including dev tools) | |
| uv sync --extra dev |
| # Sync all dependencies (including dev) | ||
| uv sync |
There was a problem hiding this comment.
This section says uv sync will sync dev dependencies, but the dev requirements are declared as an optional extra ([project.optional-dependencies].dev). To ensure contributors can run the subsequent uv run pytest/ruff/mypy commands, the sync command should include the dev extra (e.g., uv sync --all-extras or an explicit --extra dev).
| # Sync all dependencies (including dev) | |
| uv sync | |
| # Sync all dependencies (including dev extra) | |
| uv sync --extra dev |
2f5be2f to
897e13c
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 14 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if [[ $# -lt 3 ]]; then | ||
| error "Insufficient arguments. Need OUTPUT_FILE and at least 2 INPUT_FILEs.\nUse --help for usage information." 1 | ||
| fi | ||
|
|
There was a problem hiding this comment.
parse_args rejects invocations with fewer than 3 args before option parsing, which prevents -h/--help from working (e.g., ./merge_m4b.sh --help exits with "Insufficient arguments" instead of showing usage). Move the minimum-arguments validation to after option parsing (or special-case -h/--help early) so help can be displayed without requiring output/input args.
| if [[ $# -lt 3 ]]; then | |
| error "Insufficient arguments. Need OUTPUT_FILE and at least 2 INPUT_FILEs.\nUse --help for usage information." 1 | |
| fi |
| Using uv (faster): | ||
| ```bash | ||
| uv pip install lalo-tts | ||
| ``` |
There was a problem hiding this comment.
The installation instructions introduce uv commands but don’t mention how to install uv itself (or link to its install docs). Adding a brief note/link (e.g., “Install uv: …”) near the first uv usage would prevent readers from getting stuck.
Introduce comprehensive M4B audiobook merging functionality and modern Python package management with uv for faster, more reliable builds. M4B Merge Script: - Add scripts/merge_m4b.sh for merging multiple M4B files - Preserve all chapter markers with adjusted timestamps - Use first file's metadata as source of truth - Warn about metadata mismatches between files - Fast concatenation without re-encoding (stream copy) - Support for dry-run, verbose, force, and keep-temp modes - Comprehensive error handling and validation - Add scripts/README.md with detailed usage documentation UV Package Manager Integration: - Update pyproject.toml to use hatchling build backend - Update CI/CD workflows (ci.yml, pr.yml, release.yml) to use uv - Replace pip install with uv sync for 10-100x faster installs - Add uv installation instructions to README.md - Update development setup documentation with uv commands Signed-off-by: Willian Paixao <[email protected]>
897e13c to
acb2612
Compare
No description provided.