diff --git a/packages/synology-apm-cli/src/synology_apm/cli/_validate.py b/packages/synology-apm-cli/src/synology_apm/cli/_validate.py index 0801b63..72ae35e 100644 --- a/packages/synology-apm-cli/src/synology_apm/cli/_validate.py +++ b/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( diff --git a/packages/synology-apm-cli/src/synology_apm/cli/commands/m365_export.py b/packages/synology-apm-cli/src/synology_apm/cli/commands/m365_export.py index baaedb6..d02ee06 100644 --- a/packages/synology-apm-cli/src/synology_apm/cli/commands/m365_export.py +++ b/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 diff --git a/packages/synology-apm-mcp/src/synology_apm/mcp/tools/plans/_builders_machine.py b/packages/synology-apm-mcp/src/synology_apm/mcp/tools/plans/_builders_machine.py index 2e6b88e..1feb5f7 100644 --- a/packages/synology-apm-mcp/src/synology_apm/mcp/tools/plans/_builders_machine.py +++ b/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), diff --git a/packages/synology-apm-sdk/src/synology_apm/sdk/collections/machine.py b/packages/synology-apm-sdk/src/synology_apm/sdk/collections/machine.py index 53ce4f8..0a2b7c4 100644 --- a/packages/synology-apm-sdk/src/synology_apm/sdk/collections/machine.py +++ b/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 diff --git a/packages/synology-apm-sdk/src/synology_apm/sdk/models/backup_server.py b/packages/synology-apm-sdk/src/synology_apm/sdk/models/backup_server.py index 1eb5738..fa6ebd1 100644 --- a/packages/synology-apm-sdk/src/synology_apm/sdk/models/backup_server.py +++ b/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) diff --git a/tests/unit/cli/commands/test_m365_exports.py b/tests/unit/cli/commands/test_m365_exports.py index e53fb31..30fc31e 100644 --- a/tests/unit/cli/commands/test_m365_exports.py +++ b/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" diff --git a/tests/unit/cli/test_helpers.py b/tests/unit/cli/test_helpers.py index f8b26a7..115ba2f 100644 --- a/tests/unit/cli/test_helpers.py +++ b/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 ─────────────────────────────── diff --git a/tests/unit/mcp/tools/test_plan_builders.py b/tests/unit/mcp/tools/test_plan_builders.py index b6fea3b..4fdb82c 100644 --- a/tests/unit/mcp/tools/test_plan_builders.py +++ b/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 diff --git a/tests/unit/sdk/collections/test_machine_file_server.py b/tests/unit/sdk/collections/test_machine_file_server.py index d2a022f..dd8899f 100644 --- a/tests/unit/sdk/collections/test_machine_file_server.py +++ b/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 diff --git a/tests/unit/sdk/models/test_models.py b/tests/unit/sdk/models/test_models.py index 1b4ef5a..3f8e526 100644 --- a/tests/unit/sdk/models/test_models.py +++ b/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