From 55bb9f33c89232f0bcc621b30eccaaa07b4383f0 Mon Sep 17 00:00:00 2001 From: "cmeans-claude-dev[bot]" <3223881+cmeans-claude-dev[bot]@users.noreply.github.com> Date: Tue, 14 Apr 2026 21:50:33 -0500 Subject: [PATCH] Route task-failure error codes through error_from_code (F18, closes #30) Copy/move and delete background-task completion previously emitted a bare DSM_ERROR envelope when status carried an error dict, diverging from the synchronous paths that use synology_error_response / error_from_code. Both paths now route the DSM code through error_from_code(code, "SYNO.FileStation.CopyMove" | ".Delete") and use the mapped exception's error_code, retryable, and suggestion (falling back to the previous generic suggestion for unmapped codes). Behavior change: callers catching dsm_error on these two paths now receive the more specific envelope (e.g. not_found, already_exists, disk_full, filestation_error, permission_denied). Co-Authored-By: Claude Opus 4.6 (1M context) --- CHANGELOG.md | 1 + .../modules/filestation/operations.py | 28 +++- tests/modules/filestation/test_operations.py | 156 +++++++++++++++++- 3 files changed, 174 insertions(+), 11 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 59fbd2f..c120710 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,7 @@ ### Changed +- **Task-completion error envelopes now route through `error_from_code()` for specific codes** (#30) — addresses F18 from the PR #9 self-audit. When a background CopyMove or Delete task completes with an `error` dict, `operations.py:256` (copy/move) and `operations.py:365` (delete) now pass the DSM error code through `error_from_code(err_code, "SYNO.FileStation.CopyMove" | "SYNO.FileStation.Delete")` to build the envelope, matching the synchronous error paths elsewhere in the module. Envelope `code` becomes the mapped value (e.g. `408` → `not_found`, `414` → `already_exists`, `416` → `disk_full`, `1100` → `filestation_error`, `105` → `permission_denied`); envelope `retryable` inherits from the mapped exception (e.g. `disk_full` is `retryable=true`); envelope `suggestion` comes from the per-code mapping when one exists and falls back to the previous generic suggestion otherwise. Unknown/unmapped codes still produce the `dsm_error` envelope. **Behavior change** — callers previously catching `dsm_error` on these two paths will now receive the more specific code. Tests `test_copy_task_completes_with_error` and `test_delete_task_completes_with_error` updated to assert `filestation_error` on code 1100 (plus the per-code suggestion text); four new cases cover 408→not_found, 416→disk_full+retryable, 105→permission_denied, and the unknown-code fallback. - **Structured error envelopes now include `param`/`value` on five more call sites** (#29) — addresses F17 from the PR #9 self-audit. Five `error_response()` call sites in `modules/filestation/*.py` previously emitted envelopes without the `param`/`value` fields that smart clients could dispatch on: `metadata.py:76` (`get_file_info` multi-path empty-result → `param="paths"`, `value=`), `metadata.py:222` (`get_dir_size` timeout → `param="timeout"`, `value=`), `operations.py:245` (copy/move timeout), `operations.py:353` (delete timeout), and `transfer.py:209` (download local-write `OSError` → `param="dest_folder"`, `value=`). Existing `message` text is unchanged. Regression assertions added to `test_empty_files_list_returns_not_found`, `test_dir_size_timeout`, `test_copy_timeout`, `test_delete_timeout`, and `test_download_write_permission_error`. ### Added diff --git a/src/mcp_synology/modules/filestation/operations.py b/src/mcp_synology/modules/filestation/operations.py index 6826a52..6dbf892 100644 --- a/src/mcp_synology/modules/filestation/operations.py +++ b/src/mcp_synology/modules/filestation/operations.py @@ -6,7 +6,7 @@ import logging from typing import TYPE_CHECKING, Any -from mcp_synology.core.errors import ErrorCode, SynologyError +from mcp_synology.core.errors import ErrorCode, SynologyError, error_from_code from mcp_synology.core.formatting import ( error_response, format_size, @@ -254,11 +254,18 @@ async def _copy_move( err = status["error"] err_code = err.get("code", 0) if isinstance(err, dict) else err err_path = status.get("path", "") + # Route err_code through error_from_code so callers see a specific + # envelope (e.g. not_found, disk_full) matching the synchronous error + # paths in this module. Unknown codes fall back to DSM_ERROR. + mapped = error_from_code(err_code, "SYNO.FileStation.CopyMove") error_response( - ErrorCode.DSM_ERROR, + mapped.error_code, f"{operation} files failed: DSM error code {err_code} on path: {err_path}", - retryable=False, - suggestion="Check that source paths exist and you have permission to access them.", + retryable=mapped.retryable, + suggestion=( + mapped.suggestion + or "Check that source paths exist and you have permission to access them." + ), ) # Build response @@ -362,11 +369,18 @@ async def delete_files( err = status["error"] err_code = err.get("code", 0) if isinstance(err, dict) else err err_path = status.get("path", "") + # Route err_code through error_from_code so callers see a specific + # envelope (e.g. permission_denied, not_found) matching the synchronous + # error paths in this module. Unknown codes fall back to DSM_ERROR. + mapped = error_from_code(err_code, "SYNO.FileStation.Delete") error_response( - ErrorCode.DSM_ERROR, + mapped.error_code, f"Delete files failed: DSM error code {err_code} on path: {err_path}", - retryable=False, - suggestion="Check that paths exist and you have permission to delete them.", + retryable=mapped.retryable, + suggestion=( + mapped.suggestion + or "Check that paths exist and you have permission to delete them." + ), ) # Determine recycle bin status per share diff --git a/tests/modules/filestation/test_operations.py b/tests/modules/filestation/test_operations.py index 56d768e..fcdaa65 100644 --- a/tests/modules/filestation/test_operations.py +++ b/tests/modules/filestation/test_operations.py @@ -258,7 +258,13 @@ def side_effect(request: httpx.Request) -> httpx.Response: @respx.mock async def test_copy_task_completes_with_error(self, mock_client: DsmClient) -> None: - """Copy task finishes but status has an ``error`` key → dsm_error.""" + """Copy task finishes with FileStation code 1100 → filestation_error. + + Asserts the envelope is routed through ``error_from_code`` so callers + see the specific envelope code (``filestation_error``) and the per-code + suggestion from FILESTATION_ERROR_CODES instead of the old generic + ``dsm_error`` fallback. + """ def side_effect(request: httpx.Request) -> httpx.Response: params = dict(request.url.params) @@ -288,9 +294,118 @@ def side_effect(request: httpx.Request) -> httpx.Response: dest_folder="/video/Archive", ) body = json.loads(str(exc_info.value)) - assert body["error"]["code"] == "dsm_error" + assert body["error"]["code"] == "filestation_error" assert "1100" in body["error"]["message"] assert "/video/restricted/file.mkv" in body["error"]["message"] + # Suggestion now comes from the per-code mapping, not the generic fallback. + assert "shared folder" in body["error"]["suggestion"].lower() + + @respx.mock + async def test_copy_task_error_maps_408_to_not_found(self, mock_client: DsmClient) -> None: + """Copy task error 408 → ``not_found`` envelope (not ``dsm_error``).""" + + def side_effect(request: httpx.Request) -> httpx.Response: + params = dict(request.url.params) + method = params.get("method", "") + if method == "start": + return httpx.Response(200, json={"success": True, "data": {"taskid": "cm-408"}}) + if method == "status": + return httpx.Response( + 200, + json={ + "success": True, + "data": { + "finished": True, + "error": {"code": 408}, + "path": "/video/missing/file.mkv", + }, + }, + ) + return httpx.Response(200, json={"success": True, "data": {}}) + + respx.get(f"{BASE_URL}/webapi/entry.cgi").mock(side_effect=side_effect) + + with pytest.raises(ToolError) as exc_info: + await copy_files( + mock_client, + paths=["/video/missing/file.mkv"], + dest_folder="/video/Archive", + ) + body = json.loads(str(exc_info.value)) + assert body["error"]["code"] == "not_found" + + @respx.mock + async def test_copy_task_error_maps_416_to_disk_full_retryable( + self, mock_client: DsmClient + ) -> None: + """Copy task error 416 → ``disk_full`` envelope, retryable=True.""" + + def side_effect(request: httpx.Request) -> httpx.Response: + params = dict(request.url.params) + method = params.get("method", "") + if method == "start": + return httpx.Response(200, json={"success": True, "data": {"taskid": "cm-416"}}) + if method == "status": + return httpx.Response( + 200, + json={ + "success": True, + "data": { + "finished": True, + "error": {"code": 416}, + "path": "/video/big/file.mkv", + }, + }, + ) + return httpx.Response(200, json={"success": True, "data": {}}) + + respx.get(f"{BASE_URL}/webapi/entry.cgi").mock(side_effect=side_effect) + + with pytest.raises(ToolError) as exc_info: + await copy_files( + mock_client, + paths=["/video/big/file.mkv"], + dest_folder="/video/Archive", + ) + body = json.loads(str(exc_info.value)) + assert body["error"]["code"] == "disk_full" + assert body["error"]["retryable"] is True + + @respx.mock + async def test_copy_task_error_unknown_code_falls_back(self, mock_client: DsmClient) -> None: + """Unknown/unmapped error code still yields ``dsm_error`` + generic suggestion.""" + + def side_effect(request: httpx.Request) -> httpx.Response: + params = dict(request.url.params) + method = params.get("method", "") + if method == "start": + return httpx.Response(200, json={"success": True, "data": {"taskid": "cm-999"}}) + if method == "status": + return httpx.Response( + 200, + json={ + "success": True, + "data": { + "finished": True, + "error": {"code": 9999}, + "path": "/video/file.mkv", + }, + }, + ) + return httpx.Response(200, json={"success": True, "data": {}}) + + respx.get(f"{BASE_URL}/webapi/entry.cgi").mock(side_effect=side_effect) + + with pytest.raises(ToolError) as exc_info: + await copy_files( + mock_client, + paths=["/video/file.mkv"], + dest_folder="/video/Archive", + ) + body = json.loads(str(exc_info.value)) + assert body["error"]["code"] == "dsm_error" + assert "9999" in body["error"]["message"] + assert "source paths exist" in body["error"]["suggestion"] @respx.mock async def test_copy_poll_error_mid_operation(self, mock_client: DsmClient) -> None: @@ -375,7 +490,7 @@ def side_effect(request: httpx.Request) -> httpx.Response: @respx.mock async def test_delete_task_completes_with_error(self, mock_client: DsmClient) -> None: - """Delete task finishes with an error dict → dsm_error.""" + """Delete task error 1100 → ``filestation_error`` via error_from_code.""" def side_effect(request: httpx.Request) -> httpx.Response: params = dict(request.url.params) @@ -401,9 +516,42 @@ def side_effect(request: httpx.Request) -> httpx.Response: with pytest.raises(ToolError) as exc_info: await delete_files(mock_client, paths=["/video/locked/file.mkv"]) body = json.loads(str(exc_info.value)) - assert body["error"]["code"] == "dsm_error" + assert body["error"]["code"] == "filestation_error" assert "1100" in body["error"]["message"] + @respx.mock + async def test_delete_task_error_maps_105_to_permission_denied( + self, mock_client: DsmClient + ) -> None: + """Delete task error 105 (common) → ``permission_denied`` envelope.""" + + def side_effect(request: httpx.Request) -> httpx.Response: + params = dict(request.url.params) + method = params.get("method", "") + if method == "start": + return httpx.Response(200, json={"success": True, "data": {"taskid": "del-105"}}) + if method == "status": + return httpx.Response( + 200, + json={ + "success": True, + "data": { + "finished": True, + "error": {"code": 105}, + "path": "/video/locked/file.mkv", + }, + }, + ) + return httpx.Response(200, json={"success": True, "data": {}}) + + respx.get(f"{BASE_URL}/webapi/entry.cgi").mock(side_effect=side_effect) + + with pytest.raises(ToolError) as exc_info: + await delete_files(mock_client, paths=["/video/locked/file.mkv"]) + body = json.loads(str(exc_info.value)) + assert body["error"]["code"] == "permission_denied" + assert "105" in body["error"]["message"] + class TestRestoreFromRecycleBin: @respx.mock