Fix mpv command and template parsing #4162

deluan · 2025-06-01T02:01:11Z

This pull request introduces improvements to the MPV playback command handling and adds comprehensive testing for the mpv module. Key changes include the adoption of a shell-parsing library to handle complex command templates, better error handling for invalid arguments, and the addition of unit tests to ensure robustness.

Improvements to MPV command handling:

Replaced naive string splitting with shellquote.Split for parsing the MPVCmdTemplate, allowing proper handling of quoted arguments and spaces in paths.
Enhanced error handling in createMPVCommand and start functions to return meaningful errors when arguments are invalid or missing. [1] [2]

Testing enhancements:

Added a new test suite (mpv_suite_test.go) and extensive unit tests (mpv_test.go) for the mpv module, covering various scenarios such as handling spaces, malformed templates, and snapcast integration. [1] [2]

Dependency updates:

Added github.com/kballard/go-shellquote as a direct dependency in go.mod to support the improved command parsing logic. [1] [2]

Fix #3619

Signed-off-by: Deluan <deluan@navidrome.org>

… a file Signed-off-by: Deluan <deluan@navidrome.org>

Signed-off-by: Deluan <deluan@navidrome.org>

…ents Signed-off-by: Deluan <deluan@navidrome.org>

gemini-code-assist

Hello @deluan, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

Summary of Changes

Hello! Gemini here, providing a summary of this pull request to help everyone get up to speed quickly.

This PR aims to fix an issue (specifically #3619) where the MPV command template configured by the user was not being parsed correctly, leading to problems when the template or the substituted values (like file paths or device names) contained spaces or special characters that needed quoting. The previous implementation used a simple strings.Split which doesn't respect shell quoting rules.

The core change involves replacing the naive string splitting with a proper shell quoting parser (github.com/kballard/go-shellquote) to correctly interpret the command template and its arguments before substituting the placeholders (%d, %f, %s). This ensures that arguments containing spaces are treated as single arguments when passed to the MPV executable.

Additionally, comprehensive tests have been added for the createMPVCommand function to cover various scenarios, including paths with spaces, complex device names, and the specific template structure that caused issue #3619, ensuring the fix works as expected and preventing regressions.

Highlights

MPV Command Parsing Fix: Replaced simple strings.Split with shellquote.Split for parsing the MPVCmdTemplate configuration, correctly handling spaces and quotes in arguments and substituted values.
Issue #3619 Resolution: Specifically addresses and fixes the command parsing issue reported in #3619, particularly relevant for configurations involving paths or arguments with spaces, like certain Snapcast setups.
Improved Test Coverage: Added new, detailed tests for the createMPVCommand function covering various template and argument structures, including edge cases with spaces and quoting, using a mock MPV script to verify passed arguments.
Code Cleanup: Removed the now unnecessary fixCmd helper function as its logic is integrated into the improved createMPVCommand.

Changelog

Click here to see the changelog

core/playback/mpv/mpv.go
- Imported github.com/kballard/go-shellquote.
- Rewrote createMPVCommand to use shellquote.Split for parsing the command template.
- Added fallback to strings.Fields if shellquote.Split fails, logging a warning.
- Integrated the logic for replacing 'mpv' with the configured MPVPath into createMPVCommand.
- Removed the deprecated fixCmd function.
core/playback/mpv/mpv_suite_test.go
- Added a new Ginkgo test suite file for the mpv package.
core/playback/mpv/mpv_test.go
- Added comprehensive tests for createMPVCommand covering various scenarios including spaces, quoting, and the specific template from issue #3619.
- Added tests for the start function to verify that the correctly parsed arguments are passed to the executed command.
- Included a helper function createMockMPVScript to create executable scripts that output their arguments for testing purposes.
- Added tests for the mpvCommand function to ensure it correctly finds the configured MPV path.
go.mod
- Added github.com/kballard/go-shellquote as a direct dependency.
- Removed github.com/kballard/go-shellquote from indirect dependencies.

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.

Feature	Command	Description
Code Review	`/gemini review`	Performs a code review for the current pull request in its current state.
Pull Request Summary	`/gemini summary`	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in issue comments and review comments.
Help	`/gemini help`	Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

A template parsed wrong,
Arguments went astray,
Shellquote makes them strong.

Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

gemini-code-assist

Code Review

This pull request significantly improves the parsing of MPV command templates by introducing go-shellquote, which robustly handles spaces and quoted arguments. This directly addresses issue #3619. The addition of comprehensive unit tests for createMPVCommand and the start functionality is also a valuable contribution, ensuring these changes are well-verified.

Overall, the changes are positive and enhance the reliability of MPV integration. I have one main point of feedback regarding the condition used to substitute the MPV executable path, which could lead to unexpected behavior in certain configurations.

Summary of Findings

MPV Executable Path Substitution Logic: The condition for replacing the command in the MPV template (core/playback/mpv/mpv.go:95) uses strings.Contains(templateArgs[0], "mpv"). This is too broad and could incorrectly replace user-specified wrapper scripts or tools if their names contain "mpv". It's recommended to use a more precise condition, like checking for exact matches "mpv" or "mpv.exe".

Merge Readiness

The pull request introduces valuable improvements. However, there is one high-severity issue regarding the logic for MPV executable path substitution that should be addressed to prevent potential unexpected behavior with user configurations. I recommend discussing and potentially revising this logic before merging. As an AI reviewer, I am not authorized to approve pull requests; please ensure further review and approval from team members.

core/playback/mpv/mpv.go

Copilot

Pull Request Overview

This PR fixes the mpv command and template parsing by correctly handling quoted arguments and resolving the correct mpv executable path. Key changes include:

Adding a direct dependency on the shellquote package in go.mod.
Updating the command creation in mpv.go to use shellquote.Split for proper argument parsing.
Adding comprehensive unit tests to verify command construction and execution.

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

File	Description
go.mod	Adds a direct dependency on github.com/kballard/go-shellquote.
core/playback/mpv/mpv_test.go	Introduces tests validating various MPV command template scenarios.
core/playback/mpv/mpv_suite_test.go	Adds suite tests for the mpv package.
core/playback/mpv/mpv.go	Replaces naive space splitting with shellquote-based parsing logic.

core/playback/mpv/mpv.go

github-actions · 2025-06-01T02:10:16Z

…ontainment Signed-off-by: Deluan <deluan@navidrome.org>

Copilot

Pull Request Overview

This PR addresses issue #3619 by fixing the mpv command and template parsing. The key changes include:

Adding the go-shellquote dependency to properly handle shell argument parsing.
Updating the command template parsing logic in createMPVCommand and start functions.
Adding error checks to ensure that empty command arguments are handled gracefully.

Reviewed Changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
go.mod	Added dependency for proper shell argument parsing via go-shellquote.
core/playback/mpv/track.go	Added error check for empty command argument list in NewTrack.
core/playback/mpv/mpv.go	Replaced naive string splitting with shellquote.Split and improved error checks.
core/playback/mpv/mpv_test.go	Added tests covering various template parsing scenarios and error conditions.
core/playback/mpv/mpv_suite_test.go	Added overall mpv integration test suite.

core/playback/mpv/track.go

core/playback/mpv/mpv.go

…d templates Signed-off-by: Deluan <deluan@navidrome.org>

deluan added 5 commits May 31, 2025 20:14

test(mpv): add unit tests for MPV command generation and execution

9d0abba

Signed-off-by: Deluan <deluan@navidrome.org>

fix(mpv): improve command template parsing

d3d3bfb

Signed-off-by: Deluan <deluan@navidrome.org>

fix(mpv): update mock script to output arguments to stdout instead of…

ea8dacd

… a file Signed-off-by: Deluan <deluan@navidrome.org>

test(mpv): add test suite for MPV command functionality

7f6d0a9

Signed-off-by: Deluan <deluan@navidrome.org>

fix(mpv): improve MPV command template parsing to handle quoted argum…

6cd6575

…ents Signed-off-by: Deluan <deluan@navidrome.org>

gemini-code-assist bot reviewed Jun 1, 2025

View reviewed changes

gemini-code-assist bot suggested changes Jun 1, 2025

View reviewed changes

core/playback/mpv/mpv.go Outdated Show resolved Hide resolved

deluan mentioned this pull request Jun 1, 2025

fix(mpv.go): split commands via RegEx - 3619 #4161

Closed

deluan requested a review from Copilot June 1, 2025 02:06

Copilot AI reviewed Jun 1, 2025

View reviewed changes

core/playback/mpv/mpv.go Outdated Show resolved Hide resolved

fix(mpv): simplify MPV command check by removing unnecessary string c…

9dea4fb

…ontainment Signed-off-by: Deluan <deluan@navidrome.org>

deluan requested a review from Copilot June 3, 2025 00:41

Copilot AI reviewed Jun 3, 2025

View reviewed changes

core/playback/mpv/track.go Show resolved Hide resolved

core/playback/mpv/mpv.go Outdated Show resolved Hide resolved

fix(mpv): add error handling for empty command arguments and malforme…

79e6a5b

…d templates Signed-off-by: Deluan <deluan@navidrome.org>

deluan force-pushed the fix-mpv-command branch from a74935b to 79e6a5b Compare June 3, 2025 00:43

deluan merged commit 011f589 into master Jun 3, 2025
35 checks passed

deluan deleted the fix-mpv-command branch June 3, 2025 00:52

BrewTestBot mentioned this pull request Jul 1, 2025

navidrome 0.57.0 Homebrew/homebrew-core#228738

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

Fix mpv command and template parsing #4162

Fix mpv command and template parsing #4162

Uh oh!

deluan commented Jun 1, 2025 •

edited

Loading

Uh oh!

gemini-code-assist bot left a comment

Uh oh!

gemini-code-assist bot left a comment

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

github-actions bot commented Jun 1, 2025 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Fix mpv command and template parsing #4162

Fix mpv command and template parsing #4162

Uh oh!

Conversation

deluan commented Jun 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Improvements to MPV command handling:

Testing enhancements:

Dependency updates:

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Summary of Changes

Highlights

Changelog

Footnotes

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Summary of Findings

Merge Readiness

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

github-actions bot commented Jun 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

deluan commented Jun 1, 2025 •

edited

Loading

github-actions bot commented Jun 1, 2025 •

edited

Loading