fix: harden response parsing and CLI/MCP input handling

packages/synology-apm-cli/src/synology_apm/cli/_validate.py

@@ -318,22 +318,24 @@ def parse_time_filter(value: str) -> datetime:
     """Parse a --since / --until time-filter argument; supports ISO 8601 and relative times (30m, 1h, 24h, 7d)."""
     now = datetime.now(tz=UTC)
     value = value.strip()
-    if value.endswith("h"):
-        return now - timedelta(hours=float(value[:-1]))
-    if value.endswith("d"):
-        return now - timedelta(days=float(value[:-1]))
-    if value.endswith("m"):
-        return now - timedelta(minutes=float(value[:-1]))
+    _bad_format = typer.BadParameter(
+        f"Cannot parse time format: {value!r}. "
+        "Supported: ISO 8601 (e.g. 2026-04-01) or relative (e.g. 30m, 1h, 24h, 7d)."
+    )
+    unit_to_field = {"h": "hours", "d": "days", "m": "minutes"}
+    if value and value[-1] in unit_to_field:
+        try:
+            amount = float(value[:-1])
+        except ValueError:
+            raise _bad_format from None
+        return now - timedelta(**{unit_to_field[value[-1]]: amount})
     try:
         dt = datetime.fromisoformat(value)
         if dt.tzinfo is None:
             dt = dt.replace(tzinfo=UTC)
         return dt
-    except ValueError as exc:
-        raise typer.BadParameter(
-            f"Cannot parse time format: {value!r}. "
-            "Supported: ISO 8601 (e.g. 2026-04-01) or relative (e.g. 30m, 1h, 24h, 7d)."
-        ) from exc
+    except ValueError:
+        raise _bad_format from None
 
 
 def parse_time_range(

packages/synology-apm-cli/src/synology_apm/cli/commands/m365_export.py

@@ -518,9 +518,10 @@ async def _export_download(
                 await _download_with_progress(apm, url, dest_path)
 
         except OSError as exc:
-            if dest_path is not None:
-                with contextlib.suppress(OSError):
-                    await asyncio.to_thread(Path(dest_path).unlink, missing_ok=True)
+            # The SDK stages the download in a .part file and only moves it into
+            # place on success, so an existing file at dest_path is untouched on
+            # failure. Do not unlink dest_path here — that would delete the user's
+            # previous good download.
             err_console.print(f"[red]✗[/red] Download failed: {exc}")
             raise typer.Exit(EXIT_ERROR) from exc
 

packages/synology-apm-mcp/src/synology_apm/mcp/tools/plans/_builders_machine.py

@@ -141,7 +141,12 @@ def _parse_backup_window(enabled: bool, spec: str | None) -> MachineBackupWindow
                 continue
             if "-" in rng:
                 start_s, end_s = rng.split("-", 1)
-                start, end = int(start_s), int(end_s)
+                try:
+                    start, end = int(start_s), int(end_s)
+                except ValueError:
+                    raise ValueError(
+                        f"Backup window hours must be integers 0-23, got {rng!r}."
+                    ) from None
                 if enabled and not (0 <= start <= 23 and 0 <= end <= 23):
                     raise ValueError(f"Backup window hours must be 0-23, got {rng!r}.")
                 if enabled and start > end:
@@ -152,7 +157,12 @@ def _parse_backup_window(enabled: bool, spec: str | None) -> MachineBackupWindow
                     )
                 hours.update(range(start, end + 1))
             else:
-                h = int(rng)
+                try:
+                    h = int(rng)
+                except ValueError:
+                    raise ValueError(
+                        f"Backup window hour must be an integer 0-23, got {rng!r}."
+                    ) from None
                 if enabled and not 0 <= h <= 23:
                     raise ValueError(f"Backup window hour must be 0-23, got {rng!r}.")
                 hours.add(h)
@@ -196,12 +206,18 @@ def _parse_tasks_json(raw: str | None) -> tuple[MachineTaskConfig, ...] | None:
                 )
             schedule = MachineTaskSchedule(time_schedule=time_schedule, event_trigger=event_trigger)
         scope_raw = entry.get("scope")
+        custom_volumes_raw = entry.get("custom_volumes", [])
+        if not isinstance(custom_volumes_raw, list):
+            raise ValueError(
+                "tasks_json entry 'custom_volumes' must be a JSON array of strings "
+                f'(e.g. ["C:", "D:"]), got {custom_volumes_raw!r}.'
+            )
         tasks.append(
             MachineTaskConfig(
                 workload_type=MachineWorkloadType(entry["workload_type"]),
                 os_type=MachineOsType(entry["os_type"]),
                 scope=MachineTaskScope(scope_raw) if scope_raw else None,
-                custom_volumes=tuple(entry.get("custom_volumes", ())),
+                custom_volumes=tuple(custom_volumes_raw),
                 include_external_drives=entry.get("include_external_drives", False),
                 include_boot_partition=entry.get("include_boot_partition", True),
                 use_main_schedule=entry.get("use_main_schedule", True),

packages/synology-apm-sdk/src/synology_apm/sdk/collections/machine.py

@@ -583,10 +583,10 @@ def _parse_workload(raw: dict[str, Any]) -> MachineWorkload:
         if api_type == "FS":
             backup_progress = None
             raw_items = cache.get("processedSuccessCount")
-            items_backed_up = int(raw_items) if raw_items is not None else None
+            items_backed_up = int(raw_items) if raw_items not in (None, "") else None
         else:
             raw_prog = cache.get("progress")
-            backup_progress = int(float(raw_prog)) if raw_prog is not None else 0
+            backup_progress = int(float(raw_prog)) if raw_prog not in (None, "") else 0
             items_backed_up = None
     elif job_status == "WAITING_TASK":
         backup_progress = None

packages/synology-apm-sdk/src/synology_apm/sdk/models/backup_server.py

@@ -89,6 +89,7 @@ def to_dict(self) -> dict[str, Any]:
         return auto_to_dict(
             self,
             extra={
+                "storage_usage_pct": round(self.storage_usage_pct, 1),
                 "backup_data_reduction_bytes": self.backup_data_reduction_bytes,
                 "backup_data_reduction_ratio": (
                     round(self.backup_data_reduction_ratio, 1)

tests/unit/cli/commands/test_m365_exports.py

@@ -652,13 +652,21 @@ async def _fake_download(url: str, dest_path: str, on_progress: Any = None) -> N
     assert result.exit_code == 0, result.output
     assert "Saved to" in result.output
 
-def test_m365_exchange_export_download_oserror_removes_partial_file(tmp_path: Path) -> None:
-    """export download removes the partially written file when the download fails."""
+def test_m365_exchange_export_download_oserror_leaves_part_cleanup_to_sdk(tmp_path: Path) -> None:
+    """On download failure the CLI must not touch the filesystem itself.
+
+    The SDK stages into a .part file and cleans it up on failure (dest is never
+    written), so the CLI's only job is to report the error and exit non-zero. It
+    must not unlink dest_path — doing so would delete a pre-existing good file.
+    """
     mock_apm = make_mock_apm_with_export(group=False)
     mock_apm.m365.exchange_export.get_download_url_by_activity.return_value = "https://apm.example/dl/token"
 
     async def _fail_download(url: str, dest_path: str, on_progress: Any = None) -> None:
-        Path(dest_path).write_bytes(b"partial")
+        # Mirror the real SDK: stage into .part, remove it on failure, never touch dest.
+        part = Path(dest_path + ".part")
+        part.write_bytes(b"partial")
+        part.unlink()
         raise OSError("disk full")
 
     mock_apm.download_file = AsyncMock(side_effect=_fail_download)
@@ -673,3 +681,24 @@ async def _fail_download(url: str, dest_path: str, on_progress: Any = None) -> N
     assert result.exit_code == 1
     assert "Download failed" in result.output
     assert not dest.exists()
+    assert not Path(str(dest) + ".part").exists()
+
+
+def test_m365_exchange_export_download_oserror_preserves_existing_file(tmp_path: Path) -> None:
+    """A failed download must NOT delete a pre-existing file at the destination path."""
+    dest = tmp_path / "out.pst"
+    dest.write_bytes(b"previous good backup")
+    mock_apm = make_mock_apm_with_export(group=False)
+    mock_apm.m365.exchange_export.get_download_url_by_activity.return_value = "https://apm.example/dl/token"
+    mock_apm.download_file.side_effect = OSError("disk full")
+
+    result = invoke_cli(mock_apm, [
+        "m365", "exchange", "export", "download",
+        "--workload-id", WORKLOAD_ID, "--namespace", NAMESPACE,
+        "--id", "act-uuid-001", "--filename", str(dest), "--yes",
+    ])
+
+    assert result.exit_code == 1
+    # The CLI must not have unlinked the user's existing file on failure.
+    assert dest.exists()
+    assert dest.read_bytes() == b"previous good backup"

tests/unit/cli/test_helpers.py

@@ -281,6 +281,11 @@ def test_parse_time_filter_invalid_raises_bad_parameter() -> None:
     with pytest.raises(typer.BadParameter):
         parse_time_filter("not-a-date")
 
+def test_parse_time_filter_malformed_relative_raises_bad_parameter() -> None:
+    """A malformed relative value (e.g. '1h30m') raises BadParameter, not a raw ValueError."""
+    with pytest.raises(typer.BadParameter):
+        parse_time_filter("1h30m")
+
 
 # ── WorkloadRef.resolve_machine / resolve_m365 ───────────────────────────────
 

tests/unit/mcp/tools/test_plan_builders.py

@@ -239,6 +239,17 @@ def test_parse_backup_window_disabled_does_not_validate_range(self):
         assert bw is not None
         assert bw.enabled is False
 
+    def test_parse_backup_window_non_numeric_range_raises_friendly(self):
+        """A non-numeric hour token yields a friendly message, not a raw int() error."""
+        from synology_apm.mcp.tools.plans._builders_machine import _parse_backup_window
+        with pytest.raises(ValueError, match="must be integers 0-23"):
+            _parse_backup_window(True, "mon:a-b")
+
+    def test_parse_backup_window_non_numeric_single_hour_raises_friendly(self):
+        from synology_apm.mcp.tools.plans._builders_machine import _parse_backup_window
+        with pytest.raises(ValueError, match="must be an integer 0-23"):
+            _parse_backup_window(True, "mon:x")
+
     def test_parse_tasks_json_none_returns_none(self):
         from synology_apm.mcp.tools.plans._builders_machine import _parse_tasks_json
         assert _parse_tasks_json(None) is None
@@ -258,6 +269,23 @@ def test_parse_tasks_json_entry_not_a_dict_raises(self):
         with pytest.raises(ValueError, match="must be a JSON object"):
             _parse_tasks_json(json.dumps(["not-a-dict"]))
 
+    def test_parse_tasks_json_custom_volumes_string_raises(self):
+        """A string custom_volumes (e.g. "C:") must raise, not silently char-split into ('C',':')."""
+        from synology_apm.mcp.tools.plans._builders_machine import _parse_tasks_json
+        raw = json.dumps([{"workload_type": "fs", "os_type": "windows", "custom_volumes": "C:"}])
+        with pytest.raises(ValueError, match="custom_volumes.*must be a JSON array"):
+            _parse_tasks_json(raw)
+
+    def test_parse_tasks_json_custom_volumes_list_accepted(self):
+        """A proper list custom_volumes is accepted and preserved."""
+        from synology_apm.mcp.tools.plans._builders_machine import _parse_tasks_json
+        raw = json.dumps(
+            [{"workload_type": "fs", "os_type": "windows", "scope": "custom_volume", "custom_volumes": ["C:", "D:"]}]
+        )
+        tasks = _parse_tasks_json(raw)
+        assert tasks is not None
+        assert tasks[0].custom_volumes == ("C:", "D:")
+
     def test_parse_tasks_json_with_schedule_and_event_trigger(self):
         from synology_apm.mcp.tools.plans._builders_machine import _parse_tasks_json
         from synology_apm.sdk import MachineOsType, MachineWorkloadType, ScheduleFrequency

tests/unit/sdk/collections/test_machine_file_server.py

@@ -448,6 +448,78 @@ async def test_fs_running_reads_items_backed_up_from_cache() -> None:
     assert wl.items_backed_up == 1234
 
 
+async def test_pc_running_empty_progress_parses_as_zero() -> None:
+    """PC RUNNING with an empty-string cache.progress degrades to 0 instead of crashing."""
+    session = make_session()
+    running_workload = {
+        "id": WORKLOAD_ID,
+        "namespace": NAMESPACE,
+        "spec": {"workloadType": "PC", "workloadName": "TestPC", "protectStatus": "PROTECT_STATUS_PROTECTED"},
+        "status": {"lastBackupTime": "0", "usage": "0", "jobStatus": "RUNNING"},
+        "cache": {"progress": ""},
+    }
+    async with aiointercept(mock_external_urls=True) as m:
+        m.get(LOGIN_URL, payload=LOGIN_OK)
+        await session.connect()
+        m.get(LIST_URL, payload={"workloads": [running_workload], "total": 1})
+        collection = MachineWorkloadCollection(session)
+        workloads, total = await collection.list()
+        await session.disconnect()
+
+    from synology_apm.sdk.enums import WorkloadStatus
+    wl = workloads[0]
+    assert wl.status == WorkloadStatus.BACKING_UP
+    assert wl.backup_progress == 0
+
+
+async def test_fs_running_empty_count_parses_as_none() -> None:
+    """FS RUNNING with an empty-string cache.processedSuccessCount degrades to None, not a crash."""
+    session = make_session()
+    running_fs = {
+        "id": "fs-id-001",
+        "namespace": NAMESPACE,
+        "spec": {"workloadType": "FS", "workloadName": "Corp Share", "protectStatus": "PROTECT_STATUS_PROTECTED"},
+        "status": {"lastBackupTime": "0", "usage": "0", "jobStatus": "RUNNING"},
+        "cache": {"processedSuccessCount": ""},
+    }
+    async with aiointercept(mock_external_urls=True) as m:
+        m.get(LOGIN_URL, payload=LOGIN_OK)
+        await session.connect()
+        m.get(LIST_URL, payload={"workloads": [running_fs], "total": 1})
+        collection = MachineWorkloadCollection(session)
+        workloads, total = await collection.list()
+        await session.disconnect()
+
+    from synology_apm.sdk.enums import WorkloadStatus
+    wl = workloads[0]
+    assert wl.status == WorkloadStatus.BACKING_UP
+    assert wl.items_backed_up is None
+
+
+async def test_fs_running_zero_count_preserved_as_zero() -> None:
+    """A legitimate integer 0 processedSuccessCount stays 0, not collapsed to None."""
+    session = make_session()
+    running_fs = {
+        "id": "fs-id-001",
+        "namespace": NAMESPACE,
+        "spec": {"workloadType": "FS", "workloadName": "Corp Share", "protectStatus": "PROTECT_STATUS_PROTECTED"},
+        "status": {"lastBackupTime": "0", "usage": "0", "jobStatus": "RUNNING"},
+        "cache": {"processedSuccessCount": 0},
+    }
+    async with aiointercept(mock_external_urls=True) as m:
+        m.get(LOGIN_URL, payload=LOGIN_OK)
+        await session.connect()
+        m.get(LIST_URL, payload={"workloads": [running_fs], "total": 1})
+        collection = MachineWorkloadCollection(session)
+        workloads, total = await collection.list()
+        await session.disconnect()
+
+    from synology_apm.sdk.enums import WorkloadStatus
+    wl = workloads[0]
+    assert wl.status == WorkloadStatus.BACKING_UP
+    assert wl.items_backed_up == 0
+
+
 async def test_waiting_task_maps_to_queuing_status() -> None:
     """WAITING_TASK job status should map to WorkloadStatus.QUEUING."""
     from unittest.mock import AsyncMock, patch

tests/unit/sdk/models/test_models.py

@@ -369,6 +369,7 @@ def test_fields_include_reduction_stats(self) -> None:
         d = server.to_dict()
         assert d["backup_server_id"] == "srv-001"
         assert d["server_type"] == "dp"
+        assert d["storage_usage_pct"] == 30.0
         assert d["backup_data_reduction_bytes"] == 5_000_000_000
         assert d["backup_data_reduction_ratio"] == 62.5