fix: use share paths, drop additional from list_dir, remove debug logging
Root cause: DSM expects share paths (/dev) not volume paths (/volume1/dev). The 408 errors were triggered by any additional field on the wrong path format. - list_shares: use share["path"] directly (e.g. /dev), drop real_path from additional — only volume_status remains - list_dir: remove additional parameter entirely; table now shows name + type (isdir is returned by default); update docstring to show share path examples - client.py: remove diagnostic REQUEST and RAW ERROR stderr logging - tests: update assertions to match share paths and two-column table output Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
This commit is contained in:
@@ -262,11 +262,6 @@ class FileStationClient:
|
|||||||
info = self._api_cache[api]
|
info = self._api_cache[api]
|
||||||
resolved_version = version if version is not None else info["maxVersion"]
|
resolved_version = version if version is not None else info["maxVersion"]
|
||||||
url = f"{self._base_url}/webapi/{info['path']}"
|
url = f"{self._base_url}/webapi/{info['path']}"
|
||||||
sys.stderr.write(
|
|
||||||
f"[dsm] REQUEST {api}/{method} v{resolved_version} "
|
|
||||||
f"(NAS max={info['maxVersion']})\n"
|
|
||||||
)
|
|
||||||
sys.stderr.flush()
|
|
||||||
|
|
||||||
req_params: dict[str, Any] = {
|
req_params: dict[str, Any] = {
|
||||||
"api": api,
|
"api": api,
|
||||||
@@ -300,8 +295,6 @@ class FileStationClient:
|
|||||||
|
|
||||||
code = body.get("error", {}).get("code", 0)
|
code = body.get("error", {}).get("code", 0)
|
||||||
logger.debug("DSM response: %s/%s — error code %d", api, method, code)
|
logger.debug("DSM response: %s/%s — error code %d", api, method, code)
|
||||||
sys.stderr.write(f"[dsm] RAW ERROR {api}/{method}: {body!r}\n")
|
|
||||||
sys.stderr.flush()
|
|
||||||
|
|
||||||
# Transparent re-auth on session errors (one retry only)
|
# Transparent re-auth on session errors (one retry only)
|
||||||
if code in _SESSION_ERROR_CODES and not _is_retry and self._auth_manager:
|
if code in _SESSION_ERROR_CODES and not _is_retry and self._auth_manager:
|
||||||
|
|||||||
@@ -69,7 +69,7 @@ def register_filestation(
|
|||||||
data = await client.request(
|
data = await client.request(
|
||||||
"SYNO.FileStation.List",
|
"SYNO.FileStation.List",
|
||||||
"list_share",
|
"list_share",
|
||||||
params={"additional": json.dumps(["real_path", "volume_status"])},
|
params={"additional": json.dumps(["volume_status"])},
|
||||||
)
|
)
|
||||||
except SynologyError as e:
|
except SynologyError as e:
|
||||||
return f"Error: {e}"
|
return f"Error: {e}"
|
||||||
@@ -82,7 +82,7 @@ def register_filestation(
|
|||||||
rows = []
|
rows = []
|
||||||
for share in shares:
|
for share in shares:
|
||||||
name = share.get("name", "")
|
name = share.get("name", "")
|
||||||
path = share.get("additional", {}).get("real_path") or share.get("path", "")
|
path = share.get("path", f"/{name}")
|
||||||
vol = share.get("additional", {}).get("volume_status", {})
|
vol = share.get("additional", {}).get("volume_status", {})
|
||||||
total = vol.get("totalspace")
|
total = vol.get("totalspace")
|
||||||
used = vol.get("usedspace")
|
used = vol.get("usedspace")
|
||||||
@@ -119,8 +119,11 @@ def register_filestation(
|
|||||||
) -> str:
|
) -> str:
|
||||||
"""List the contents of a directory on the NAS.
|
"""List the contents of a directory on the NAS.
|
||||||
|
|
||||||
|
Use share paths as returned by list_shares (e.g. "/dev", "/data"),
|
||||||
|
not volume paths (e.g. "/volume1/dev" will not work).
|
||||||
|
|
||||||
Args:
|
Args:
|
||||||
path: Absolute path on the NAS (e.g. "/volume1/data").
|
path: Share-relative path on the NAS (e.g. "/dev" or "/data/photos").
|
||||||
offset: Number of items to skip (for pagination).
|
offset: Number of items to skip (for pagination).
|
||||||
limit: Maximum items to return (1-500, default 100).
|
limit: Maximum items to return (1-500, default 100).
|
||||||
sort_by: Sort field — one of: name, size, user, group, mtime, atime,
|
sort_by: Sort field — one of: name, size, user, group, mtime, atime,
|
||||||
@@ -128,8 +131,8 @@ def register_filestation(
|
|||||||
sort_direction: "asc" or "desc".
|
sort_direction: "asc" or "desc".
|
||||||
|
|
||||||
Returns:
|
Returns:
|
||||||
Formatted table with name, type, size, and modification time,
|
Formatted table with name and type, plus the total item count
|
||||||
plus the total item count for pagination context.
|
for pagination context.
|
||||||
"""
|
"""
|
||||||
from mcp_synology_filestation.client import SynologyError
|
from mcp_synology_filestation.client import SynologyError
|
||||||
|
|
||||||
@@ -151,7 +154,6 @@ def register_filestation(
|
|||||||
"limit": limit,
|
"limit": limit,
|
||||||
"sort_by": sort_by,
|
"sort_by": sort_by,
|
||||||
"sort_direction": sort_direction,
|
"sort_direction": sort_direction,
|
||||||
"additional": "size,time", # DEBUG: comma-separated (no json.dumps)
|
|
||||||
},
|
},
|
||||||
)
|
)
|
||||||
except SynologyError as e:
|
except SynologyError as e:
|
||||||
@@ -163,45 +165,22 @@ def register_filestation(
|
|||||||
if not files:
|
if not files:
|
||||||
return f"Directory '{path}' is empty (or does not exist)."
|
return f"Directory '{path}' is empty (or does not exist)."
|
||||||
|
|
||||||
# Build table
|
# Build table — name + type only (no additional fields requested)
|
||||||
rows = []
|
rows = []
|
||||||
for f in files:
|
for f in files:
|
||||||
name = f.get("name", "")
|
name = f.get("name", "")
|
||||||
is_dir = f.get("isdir", False)
|
ftype = "dir" if f.get("isdir", False) else "file"
|
||||||
ftype = "dir" if is_dir else "file"
|
rows.append((name, ftype))
|
||||||
add = f.get("additional", {})
|
|
||||||
size_val = add.get("size")
|
|
||||||
size_str = "-" if is_dir else _fmt_size(size_val)
|
|
||||||
mtime = add.get("time", {}).get("mtime")
|
|
||||||
mtime_str = _fmt_time(mtime)
|
|
||||||
rows.append((name, ftype, size_str, mtime_str))
|
|
||||||
|
|
||||||
w_name = max(len("Name"), *(len(r[0]) for r in rows))
|
w_name = max(len("Name"), *(len(r[0]) for r in rows))
|
||||||
w_type = max(len("Type"), *(len(r[1]) for r in rows))
|
w_type = max(len("Type"), *(len(r[1]) for r in rows))
|
||||||
w_size = max(len("Size"), *(len(r[2]) for r in rows))
|
|
||||||
w_mtime = max(len("Modified"), *(len(r[3]) for r in rows))
|
|
||||||
|
|
||||||
sep = (
|
sep = f"+{'-' * (w_name + 2)}+{'-' * (w_type + 2)}+"
|
||||||
f"+{'-' * (w_name + 2)}"
|
header = f"| {'Name':<{w_name}} | {'Type':<{w_type}} |"
|
||||||
f"+{'-' * (w_type + 2)}"
|
|
||||||
f"+{'-' * (w_size + 2)}"
|
|
||||||
f"+{'-' * (w_mtime + 2)}+"
|
|
||||||
)
|
|
||||||
header = (
|
|
||||||
f"| {'Name':<{w_name}} "
|
|
||||||
f"| {'Type':<{w_type}} "
|
|
||||||
f"| {'Size':<{w_size}} "
|
|
||||||
f"| {'Modified':<{w_mtime}} |"
|
|
||||||
)
|
|
||||||
|
|
||||||
lines = [f"Path: {path}", sep, header, sep]
|
lines = [f"Path: {path}", sep, header, sep]
|
||||||
for name, ftype, size_str, mtime_str in rows:
|
for name, ftype in rows:
|
||||||
lines.append(
|
lines.append(f"| {name:<{w_name}} | {ftype:<{w_type}} |")
|
||||||
f"| {name:<{w_name}} "
|
|
||||||
f"| {ftype:<{w_type}} "
|
|
||||||
f"| {size_str:<{w_size}} "
|
|
||||||
f"| {mtime_str:<{w_mtime}} |"
|
|
||||||
)
|
|
||||||
lines.append(sep)
|
lines.append(sep)
|
||||||
|
|
||||||
end = offset + len(files)
|
end = offset + len(files)
|
||||||
|
|||||||
@@ -79,7 +79,8 @@ async def test_list_shares_success(config: AppConfig) -> None:
|
|||||||
result = await tools["list_shares"]()
|
result = await tools["list_shares"]()
|
||||||
|
|
||||||
assert "data" in result
|
assert "data" in result
|
||||||
assert "/volume1/data" in result
|
assert "/data" in result # share path, not volume path
|
||||||
|
assert "/volume1/data" not in result
|
||||||
assert "photos" in result
|
assert "photos" in result
|
||||||
assert "2 share(s) found" in result
|
assert "2 share(s) found" in result
|
||||||
# Check usage percentage appears
|
# Check usage percentage appears
|
||||||
@@ -120,33 +121,21 @@ async def test_list_shares_dsm_error(config: AppConfig) -> None:
|
|||||||
|
|
||||||
@pytest.mark.asyncio
|
@pytest.mark.asyncio
|
||||||
async def test_list_dir_success(config: AppConfig) -> None:
|
async def test_list_dir_success(config: AppConfig) -> None:
|
||||||
"""list_dir returns a formatted table with files and directories."""
|
"""list_dir returns a formatted table with name and type columns."""
|
||||||
client = MagicMock()
|
client = MagicMock()
|
||||||
client.request = AsyncMock(
|
client.request = AsyncMock(
|
||||||
return_value={
|
return_value={
|
||||||
"total": 3,
|
"total": 3,
|
||||||
"files": [
|
"files": [
|
||||||
{
|
{"name": "documents", "isdir": True},
|
||||||
"name": "documents",
|
{"name": "photo.jpg", "isdir": False},
|
||||||
"isdir": True,
|
{"name": "readme.txt", "isdir": False},
|
||||||
"additional": {"size": None, "time": {"mtime": 1700000000}},
|
|
||||||
},
|
|
||||||
{
|
|
||||||
"name": "photo.jpg",
|
|
||||||
"isdir": False,
|
|
||||||
"additional": {"size": 2_500_000, "time": {"mtime": 1700100000}},
|
|
||||||
},
|
|
||||||
{
|
|
||||||
"name": "readme.txt",
|
|
||||||
"isdir": False,
|
|
||||||
"additional": {"size": 1024, "time": {"mtime": 1700200000}},
|
|
||||||
},
|
|
||||||
],
|
],
|
||||||
}
|
}
|
||||||
)
|
)
|
||||||
|
|
||||||
tools = _make_mcp_and_tools(config, client)
|
tools = _make_mcp_and_tools(config, client)
|
||||||
result = await tools["list_dir"](path="/volume1/data")
|
result = await tools["list_dir"](path="/dev")
|
||||||
|
|
||||||
assert "documents" in result
|
assert "documents" in result
|
||||||
assert "photo.jpg" in result
|
assert "photo.jpg" in result
|
||||||
@@ -164,11 +153,7 @@ async def test_list_dir_pagination(config: AppConfig) -> None:
|
|||||||
return_value={
|
return_value={
|
||||||
"total": 200,
|
"total": 200,
|
||||||
"files": [
|
"files": [
|
||||||
{
|
{"name": f"file{i}.txt", "isdir": False}
|
||||||
"name": f"file{i}.txt",
|
|
||||||
"isdir": False,
|
|
||||||
"additional": {"size": 100, "time": {"mtime": 1700000000 + i}},
|
|
||||||
}
|
|
||||||
for i in range(100)
|
for i in range(100)
|
||||||
],
|
],
|
||||||
}
|
}
|
||||||
|
|||||||
Reference in New Issue
Block a user