Add cli command artist/album/download commands. Enhance cli command play with --artist/--album/--songs to support batch play.

[yveming]

Add cli command artist/album to show songs of artist/album. Add cli command download to download songs of artist/album/playlist. Add --artist/--album/--songs to cli command play to support batch play. Update README.md and skills/musicbox/SKILL.md.

modified:   NEMbox/cli.py
modified:   README.md
modified:   skills/musicbox/SKILL.md

[darknessomi]

Thanks for the contribution — artist / album, batch play, and download are useful. A few issues introduced by this change that should be fixed before merge:

Bugs / regressions

1. play --artist / --album should not call dig_info just to collect IDs
   You only need song_ids, then queue.add loads them again. Calling dig_info here is unnecessary and slower (and can make the whole command fail when URL resolution fails for the batch). Prefer extracting IDs from the raw API payload (e.g. raw[:limit] → id).

2. Typo in error message
   歌手 {ns.artist} 没有歌曲为空 — should be something like 没有歌曲 or 歌曲为空.

3. play --artist/--album/--songs bypasses _rpc
   Direct send_request after _ensure_daemon breaks existing CLI contracts:

   • --dry-run still runs queue.clear / queue.add (queue is mutated); only the final player.play goes through _rpc and respects dry-run
   • connection errors use daemon_offline instead of the existing daemon_not_running mapping in _rpc
     Please route through _rpc (or equivalent) so dry-run / error types stay consistent with other control commands.

4. README: duplicated / misplaced npx skills add block
   The install command was inserted as its own fenced block before the “安装 Agent Skill 后…” sentence, and the original install block remains — so the section is duplicated and the prose order is wrong.

Design / correctness in the new code

5. No mutual exclusion among play/download sources
   e.g. play --id X --artist Y silently ignores --artist (because any of --id / --playlist / --index takes the old path). Same pattern for download with multiple of --artist / --album / --songs / --playlist. Please reject conflicting flags with invalid_args, or use an argparse mutually exclusive group.

6. download --playlist ignores --limit
--limit is documented and wired for artist/album, but not applied to playlists.

7. download filename / docs mismatch
   Code writes {artist}-{song_name}{ext}; SKILL says <artist> - <song>.mp3. Align code and docs (existing cache naming uses artist - song).

8. All downloads failed still reports success at the top level
   If every track fails, non-JSON still returns EXIT_OK and JSON still emits ok: true (even though per-item ok in data may be false). Prefer a non-zero exit (or at least top-level failure) when ok_count == 0.

Tests

9. No coverage for the new paths
   Please add tests for at least: dry-run on play --songs / --artist (no queue mutation), conflicting flags, and download exit behavior when nothing succeeds.

Happy to re-review after these are addressed.

[yveming]

--fixed and more

PR #987 – Changes Log

Files Modified

File	Change
NEMbox/cli.py	6 fix items (see below)
skills/musicbox/SKILL.md	Fix download filename documentation
tests/test_cli.py	Add 10 new tests; extend FakeNetEase mock

---

Changes to NEMbox/cli.py

1. Mutually exclusive argument groups

• play: --id, --playlist, --index, --artist, --album, --songs placed in an add_mutually_exclusive_group(). argparse natively rejects conflicting flags (previously the last flag silently won).
• download: --artist, --album, --songs, --playlist placed in add_mutually_exclusive_group(required=True). At least one source must be supplied.

2. Remove unnecessary dig_info calls in play --artist/--album

Both branches previously called api.dig_info(raw, "songs") just to extract song_id. Now they read s["id"] directly from the raw API payload:

• artist raw payload (hotSongs[]) → [s["id"] for s in raw[:limit] if s.get("id")]
• album raw payload (songs[]) → [s["id"] for s in raw if s.get("id")]

This avoids URL-resolution failures inside dig_info that could take down the whole command.

3. Fix error message typo

f"歌手 {ns.artist} 没有歌曲为空" → f"歌手 {ns.artist} 没有歌曲"

4. --limit only applies to --artist to keep consistance with play --playlist

• play: removed if ns.limit is not None: raw = raw[:ns.limit] from the --album and --songs branches.
• download: removed if limit is not None: raw = raw[:limit] from the --album branch.
• Updated --limit help text to "获取数量上限（仅对 --artist 生效）".
• Updated epilog examples to match.

5. Fix play --artist/--album/--songs dry-run and error-type consistency

The old code bypassed _rpc, calling send_request directly after _ensure_daemon. This broke:

• dry-run: queue.clear and queue.add ran even with --dry-run; only the final player.play was intercepted.
• error type: ConnectionError was mapped to "daemon_offline" instead of the standard "daemon_not_running" used by every other control command via _rpc.

New flow (after collecting ids):

1. Check --dry-run → emit a JSON preview of all three steps (queue.clear, queue.add, player.play) and return EXIT_OK with no side effects.
2. _ensure_daemon → return proper exit code if daemon cannot start.
3. For each of queue.clear and queue.add: send_request with ConnectionError mapped to "daemon_not_running", and error responses mapped through _ERROR_EXIT_CODES.
4. Final player.play via _rpc (consistent formatting and single-step dry-run behavior for that call).

6. Download all-fail exit code

When every track fails (ok_count == 0), the function now returns EXIT_GENERIC with a "download_failed" error type, instead of EXIT_OK.

---

Changes to skills/musicbox/SKILL.md

Updated download filename documentation:

• Before: 每首歌曲下载为 <artist> - <song>.mp3
• After: 每首歌曲下载为 <artist>-<song>{ext}，扩展名取决于音源（.mp3 或 .flac）

Removed hardcoded .mp3 extension and spaces around the dash to match the actual code.

---

Bug Fix: If the sound device is not avaliable, musicbox play --id/--playlist start the daemon, add songs to the queue, musicbox queue list shows all the queued songs, but musicbox status says the queue empty.

File: NEMbox/player.py (line ~676 in run_mpg123)

Root cause

When mpg123 exits immediately (e.g. no audio device available), run_mpg123's while loop breaks immediately because process.poll() returns non-None. The post-loop decision logic then takes the else: self.next() path:

Flag	Value
playing_flag	True (set by replay())
copyright_issue_flag	False (no @E frame received)
refresh_url_flag	False
frame_cnt	0 (no @F frame received)

self.next() calls next_idx(), which increments self.info["idx"] by 1 for each failed song. Eventually idx reaches len(self.list), making is_index_valid False. Then replay() sees the invalid index and stops. When status is queried, current_song returns {} because is_index_valid is False, and _format_status shows (空队列) — even though queue list correctly shows the songs.

run_mpv does not have this bug because its post-loop logic calls _advance_on_playback_failure() when process.returncode != 0, which handles the failure without advancing the index.

Fix

Added a frame_cnt == 0 check before self.next():

elif frame_cnt == 0:
    self.stop()
    self.playing_flag = False

frame_cnt is only incremented inside the @F branch (actual audio frames received). If frame_cnt is still 0 when the loop exits, the backend never started playing — so self.next() should not be called. Instead, the player stops cleanly with playing_flag = False and the index remains valid, allowing status to report the correct state.

---

Changes to tests/test_cli.py

Extended FakeNetEase mock

• Added artists(artist_id) — returns 2 songs with id/name/etc.
• Added album(album_id) — returns 1 song.
• Improved dig_info(data, "songs") — now iterates over all input items instead of returning a hardcoded single song.
• Added NoUrlNetEase(FakeNetEase) — returns songs with empty mp3_url for all-fail tests.
• Added FakeResponse — mock HTTP response for download tests.
• Fixed 3 ruff UP032 lint warnings (.format() → f-strings).

New tests (10 total)

Test	What it verifies
test_play_dry_run_artist_no_side_effect	play --artist 6452 --dry-run --json → dry-run preview, no daemon calls, no queue mutation
test_play_dry_run_album_no_side_effect	play --album 32311 --dry-run --json → same
test_play_dry_run_songs_no_side_effect	play --songs 33894312 1847408145 --dry-run --json → same
test_play_conflicting_flags_rejected	play --id X --artist Y --json → exit 2, argparse error on stderr
test_download_artist_success	download --artist 6452 --json → exit 0, 2 songs downloaded
test_download_album_success	download --album 32311 --json → exit 0
test_download_songs_success	download --songs 33894312 --json → exit 0
test_download_playlist_success	download --playlist 12345 --json → exit 0
test_download_conflicting_flags_rejected	download --artist X --album Y --json → exit 2
test_download_all_fail_exit_nonzero	All songs have no mp3_url → exit != 0, JSON ok: false with "download_failed" error type

---

Verification Results

Unit tests

$ uv run pytest -q --tb=short tests/test_cli.py
..........................................
=> 42 passed (32 original + 10 new)

$ uv run pytest -q --tb=short (full suite, excluding pre-existing daemon test)
........................................................................
...............                                                          
=> all passed (except test_acquire_lock_is_exclusive, pre-existing)

Lint (ruff)

$ uv run ruff check
Found 3 errors.
=> All 3 are pre-existing (daemon.py x2, player.py x1); no new violations.

Syntax validation

Confirmed via ast.parse() — cli.py is valid Python.

---

[darknessomi]

Re-reviewed at 975915f. Previous review items look addressed:

#	Item	Status
1	play --artist/--album no longer uses dig_info for IDs	Fixed
2	Typo 没有歌曲为空	Fixed
3	dry-run / daemon_not_running for batch play	Fixed
4	README npx skills duplication	Fixed
5	Mutually exclusive play/download sources	Fixed
6	--limit scope	Resolved (documented as artist-only)
7	download filename vs SKILL	Fixed
8	all-fail → non-zero / download_failed	Fixed
9	Tests for dry-run / conflicts / download	Fixed

The extra run_mpg123 change (frame_cnt == 0 → stop without next()) looks correct for the “backend never started / no audio device” case. Note that stop() returns early when the process has already exited, so the explicit self.playing_flag = False is necessary — good.

Nits (non-blocking):

• Latest commit message is | File | Change | (changelog pasted as subject). Please rewrite / squash before merge.
• download all-fail path drops the per-item results array from the JSON error payload; fine for exit status, but attaching them under error/data would help agents.
• SKILL still says download “结果输出 JSON 数组”; it is wrapped in the usual {ok, data} envelope.

LGTM otherwise.