Rename package to mcp-synology, add transfer module and icons#1
Conversation
Welcome to Codecov 🎉Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests. ℹ️ You can also turn on project coverage checks and project coverage reporting on Pull Request comment Thanks for integrating Codecov - We've got you covered ☂️ |
|
Adding QA Active — starting code review of rename + transfer module + icons. |
cmeans
left a comment
There was a problem hiding this comment.
QA Review — PR #1: Rename package to mcp-synology, add transfer module and icons
Automated Checks
| Check | Result |
|---|---|
| pytest | ✅ 262/262 pass (matches PR claim) |
| ruff check | ✅ All checks passed |
| ruff format | ✅ 56 files already formatted |
| mypy | ✅ 3 errors — pre-existing keyring stub warnings (same on main) |
| CI | ✅ All 5 checks pass (lint, typecheck, test 3.11/3.12/3.13) |
Code Review
Rename completeness — Thorough. Verified:
- ✅ All
synology_mcp→mcp_synologyimport paths updated across 92 files - ✅ All
synology-mcp→mcp-synologyuser-facing strings (CLI, error messages, keyring service names, config paths, state paths) - ✅
SYNOLOGY_MCP_CONFIGenv var →MCP_SYNOLOGY_CONFIG - ✅
SynologyMCPsession name →MCPSynology - ✅ No stale references in
src/,tests/,docs/, or config files (only legitimate references in migration script and CHANGELOG) - ✅ Old
src/synology_mcp/directory removed; newsrc/mcp_synology/in place - ✅
pyproject.tomlupdated: name, version (0.4.0), license (Apache 2.0), entry points, URLs, wheel packages, artifacts
Transfer module (upload/download) — Well structured:
- ✅
DsmClient.upload()— POST multipart with SID as query param (not form field), session error re-auth with fresh file handle, version pinned to v2 - ✅
DsmClient.download()— streaming GET to disk (64KB chunks), JSON error envelope detection, Content-Length disk space check, partial file cleanup - ✅
transfer.py— pre-flight disk space check (best-effort via getinfo), overwrite protection, partial cleanup on failure, large file timeout warnings - ✅ Upload/Download APIs marked
optional=Truein module requirements — graceful degradation on older DSM - ✅ Progress callbacks wired through to MCP
ctx.report_progress() - ✅ 14 unit tests covering success, file-not-found, exists-no-overwrite, DSM errors, permissions, disk space, progress callbacks, large file warnings
- ✅ 10 integration tests (upload, duplicate, overwrite, custom filename, download round-trip, cleanup)
Workflow changes:
- ✅
publish.ymlsimplified — removed TestPyPI and conditional guards (now tag-only) - ✅ New
test-publish.ymlfor manual TestPyPI dispatch — clean separation of concerns
Migration script — Solid:
- ✅ Dry-run by default,
--applyto execute - ✅ Discovers instances from both config and state dirs
- ✅ Keyring migration: copies (not moves) then optional
--cleanup - ✅ Idempotent — skips already-migrated items
vDSM test framework — Good foundation:
- ✅ Golden image save/restore for fast test startup
- ✅ Playwright-based wizard automation (fully headless)
- ✅ Podman/Docker/KVM socket auto-detection
- ✅ Re-exports standard integration tests — no test duplication
Icons & README:
- ✅ Light/dark SVGs with square viewBox (512x512), PNGs at multiple sizes
- ✅ MCP
Iconobjects with GitHub raw URLs - ✅ README badges, logo, license footer
Findings
0 blockers, 0 substantive, 0 observations.
This is a clean, comprehensive rename with well-tested new features. All automated checks pass, CI is green, test count matches the PR body (262).
QA Audit
Findings: 0 Adding Ready for QA Signoff — all automated checks pass, zero findings on code review. |
cmeans
left a comment
There was a problem hiding this comment.
README doesn't appear to document new Transfer capabilities, nor does it list the new icons in the project structure section...did it include the migration script and usage?
CHANGELOG should include the Transfer implementation, the virtual DSM testing, the migration script etc.
…cons Rename Python package from synology_mcp to mcp_synology to match PyPI project name (mcp-synology). Includes all import updates across source, tests, docs, and config. New features: - File transfer tools (upload_file, download_file) with progress callbacks, disk space preflight, and partial download cleanup - Project icons (light/dark SVGs, PNGs at 16-256px, favicon.ico) exposed via MCP interface - Dedicated test-publish.yml workflow for TestPyPI - Migration script for existing users (scripts/migrate-from-synology-mcp.py) - Virtual DSM test framework (tests/vdsm/) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Adds centered light/dark logo using GitHub's picture element and standard badges: PyPI version, Python versions, license, CI status, codecov coverage, and download count. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- README: update File Station from 12 to 14 tools, add upload_file and download_file to tier listings - CHANGELOG 0.4.0: add Features section with transfer tools, icons, TestPyPI workflow, and vDSM test framework Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
c626000 to
e75ca58
Compare
cmeans
left a comment
There was a problem hiding this comment.
QA Review — PR #1: Rename package to mcp-synology, add transfer module and icons
Scope Reviewed
All 6 commits, ~90 changed files covering: package rename (synology_mcp → mcp_synology), file transfer tools (upload_file, download_file), project icons, TestPyPI workflow, migration script, virtual DSM test framework, README/CHANGELOG/docs updates, LICENSE change (MIT → Apache 2.0).
CI Status
All green: lint ✅, typecheck ✅, test 3.11 ✅, test 3.12 ✅, test 3.13 ✅
Code Review Verified
- Package rename — zero stale
synology_mcporsynology-mcpreferences insrc/ortests/. All imports, configs, pyproject.toml, CLAUDE.md paths updated. Oldsrc/synology_mcp/directory deleted. - Transfer module —
transfer.pyis well-structured: proper error handling with typed exceptions, partial file cleanup on failure, disk space preflight check, progress callbacks, large file timeout warnings. 19 unit tests covering success, errors, overwrite, custom filenames, permission errors, partial cleanup, disk space, and progress. - Client upload/download —
client.pyupload uses POST multipart (correctly — the one case where POST is mandatory). Download uses streaming GET with Content-Length disk space check and JSON error envelope detection. Re-auth retry on session errors preserved for both. - Migration script — safe: dry-run by default, discovers instances from both old/new locations, keyring copy (not move) with optional cleanup. Idempotent.
- LICENSE — MIT → Apache 2.0, documented as breaking change. pyproject.toml license field and classifier match.
- Workflows — ci.yml rename (--cov=mcp_synology) correct. publish.yml simplified to tag-only. test-publish.yml clean and correct. Workflow file deletions are from the revert commit (see Finding #4).
- README — 14 tools (7 READ + 7 WRITE) matches MODULE_INFO. Badges, logo, footer all reference correct repo name.
- CHANGELOG — 0.4.0 section properly documents all breaking changes and new features. Uses standard Keep a Changelog categories.
- Test count — PR body says 262. Counted: 309 total test functions minus 47 integration tests (excluded by addopts) = 262. Matches.
Findings
1. SUBSTANTIVE — CLAUDE.md has stale tool count (12 → 14)
Three places still say 12 tools / 6 READ + 6 WRITE:
- Line 9:
File Station (12 tools)→ should be 14 - Line 16:
File Station (12 tools: 6 READ + 6 WRITE)→ 14 tools: 7 READ + 7 WRITE - Line 28:
all 12 File Station tools→ 14
CLAUDE.md is the primary dev guide — stale counts will confuse future development.
2. SUBSTANTIVE — docs/specs/project-scaffolding-spec.md has stale tool count
Lines 55 and 422 reference "12 tools" — should be 14.
3. OBSERVATION — Dual icon directories with divergent SVG designs
Two icon sets exist:
assets/icons/— newer design (54-line SVGs with video transport panel section, PNGs at 16-256px)src/mcp_synology/icons/— older design (42-line SVGs without transport panel, PNGs at 16-256px + 48px)
server.py uses GitHub raw URLs pointing to assets/icons/. The src/mcp_synology/icons/ are included in the wheel (pyproject.toml artifacts) but never referenced by code — ~110KB of dead weight in the published package.
Additionally, naming is inconsistent: assets/ PNGs use mcp-synology-dark-*.png while src/ PNGs use mcp-synology-logo-dark-*.png.
Not blocking merge, but should be cleaned up.
4. OBSERVATION — Merge order dependency for workflow files
The revert commit (e75ca58) deletes .github/workflows/pr-labels.yml, pr-labels-ci.yml, and qa-gate.yml — the same files PR #2 adds. After PR #2 merges to main, this PR should be rebased so the diff no longer shows those deletions. Without a rebase, a three-way merge should still preserve the files (net no-op from base), but a rebase would make the diff clean and verifiable.
5. NIT — Silent exception in transfer.py:135
except Exception: # noqa: BLE001
pass # Best-effort — download will catch disk space issues via Content-LengthThe fallback logic is sound (client.download() has its own disk space check), but a logger.debug("Pre-flight getinfo failed: %s", e) would be better than silent swallowing per project conventions.
6. NIT — setup-uv version inconsistency
ci.yml uses astral-sh/setup-uv@v4, new test-publish.yml uses @v5. Pre-existing on ci.yml side but introduces inconsistency with the new workflow.
Test Plan Status
- All 262 unit/module tests pass — verified via CI (lint, typecheck, test 3.11/3.12/3.13)
- Lint (ruff check) passes — verified via CI
- Format (ruff format) passes — verified via CI (ruff check includes format)
- Type check (mypy) passes — verified via CI
- Verify TestPyPI workflow via manual dispatch after merge — post-merge
- Integration test against real NAS with new package name — post-merge
- Rename GitHub repo
synology-mcp→mcp-synologyafter merge — post-merge
Result
2 substantive findings (stale tool counts in CLAUDE.md and scaffolding spec) that should be fixed before merge. 4 observations/nits that can be addressed in a follow-up.
|
Adding QA Failed — 2 substantive findings require fixes before merge:
Both are quick fixes. 4 additional observations/nits noted in the review for optional follow-up. |
cmeans
left a comment
There was a problem hiding this comment.
QA Re-Review — Round 2
All 4 findings from Round 1 addressed in commit f189859:
| Finding | Status |
|---|---|
| CLAUDE.md stale tool count (12→14) | Fixed — lines 9, 16, 28 now say 14 tools / 7 READ + 7 WRITE |
| project-scaffolding-spec.md stale count | Fixed — lines 55, 422 updated to 14 |
| Dead icon directory (src/mcp_synology/icons/) | Fixed — directory completely removed |
| Silent exception in transfer.py:135 | Fixed — now logs debug message |
| setup-uv version inconsistency | Fixed — all workflows now use @v5 |
Remaining nit (non-blocking): pyproject.toml lines 64-65 still reference src/mcp_synology/icons/*.svg and *.png in wheel artifacts, but the directory no longer exists. Glob matches nothing — no build error, just dead config. Can clean up in a follow-up.
CI: All green (lint, typecheck, test 3.11/3.12/3.13).
Result: Pass — zero blocking findings. Ready for signoff.
|
Adding Ready for QA Signoff — all Round 1 findings resolved in f189859. Zero blocking findings on re-review. CI green. One non-blocking nit (dead pyproject.toml icon globs) noted for follow-up. |
cmeans
left a comment
There was a problem hiding this comment.
QA Re-Review — Round 3
Two new commits since Round 2:
| Commit | Change | Verified |
|---|---|---|
| 2d07ae1 | Move icons from assets/icons/ to src/mcp_synology/icons/ | ✅ server.py, README, pyproject.toml all updated to match |
| 7c06cb8 | Remove accidentally committed test-data/awareness.db | ✅ File gone, no sensitive data in repo |
Fixes the remaining nit from Round 2: pyproject.toml icon globs now match real files (no longer dead config).
Single icon directory: All icons (SVGs, PNGs, favicon) now live in src/mcp_synology/icons/ — packaged in wheel AND referenced by server.py/README via GitHub raw URLs. Clean.
CI: All green (lint, typecheck, test 3.11/3.12/3.13).
Findings: 0
Result: Pass — zero findings across all rounds. Ready for signoff.
|
Adding Ready for QA Signoff — Round 3 re-review complete. Icon consolidation and awareness.db cleanup verified. Zero findings. CI green. |
Replace old icon paths with new logo design (isometric cube with drive bays, Docker containers, BitTorrent swarm, and media controls). SVGs in light/dark variants, PNGs at 16-256px, and multi-size favicon. Move icons from src/mcp_synology/icons/ to assets/icons/. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…version - Update File Station tool count from 12 to 14 in CLAUDE.md and project-scaffolding-spec.md (upload_file + download_file were added) - Remove old src/mcp_synology/icons/ directory (dead weight, replaced by assets/icons/) - Add debug logging to pre-flight getinfo exception in transfer.py - Bump setup-uv from v4 to v5 in ci.yml for consistency with other workflows Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Icons belong under the package so they're included in the wheel. Update all GitHub raw URL references in README and server.py. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
7c06cb8 to
6feb0b1
Compare
Two related problems addressed together per Option C in #12: 1. Not idempotent ----------------- The old github-release step ran ``gh release create`` unconditionally. If a Release for the tag already existed (e.g. pre-created with hand-written notes before the workflow ran), the step failed with HTTP 422 and the entire workflow was marked failed — even though the PyPI publish job had already succeeded. This is exactly what happened on the v0.5.0 publish (run 24263840766). The job now checks for an existing Release with ``gh release view`` and uses ``gh release edit`` to update it in place instead of failing. Create flow handles the no-existing-release case. Net: the job is a no-op in content terms when the release already exists with the same notes, and a silent content update when it exists with different notes. 2. Low-quality auto-generated notes ----------------------------------- The old step used ``--generate-notes``, which produces a plain commit list. That's significantly worse than hand-written CHANGELOG entries and created the incentive to pre-create releases manually, which in turn triggered problem #1. The new step reads the Release body from CHANGELOG.md directly. A small awk block extracts the section between the current ``## <ver>`` heading (skipped to avoid duplicating the title) and the next ``## `` heading. CHANGELOG.md is already the authoritative release narrative, so there's no duplication and no quality tradeoff. If the CHANGELOG has no matching entry (e.g. an emergency tag without doc prep), the step falls back to ``--generate-notes`` rather than creating an empty release body. A ::warning:: annotation surfaces the fallback in the workflow UI. Verification performed locally on CHANGELOG.md at HEAD: - awk extraction for v0.5.0 returns 34 lines of content starting with ``### Changed``, ending at the next ``## `` header. - Extraction for a nonexistent version produces an empty file, triggering the fallback path. - ``python3 -c 'import yaml; yaml.safe_load(...)'`` confirms the workflow file parses cleanly. The fix cannot be fully end-to-end tested without a tag push, so the next real release (or a manual workflow re-dispatch against an existing tag) will be the first full exercise of both the create and edit paths. The failure mode if something is wrong is loud (workflow red) and the PyPI publish step is independent and already succeeded before this step runs, so there's no risk to the actual package distribution. Closes #12 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Two related problems addressed together per Option C in #12: 1. Not idempotent ----------------- The old github-release step ran ``gh release create`` unconditionally. If a Release for the tag already existed (e.g. pre-created with hand-written notes before the workflow ran), the step failed with HTTP 422 and the entire workflow was marked failed — even though the PyPI publish job had already succeeded. This is exactly what happened on the v0.5.0 publish (run 24263840766). The job now checks for an existing Release with ``gh release view`` and uses ``gh release edit`` to update it in place instead of failing. Create flow handles the no-existing-release case. Net: the job is a no-op in content terms when the release already exists with the same notes, and a silent content update when it exists with different notes. 2. Low-quality auto-generated notes ----------------------------------- The old step used ``--generate-notes``, which produces a plain commit list. That's significantly worse than hand-written CHANGELOG entries and created the incentive to pre-create releases manually, which in turn triggered problem #1. The new step reads the Release body from CHANGELOG.md directly. A small awk block extracts the section between the current ``## <ver>`` heading (skipped to avoid duplicating the title) and the next ``## `` heading. CHANGELOG.md is already the authoritative release narrative, so there's no duplication and no quality tradeoff. If the CHANGELOG has no matching entry (e.g. an emergency tag without doc prep), the step falls back to ``--generate-notes`` rather than creating an empty release body. A ::warning:: annotation surfaces the fallback in the workflow UI. Verification performed locally on CHANGELOG.md at HEAD: - awk extraction for v0.5.0 returns 34 lines of content starting with ``### Changed``, ending at the next ``## `` header. - Extraction for a nonexistent version produces an empty file, triggering the fallback path. - ``python3 -c 'import yaml; yaml.safe_load(...)'`` confirms the workflow file parses cleanly. The fix cannot be fully end-to-end tested without a tag push, so the next real release (or a manual workflow re-dispatch against an existing tag) will be the first full exercise of both the create and edit paths. The failure mode if something is wrong is loud (workflow red) and the PyPI publish step is independent and already succeeded before this step runs, so there's no risk to the actual package distribution. Closes #12 Co-authored-by: cmeans-claude-dev[bot] <3223881+cmeans-claude-dev[bot]@users.noreply.github.com> Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Summary
synology_mcp→mcp_synologyto match PyPI project name (mcp-synology). All imports, configs, docs, and tests updated.upload_fileanddownload_filewith progress callbacks, disk space preflight checks, partial download cleanup, and large file warnings. 14 new tests.favicon.ico). Three themed faces: drive bays, Docker containers, BitTorrent swarm + media controls. Exposed via MCPiconsparameter using GitHub raw URLs.test-publish.yml(manual dispatch). Removed TestPyPI frompublish.ymlso each workflow has a single concern.scripts/migrate-from-synology-mcp.pyfor existing users.tests/vdsm/for container-based integration testing.Test plan
synology-mcp→mcp-synologyafter merge🤖 Generated with Claude Code