Skip to content

feat(downloadstation): Phase 2 — 7 WRITE tools + DS error codes#105

Merged
cmeans-claude-dev[bot] merged 14 commits into
mainfrom
feat/downloadstation-phase2-write
May 13, 2026
Merged

feat(downloadstation): Phase 2 — 7 WRITE tools + DS error codes#105
cmeans-claude-dev[bot] merged 14 commits into
mainfrom
feat/downloadstation-phase2-write

Conversation

@cmeans-claude-dev
Copy link
Copy Markdown
Contributor

@cmeans-claude-dev cmeans-claude-dev Bot commented May 13, 2026

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

Test plan

  • `uv run pytest` — 727 tests pass at 96.26% coverage
  • `uv run mypy src/` — strict mode clean
  • `uv run ruff check src/ tests/` — clean
  • `uv run ruff format --check src/ tests/` — clean
  • 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

cmeans-claude-dev Bot and others added 12 commits May 13, 2026 18:08
@cmeans-claude-dev
feat(core): add Download Station error code map
133c56f 
DS overloads the 400-series with different meanings than FileStation
(e.g., DS 400 = file upload failed; FS 400 = invalid parameter). New
core/downloadstation_errors.py carries DOWNLOADSTATION_ERROR_CODES
plus a DownloadStationError exception subclass. error_from_code()
dispatches to the DS map for any SYNO.DownloadStation.* API.

Foundation for Phase 2 of the Download Station module — Phase 1
(READ tools) does not exercise this path because the read APIs use
100-series codes only.
@cmeans-claude-dev
test(downloadstation-errors): tighten fallback + add 105/106 dispatch…
8b4b9d5 
… guards

Code review caught two test gaps:

- test_unknown_ds_code_falls_back_to_dsm_error was asserting only "exc
  is not None" — error_from_code never returns None, so this test
  couldn't fail. Tightened to verify the fallback returns a generic
  SynologyError and crucially NOT a DownloadStationError, so a future
  refactor that accidentally promotes unknown codes to DS errors fails
  loudly.

- Added test_ds_api_105_routes_to_permission_error_not_session_expired
  and test_ds_api_106_routes_to_session_expired. Code 105 is the
  CLAUDE.md-mandated "permission denied — never re-auth" invariant;
  without this test a future early-return inside the DS dispatch branch
  could silently bypass the 100-series fall-through and break the
  re-auth contract for DS APIs.
@cmeans-claude-dev
feat(downloadstation): Phase 2 MODULE_INFO + register stubs
6ed8367 
Add seven WRITE-tier ToolInfo entries to MODULE_INFO and seven stub
closures in register() pointing at NotImplementedError handler stubs
in tasks.py / config.py. Each subsequent task replaces one stub with
a working implementation. Register tests updated to assert all 12
tools (5 READ + 7 WRITE) are present with correct permission tiers.
@cmeans-claude-dev
feat(core): add DsmClient.create_download_task_with_file()
7fffd13 
Multipart POST helper for SYNO.DownloadStation.Task.create v1, modeled
on the existing upload() pattern. Same re-auth-on-session-error
retry semantics (codes 106/107/119 trigger one retry; 105 does NOT
per the CLAUDE.md invariant), same _sid-masked debug logging, same
ApiNotFoundError guard.

Will be called by create_download in the downloadstation module when
the caller supplies a torrent_file_path.
@cmeans-claude-dev @claude
feat(downloadstation): implement create_download
616b4a3 
Replace the create_download stub with a working implementation that
branches between URI (standard GET) and torrent/NZB file (multipart
POST via create_download_task_with_file). Validates mutual-exclusion
of inputs, file existence, and surfaces DSM errors as ToolError.
Add 8 TestCreateDownload tests covering both paths and error cases.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@cmeans-claude-dev @claude
feat(downloadstation): implement delete_download
39f441f 
DSM v1 Task.delete has no documented "remove task, keep files" mode —
it always removes both. delete_data is exposed as a REQUIRED explicit
parameter so the caller acknowledges the destructive side effect:
True proceeds with the call; False refuses with a clear ToolError
explaining why the safer mode isn't available.

Also fixes DsmClient.request() debug logging to handle list-typed data
responses (Task.delete returns a list, not a dict).

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@cmeans-claude-dev
feat(downloadstation): implement pause_download + resume_download
96b6e92 
Siblings sharing a private _task_state_change helper. Regression-guard
tests verify each tool calls the right DSM method (pause vs resume) —
the shared helper would silently swap them if the wrong method string
were threaded through.
@cmeans-claude-dev @claude
feat(downloadstation): implement edit_download (destination only)
43d2ab7 
DSM Task.edit v1 supports multiple fields depending on DSM version;
the spec calls out priority and max-speed as candidates that need
per-NAS verification before exposing. Phase 2 ships destination only
— additional fields are explicit follow-up work once verified against
a live NAS.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@cmeans-claude-dev @claude
feat(downloadstation): implement set_download_config
3ae189e 
Replace the Task 8 stub with the real implementation of
set_download_config: partial-update semantics (only supplied fields
sent to DSM), no-op guard (raises ToolError when nothing supplied),
DSM error propagation, and regression test coverage for API method
name and parameter encoding.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@cmeans-claude-dev
feat(downloadstation): implement set_schedule with 168-char plan vali…
97579af 
…dation

schedule_plan length is validated client-side (reusing SCHEDULE_PLAN_LENGTH
from helpers.py — same invariant format_schedule_grid enforces on the read
side) so a bad plan surfaces a clear local error before the round-trip.
@cmeans-claude-dev
test(downloadstation): lift Phase 2 coverage to 100% on every module …
71d0554 
…file

Following the same pattern as Phase 1's coverage lift — once each WRITE
tool's stub is replaced with a real handler, the corresponding register()
closure body needs an invocation test to walk the closure source lines
in __init__.py, and the partial-update branches need targeted tests:

- 7 new TestDownloadstationPhase2ToolInvocation tests mirror Phase 1's
  invocation pattern, covering tool_create_download / tool_delete_download
  / tool_pause_download / tool_resume_download / tool_edit_download /
  tool_set_download_config / tool_set_schedule.
- 2 set_download_config tests for the emule_max_download/upload branches
  (existing tests only exercised bt + default_destination).
- 1 set_schedule test for emule_enabled branch + 1 for DSM-error catch
  (existing tests covered length-validation and method-name, not the
   per-field non-schedule_plan branches or the SynologyError catch).

Every DS module file now reports 100% line coverage; 727 repo tests pass
at 96.26%.
@cmeans-claude-dev
docs(changelog): add Phase 2 Download Station entry + bump integratio…
365a18e 
…n permission
@cmeans-claude-dev cmeans-claude-dev Bot added the Awaiting CI Dev complete, waiting for CI/Codecov to pass before QA label May 13, 2026
@cmeans-claude-dev
docs(changelog): substitute real PR# 105 for placeholder
5e4195d
@cmeans-claude-dev
docs(changelog): fix ADR-0001 entry PR# clobbered by sed substitution
6ae8da3 
The previous commit ran sed -i 's/#PR_PLACEHOLDER/#105/' globally, which
substituted both the Phase 2 entry (correct) AND the ADR-0001 entry
(wrong — that one actually shipped via #100). Restoring #100 on the
ADR line.

The ADR-0001 entry has had #PR_PLACEHOLDER on main since #100 itself
merged — the original PR forgot the post-open substitution. This
commit also closes that latent drift while fixing the regression I
just introduced.
@github-actions github-actions Bot added Ready for QA Dev work complete — QA can begin review and removed Awaiting CI Dev complete, waiting for CI/Codecov to pass before QA labels May 13, 2026
@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented May 13, 2026

Codecov Report

❌ Patch coverage is 92.98246% with 16 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
src/mcp_synology/modules/downloadstation/tasks.py 89.15% 9 Missing ⚠️
src/mcp_synology/core/client.py 86.00% 7 Missing ⚠️

📢 Thoughts on this report? Let us know!

@cmeans cmeans added the QA Active QA is actively reviewing; Dev should not push changes label May 13, 2026
@github-actions github-actions Bot removed the Ready for QA Dev work complete — QA can begin review label May 13, 2026
cmeans
cmeans reviewed May 13, 2026
View reviewed changes
Copy link
Copy Markdown
Owner

@cmeans cmeans left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

QA Round 1 — Pass

Head: 6ae8da3 (rebased onto post-#104 main; merge-base 872e873).

Local verification at 6ae8da3 (post uv sync --frozen --extra dev):

  • uv run pytest: 727 passed / 112 deselected / 17 warnings / 96.26% coverage — exact match against PR body claim.
  • All new module files at 100% line coverage (tasks.py, config.py, __init__.py, helpers.py, core/downloadstation_errors.py, core/client.py).
  • ruff check, ruff format --check, mypy src/ — clean.
  • CI on 6ae8da3: 12 required SUCCESS / 3 SKIPPED incl. vdsm integration tests SUCCESS.

Code review notes

Destructive-op safeguards (the key thing on a WRITE-tier PR):

  • delete_download: delete_data: bool is positional-required at the tool level (no default). Refuses delete_data=False with an explicit message because DSM v1 Task.delete has no "keep files" mode — turning a misleading-looking safety toggle into a deliberate acknowledgment of the destructive default. Empty task_ids list rejected up front.
  • edit_download: refuses an empty edit (no destination supplied) with valid=["destination"] so the future-field expansion path is self-documenting.
  • set_download_config / set_schedule: partial-update semantics with explicit empty-field-set rejection; set_schedule validates len(schedule_plan) == SCHEDULE_PLAN_LENGTH client-side so the bad input never reaches DSM.

Error code map (core/downloadstation_errors.py):

  • DS 400–408 each carry a short message + actionable suggestion tuple.
  • error_from_code() dispatches to the DS map via deferred local import (documented inline to avoid the obvious circular-import question).
  • "DownloadStation" in api_name substring match handles both SYNO.DownloadStation.* and future SYNO.DownloadStation2.* (Phase 2 of #103).
  • Explicit regression tests for DS 105 → SynologyPermissionError (NOT session-expired) and DS 106 → SessionExpiredError, guarding the CLAUDE.md "never re-auth on 105" invariant for DS calls — same shape as the existing Auth/FileStation guards.
  • Unknown DS code falls through to generic SynologyError, NOT to DownloadStationError — verified by test_unknown_ds_code_falls_back_to_generic_synology_error.

Multipart helper (DsmClient.create_download_task_with_file):

  • Mirrors upload_file() faithfully: _sid as query param (not form field); _sid + password masked in debug logs; file re-opened on retry (closure pattern); single retry on session errors via _re_auth_callback.
  • _open_file / try/finally fh.close() keeps the file descriptor lifetime tight — finally clause runs after httpx.post returns/raises.
  • Pins to DS Task.create v1 (the only documented multipart path).
  • 3 test cases: happy path returns task_id, DS 400 raises DownloadStationError, session 106 triggers exactly one re-auth retry then succeeds with the new SID.

Debug-logging bug fix in DsmClient.request():

  • body["data"] was assumed dict and called .keys() unconditionally. Task.delete returns a list and crashed the debug log call.
  • Now isinstance(data, dict) branches the log line — list form logs item count. Non-functional path (debug only) but the crash would surface as a KeyError/AttributeError on log emission, which is worse than the operation's actual result. Bonus catch.

CHANGELOG (## Unreleased / ### Added):

  • Phase 2 entry slotted above the existing #104 entry (Added-section newest-first ordering).
  • Bonus catch: ADR-0001 entry previously read #PR_PLACEHOLDER (clobbered by an earlier sed substitution) — corrected to #100 in the same PR. Verified #100 is the right PR# (sed-substitution drift documented in commit 6ae8da3).
  • Counts in entry — 62 new tests; every new module file at 100% line coverage; 727 repo tests pass at 96.26% — match actuals exactly (lesson from PR #104's R1 clearly stuck).

Operator opt-in (tests/integration_config.yaml.example):

  • downloadstation.permission bumped read → write with a comment listing the Phase 2 tool surface that needs it. Stays enabled: false by default.

Deferrals:

  • DS2 negotiate_version(max_version=2) for Task.list / Task.getinfo and DS2.Task.get fallback — both still tracked at #103, carried forward unchanged from Phase 1.
  • Task.edit field expansion beyond destination — flagged in PR body as "follow-up for a future PR" pending live-NAS field verification.
  • CLAUDE.md "Version pinning" section update — defer per Phase 1 framing until post-#103.

Test-plan checkbox status

  • Boxes 1–5 (auto + 100% coverage attestation): ticked pre-QA, every claim re-verified at 6ae8da3.
  • Boxes 6–11 (manual smokes against a real DS-installed NAS — magnet URI create, .torrent file create, pause/resume/delete, delete_data=False refusal, set_download_config round-trip, set_schedule round-trip): out of QA scope — no DS-installed NAS available to QA today; intended as post-merge install smokes.

Findings: none. Zero blockers, zero substantive, zero observations.

Verdict: QA Pass. Applying Ready for QA Signoff as final act. Awaiting maintainer's QA Approved.

@cmeans cmeans added Ready for QA Signoff QA passed — ready for maintainer final review and merge and removed QA Active QA is actively reviewing; Dev should not push changes labels May 13, 2026
cmeans
cmeans approved these changes May 13, 2026
View reviewed changes
Copy link
Copy Markdown
Owner

@cmeans cmeans left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@cmeans cmeans added QA Approved Manual QA testing completed and passed and removed Ready for QA Signoff QA passed — ready for maintainer final review and merge labels May 13, 2026
@cmeans-claude-dev cmeans-claude-dev Bot merged commit 76be388 into main May 13, 2026
34 checks passed
@cmeans-claude-dev cmeans-claude-dev Bot deleted the feat/downloadstation-phase2-write branch May 13, 2026 23:56
This was referenced May 14, 2026
Open
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cmeans cmeans cmeans approved these changes

Assignees

No one assigned

Labels

QA Approved Manual QA testing completed and passed

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@codecov-commenter @cmeans