feat(downloadstation): Phase 1 — module scaffolding + 5 READ tools#104
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
cmeans
left a comment
There was a problem hiding this comment.
QA Round 1 — Failed
Head: d6913fc
Local verification at d6913fc:
uv run pytest: 665 passed / 112 deselected (integration+vdsm markers) / 18 warnings / 96.52% coverage. All newmodules/downloadstation/*.pyfiles at 100%.ruff check,ruff format --check,mypy src/— all clean.- CI (12 required + 5 skipped) all SUCCESS incl.
vdsm integration testsSUCCESS on d6913fc.
Findings
| # | Severity | Finding |
|---|---|---|
| F1 | Substantive | Test-count + coverage drift in CHANGELOG and PR body. CHANGELOG line 7 says "42 new unit tests; total downloadstation module coverage above 95%" — actual is 60 new tests at tests/modules/downloadstation/ (the d6913fc "lift coverage to 100%" commit added 18 after the entry was written) and 100% coverage on every new module file. PR body Summary says "42 unit tests via respx-mocked HTTP"; test-plan box 1 says "647 tests pass at 96% coverage" — actual is 665 / 96.52%. Same drift pattern as PRs #83 and #85 round 1. CHANGELOG drift is the higher-priority half since the line merges into release notes as-is. |
Acceptable resolutions for F1: update CHANGELOG line + PR body Summary + test-plan box 1 to actuals (60, 100%, 665 / 96.52%); or use range/rounded language that won't drift.
Code review notes (non-blocking)
- Scaffolding mirrors
modules/system/(lazyregister()imports, per-toolallowed_toolsgating, alphabetical_MODULE_REGISTRYslot inserver.py). helpers.py:STATUS_GROUPSgrouping is documented (1+8 bucketed with active states); ETA boundary cases (zero/negative speed, downloaded ≥ total seed-after-finish overshoot, day/hour/minute/second bands); schedule-grid renderer validatesSCHEDULE_PLAN_LENGTHexplicitly.tasks.py/stats.py/config.py: structurederror_response/synology_error_responseenvelopes throughout;version=1hard-pin on Task / Statistic / Info / Schedule with inline comments flagging DS Task's comma-separatedadditional=format (vs FileStation v2's JSON-array format) so a future fixer doesn't strip it.- Tests (60 functions): respx-mocked, mirrors filestation layout, covers happy paths + DSM errors + every documented edge case (em-dash for
Nonebools, malformedschedule_planlength, non-stringschedule_plan, emptytasks[]as not-found,status_filterrejection). - Deferrals (
negotiate_version(max_version=2),DS2.Task.getfallback, CLAUDE.md "Version pinning" section update) all tracked in #103 with concrete acceptance criteria. - vdsm intentionally out of scope (virtual-dsm doesn't ship the Download Station package) — factual.
Checkbox status
- Boxes 1–4 (auto checks): functionally pass at d6913fc, but box 1 text is stale (
647 tests pass at 96% coverage). Will re-tick after F1 fix lands with corrected numbers. - Box 5 (manual smoke against real DS NAS): out of QA scope — depends on user having a NAS with DS installed.
- Box 6 (manual smoke on a NAS without DS, preflight skip): testable in principle on vdsm, but the gating is fully unit-tested via
MODULE_INFO.required_apisshape. Happy to spin up vdsm in round 2 if you want belt-and-braces confirmation; otherwise reasonable as a post-merge install smoke.
Verdict: QA Failed — applying label as final act.
Round 1 F1 — fixedHead: b64680b
No code changes — pure prose. CI for Re-ticked the label to |
cmeans
left a comment
There was a problem hiding this comment.
QA Round 2 — Pass
Head: b64680b (round-1 head was d6913fc).
F1 closed. git diff d6913fc..b64680b is one CHANGELOG line — round-1 code review carries forward unchanged. Verified all three edits:
- CHANGELOG line 7:
42 new unit tests; total downloadstation module coverage above 95%→60 new unit tests; every new module file (__init__.py, tasks.py, stats.py, config.py, helpers.py) at 100% line coverage(more precise than my round-1 suggestion). - PR body Summary:
42 unit tests via respx-mocked HTTP→60 unit tests via respx-mocked HTTP, mirrors the FileStation test layout. Every new module file is at 100% line coverage. - PR body test-plan box 1:
647 tests pass at 96% coverage→665 tests pass at 96.52% coverage.
Bonus: the helpers description in PR body's "What's in this PR" now lists format_eta and format_schedule_grid alongside the three originally named — closes a smaller drift I hadn't flagged.
Re-verification at b64680b (post uv sync --frozen --extra dev):
uv run pytest: 665 passed / 112 deselected / 17 warnings / 96.52% coverage — identical to round 1.ruff check,ruff format --check,mypy src/— clean.- CI: 14 SUCCESS / 7 SKIPPED on b64680b incl.
vdsm integration testsSUCCESS.
Test-plan checkboxes: 1–4 stay ticked (text now accurate). Boxes 5 (manual smoke on real DS NAS) and 6 (manual smoke on NAS without DS) remain post-merge install smokes per round-1 framing — unit coverage of MODULE_INFO.required_apis plus the TestDownloadstationModuleRegister contract tests cover the preflight gating without needing a vdsm spin-up.
Findings: none. Zero blockers, zero substantive, zero observations.
Verdict: QA Pass. Applying Ready for QA Signoff as final act. Awaiting maintainer's QA Approved.
Adds the Download Station module package with MODULE_INFO declaring Phase 1's five READ tools, plus an empty register() that wires them via stubbed handlers. Adds "downloadstation" to server._MODULE_REGISTRY. Handler bodies land in subsequent tasks. Refs: docs/superpowers/specs/2026-05-13-downloadstation-module-design.md
…ield Code-quality review flagged enabled_extras as YAGNI — it had no consumer in Phase 1 or any defined consumer in Phase 2/3. Empty settings schema is cleaner and equally forward-compatible; fields land in the task that needs them.
Replaces the list_downloads stub with a working implementation that fetches SYNO.DownloadStation.Task.list (v1), applies client-side status_filter (all/downloading/finished/paused/error), and renders a table with ID, title, type, status, size, progress, speed, and ETA. Also adds two reusable helpers used by later Phase 1 tasks: - format_task_status: maps DSM numeric codes 1–10 to labels - format_transfer_progress: renders "X / Y (Z%)" with clamping Tests: 16 helper tests + 5 list_downloads tests (21 new); 26 total in the downloadstation module. Adds SYNO.DownloadStation.Task to the shared test api_cache so mock_client is usable in DS tests. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- status_filter validation now uses error_response(INVALID_PARAMETER, ...)
to produce the structured error envelope every other validation in the
project uses (precedent: filestation/helpers.py validate_additional).
- Add a comment on the DS Task additional= format (comma-separated, not
the JSON-array format FileStation v2 uses) so a future "fixer" doesn't
break it.
- Add a comment explaining STATUS_GROUPS["downloading"] including
waiting/filehosting_waiting (transient pre-active states bucketed with
in-flight states).
- Tighten test_partial to assert the unit ("512 MB") not just the digit.
- Drop the unreachable "0 tasks" alternative in test_empty_queue;
format_table always returns "No items to display." for empty rows.
Replace the NotImplementedError stub with a full implementation that calls SYNO.DownloadStation.Task getinfo (v1) with all additional groups (detail, transfer, file, tracker, peer) and renders them as distinct sections using shared formatters. Includes three unit tests covering the happy path, DSM 404 error propagation, and the empty-tasks-array not-found path. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Move _format_epoch up with other private _format_* helpers for consistency with the existing module-internal layout. - Tighten test_returns_detail_transfer_blocks to assert the three section headers (Files / Trackers / Peers) so a regression that silently dropped a table would fail loudly.
Replaces the Task 5 stub with the real implementation of get_download_stats, which calls SYNO.DownloadStation.Statistic getinfo and renders download/upload throughput totals. eMule fields are conditionally included when present in the response. Adds three unit tests (total/speeds, eMule present, DSM error propagation) and seeds SYNO.DownloadStation.Statistic into the shared test api_cache. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Replace the Task 6 stub with a real SYNO.DownloadStation.Info getconfig call; render rate limits, destination, and eMule/unzip flags via format_key_value. Add test_config.py with renders-known-fields and DSM-error-propagates tests. Seed SYNO.DownloadStation.Info into make_api_cache() in conftest.py. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Add format_schedule_grid helper that renders the DSM 168-char weekly schedule plan as a 7×24 ASCII grid with per-cell glyphs and a legend. Replace the get_schedule stub with a real implementation that calls SYNO.DownloadStation.Schedule getconfig, renders the enable flags via format_key_value, and delegates grid rendering to format_schedule_grid. Malformed schedule_plan (wrong length or non-string) raises ToolError via error_response(ErrorCode.INVALID_PARAMETER). Add SYNO.DownloadStation.Schedule to the test API cache in conftest. 8 new tests (4 helper, 4 config). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
stats.py and tasks.py each defined a private _speed_str / _format_speed with identical bodies. Move to helpers.py as public format_speed() and import in both. Removes the DRY divergence risk flagged by the final code review.
The shipped Phase 1 had several uncovered gaps that Codecov flagged: the 5 register() tool closures (10 lines), the _bool_str(None) branch in get_download_config, the non-string schedule_plan branch in get_schedule, and the under-1-hour / 1-day branches of _format_eta. - Hoist _format_eta → public format_eta in helpers.py with direct parametrized tests (under-min/under-hour/under-day/day-or-more, plus all em-dash edge cases). The final reviewer flagged the gap in their Phase 1 review. - Add TestFormatSpeed for the format_speed helper (covers zero, negative, and positive paths). - Add TestDownloadstationToolInvocation in test_register.py that mocks each domain handler and invokes the registered tool closure via server._tool_manager._tools[name].fn(...) — mirrors the filestation TestFilestationToolInvocation pattern. - Add TestGetScheduleNonStringPlan covering the schedule_plan-is- not-a-string error path. - Add TestGetDownloadConfigBoolNone covering _bool_str(None) when DSM omits a bool field on older firmware. After: 60 module tests (was 42), all five module files at 100% line coverage, 665 repo tests pass at 96.52% overall.
…, 100%) QA round 1 F1: the Download Station Phase 1 CHANGELOG entry was written before the coverage-lift commit (d6913fc) added 18 more tests and pushed every new module file to 100% line coverage. Update the numbers to match the shipped state (60 tests, 100% on each file) so release notes land accurate.
b64680b to
cfe610a
Compare
Rebased onto mainOld head: b64680b Branch rebased onto main after #102 (python-multipart 0.0.26 → 0.0.27) merged. 15 commits replayed cleanly with no conflicts. The diff vs main is unchanged from b64680b — pure history rewrite. CI re-runs on cfe610a. Local re-verify on cfe610a:
Round-2 QA review can target cfe610a directly. |
cmeans
left a comment
There was a problem hiding this comment.
QA Round 3 — Pass (post-rebase)
Head: cfe610a (round-2 head was b64680b). 14 commits rebased onto post-#102 main.
Rebase delta git diff b64680b..cfe610a is byte-identical to PR #102's contribution:
CHANGELOG.md: +1 lineBump uv group: python-multipart 0.0.26→0.0.27 (#102)slotted above the existing#98 setup-uventry in### Changed.uv.lock: samepython-multipart 0.0.26 → 0.0.27block with the same new sdist + wheel URLs/hashes.
No other files moved. Clean rebase, no conflicts, no merge artifacts. Round 1/2 code review carries forward unchanged.
Re-verification at cfe610a (after uv sync --frozen --extra dev):
uv run pytest: 665 passed / 112 deselected / 17 warnings / 96.52% coverage — identical to rounds 1 and 2.ruff check,ruff format --check,mypy src/— clean.- CI on cfe610a: 12 required SUCCESS / 5 SKIPPED incl.
vdsm integration testsSUCCESS.
mergeStateStatus flipped BLOCKED → CLEAN as expected (round-2 review was invalidated by the new head; this round restores it).
Findings: none. Zero blockers, zero substantive, zero observations.
Verdict: QA Pass. Applying Ready for QA Signoff as final act. Awaiting maintainer's QA Approved.
## Summary Second of three planned PRs from the DS module spec (Phase 1 was #104). Ships the WRITE-tier surface of the Download Station module plus supporting infrastructure. **New tools (7):** - `create_download` — URI list (HTTP/FTP/magnet/etc.) **or** local `.torrent`/`.nzb` via multipart POST - `delete_download` — explicit `delete_data: bool` required; `delete_data=False` is **refused** because DSM v1 `Task.delete` has no documented "keep files" mode - `pause_download` / `resume_download` — siblings sharing a private `_task_state_change` helper - `edit_download` — `destination` only (other DSM `Task.edit` fields vary by DSM version) - `set_download_config` — partial updates only; rates in KB/s, 0 = unlimited - `set_schedule` — 168-char weekly plan, length-validated client-side via the same `SCHEDULE_PLAN_LENGTH` invariant `format_schedule_grid` uses on the read side **New infrastructure:** - `src/mcp_synology/core/downloadstation_errors.py` — DS-specific 400-series error codes (DS 400 = "file upload failed"; FS 400 = "invalid parameter") with `error_from_code()` dispatch to the DS map for `SYNO.DownloadStation.*` APIs - Explicit regression tests for DS 105 → `SynologyPermissionError` and DS 106 → `SessionExpiredError` so the CLAUDE.md "never re-auth on 105" invariant holds for DS calls too - `DsmClient.create_download_task_with_file()` multipart POST helper, mirroring the existing `upload_file()` pattern with full re-auth retry **Bonus fix:** - `DsmClient.request()` debug logging was calling `.keys()` unconditionally on `body["data"]`, which crashed for endpoints (like `Task.delete`) that return a list instead of a dict. Now guarded with `isinstance(data, dict)`. Discovered during `delete_download` integration. Phase 3 (BT search + RSS) and #103 (DS2 `negotiate_version`) remain open for follow-up. ## Key design decisions documented inline - **`delete_download` refuses `delete_data=False`** rather than silently delete files. DSM v1 has no "remove task, keep files" mode; making the parameter explicit-required and refusing the unsafe value turns it from a misleading safety toggle into a deliberate acknowledgement of the destructive default. - **Coverage discipline matched Phase 1's bar**: 7 closure-invocation tests added for the new WRITE register() closures so module coverage stays at 100% on every file. ## Known deferrals - **DS2 \`negotiate_version(max_version=2)\` for `Task.list` and `Task.getinfo`** — tracked at #103 (carried forward from Phase 1). - **\`SYNO.DownloadStation2.Task.get\` fallback** for `get_download_info` — also covered under #103. - **\`Task.edit\` field expansion beyond `destination`** — needs per-NAS verification of which fields DSM actually accepts; follow-up for a future PR. - **CLAUDE.md "Version pinning" section update** — defer until DS pinning rationale (post-#103) is settled. ## Test plan - [x] \`uv run pytest\` — 727 tests pass at 96.26% coverage - [x] \`uv run mypy src/\` — strict mode clean - [x] \`uv run ruff check src/ tests/\` — clean - [x] \`uv run ruff format --check src/ tests/\` — clean - [x] Every new module file at 100% line coverage - [ ] Manual smoke: create download from a magnet URI against a real DS NAS - [ ] Manual smoke: create download from a local .torrent file against a real DS NAS - [ ] Manual smoke: pause / resume / delete (delete_data=true) - [ ] Manual smoke: confirm delete_data=false surfaces the refusal message rather than silently failing - [ ] Manual smoke: set_download_config bandwidth caps + revert via get_download_config - [ ] Manual smoke: set_schedule with a partial plan + revert 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: cmeans-claude-dev[bot] <272174644+cmeans-claude-dev[bot]@users.noreply.github.com> Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
Summary
First of three planned PRs introducing a Synology Download Station module. Phase 1 ships the READ-only surface: scaffolding plus five tools —
list_downloads,get_download_info,get_download_stats,get_download_config,get_schedule.Phase 2 (task CRUD writes + global config writes) and Phase 3 (BT search + RSS) to follow as separate PRs.
What's in this PR
New module
src/mcp_synology/modules/downloadstation/parallel tomodules/filestation/andmodules/system/:__init__.py—MODULE_INFOdeclaring the DS API set + 5 ToolInfo entries (all READ),register()with lazy handler imports and per-toolallowed_toolsgatingtasks.py—list_downloads,get_download_infostats.py—get_download_statsconfig.py—get_download_config,get_schedulehelpers.py— shared formatters:format_task_status,format_transfer_progress,format_speed,format_eta,format_schedule_grid+STATUS_GROUPS/_STATUS_LABELS/SCHEDULE_PLAN_LENGTHconstantsTests at
tests/modules/downloadstation/: 60 unit tests viarespx-mocked HTTP, mirrors the FileStation test layout. Every new module file is at 100% line coverage. EachSYNO.DownloadStation.*API used is registered in the sharedtests/conftest.py:make_api_cache().Server wiring adds
\"downloadstation\"tosrc/mcp_synology/server.py:_MODULE_REGISTRY(alphabetical, between filestation and system).Operator opt-in documented in
tests/integration_config.yaml.example— module ships disabled by default; operators addmodules.downloadstation.enabled: trueto enable it.CHANGELOG.md entry under
## Unreleased / ### Added.Key design properties
MODULE_INFO.required_apispreflight onSYNO.DownloadStation.Task, lazy handler imports insideregister(). The full design (docs/superpowers/specs/2026-05-13-downloadstation-module-design.md, local-only doc) walks through the verification.additional=params use comma-separated format, not the JSON-array format File Station v2 uses. The DS Task API predates the FileStation v2 JSON convention; inline comments intasks.pyflag this so a future "fixer" doesn't break it.error_response(ErrorCode.INVALID_PARAMETER, ...)envelope, matchingfilestation/helpers.py validate_additionalprecedent.list_downloadsdoes client-side status filtering because DSM v1Task.listdoesn't support a server-side filter. Groups:downloading≡ {1,2,4,6,8,9},finished≡ {5,7},paused≡ {3},error≡ {10}.get_scheduleparses the 168-charschedule_planinto a 7×24 ASCII grid with off (.), on (#), throttled (~) glyphs and a legend.Known deferrals
Task.listandTask.getinfo— tracked at Phase 2 (downloadstation): use negotiate_version(max_version=2) for Task.list / Task.getinfo #103. Spec calls for trying v2 first when available; Phase 1 stays on v1 to ship a clean baseline.SYNO.DownloadStation2.Task.getfallback forget_download_info— also covered under Phase 2 (downloadstation): use negotiate_version(max_version=2) for Task.list / Task.getinfo #103.Test plan
uv run pytest— 665 tests pass at 96.52% coverageuv run mypy src/— strict mode cleanuv run ruff check src/ tests/— cleanuv run ruff format --check src/ tests/— cleandownloadstationin a live config, invoke each of the 5 tools against a real NAS with DS installed; spot-check renderingdownloadstationon a NAS without DS installed — confirm the API preflight skips registration with a WARNING log and no tools appear intools/list🤖 Generated with Claude Code