fix: restore additional=["size","time"] to list_dir, update SPEC + tests
Root cause of DSM 408 was wrong path format (volume path vs share path), not the additional parameter. Confirmed via test_additional.py that json.dumps(["size","time"]) is the correct format; comma-separated string is silently ignored by DSM. - list_dir: restore 4-column table (Name, Type, Size, Modified) - list_dir: use additional=json.dumps(["size","time"]) (confirmed working) - SPEC.md: document share path requirement, additional format rules, note SYNO.FileStation.Stat unavailability, remove comma-sep gotcha - tests: restore size/mtime mock data and assertions - delete test_additional.py (throwaway diagnostic script) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
This commit is contained in:
@@ -84,9 +84,12 @@ List all shared folders visible to the authenticated user.
|
||||
|
||||
**DSM call:** `SYNO.FileStation.List::list_share`
|
||||
```
|
||||
additional=["real_path","volume_status"]
|
||||
additional=["volume_status"]
|
||||
```
|
||||
|
||||
> **Note:** DSM returns share paths directly (e.g. `/dev`, `/data`). The `path` field from the
|
||||
> response is used as-is — do not prepend the volume prefix.
|
||||
|
||||
---
|
||||
|
||||
#### `list_dir`
|
||||
@@ -95,7 +98,7 @@ List contents of a directory with optional pagination and sorting.
|
||||
**Parameters:**
|
||||
| Name | Type | Required | Default | Description |
|
||||
|------|------|----------|---------|-------------|
|
||||
| `path` | str | yes | — | Absolute path on the NAS |
|
||||
| `path` | str | yes | — | Share-relative path (e.g. `/dev`, `/data/photos`) |
|
||||
| `offset` | int | no | 0 | Number of items to skip |
|
||||
| `limit` | int | no | 100 | Max items to return (max 500) |
|
||||
| `sort_by` | str | no | `name` | `name`, `size`, `user`, `group`, `mtime`, `atime`, `crtime`, `posix`, `type` |
|
||||
@@ -108,9 +111,21 @@ pagination context.
|
||||
```
|
||||
folder_path={path}, offset={offset}, limit={limit},
|
||||
sort_by={sort_by}, sort_direction={sort_direction},
|
||||
additional=["real_path","size","time","perm","type"]
|
||||
additional=["size","time"]
|
||||
```
|
||||
|
||||
> **Path requirement:** `folder_path` must be a share path as returned by `list_share`
|
||||
> (e.g. `/dev`, `/data`). Volume paths (`/volume1/dev`) are **not accepted** and cause DSM
|
||||
> error 408.
|
||||
>
|
||||
> **`additional` format:** Must be a JSON array serialised as a string
|
||||
> (`json.dumps(["size","time"])` → `'["size", "time"]'`). A comma-separated string
|
||||
> (`"size,time"`) is silently ignored by DSM — the `additional` field will be absent from
|
||||
> every file entry.
|
||||
>
|
||||
> **`SYNO.FileStation.Stat`:** Not available on all NAS firmware versions. Do not use for
|
||||
> `get_info`; fall back to `SYNO.FileStation.List::list` with `additional=["size","time","perm","owner"]`.
|
||||
|
||||
---
|
||||
|
||||
#### `get_info`
|
||||
|
||||
@@ -154,6 +154,7 @@ def register_filestation(
|
||||
"limit": limit,
|
||||
"sort_by": sort_by,
|
||||
"sort_direction": sort_direction,
|
||||
"additional": json.dumps(["size", "time"]),
|
||||
},
|
||||
)
|
||||
except SynologyError as e:
|
||||
@@ -165,22 +166,43 @@ def register_filestation(
|
||||
if not files:
|
||||
return f"Directory '{path}' is empty (or does not exist)."
|
||||
|
||||
# Build table — name + type only (no additional fields requested)
|
||||
# Build table
|
||||
rows = []
|
||||
for f in files:
|
||||
name = f.get("name", "")
|
||||
ftype = "dir" if f.get("isdir", False) else "file"
|
||||
rows.append((name, ftype))
|
||||
is_dir = f.get("isdir", False)
|
||||
ftype = "dir" if is_dir else "file"
|
||||
add = f.get("additional", {})
|
||||
size_str = "-" if is_dir else _fmt_size(add.get("size"))
|
||||
mtime_str = _fmt_time(add.get("time", {}).get("mtime"))
|
||||
rows.append((name, ftype, size_str, mtime_str))
|
||||
|
||||
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_size = max(len("Size"), *(len(r[2]) for r in rows))
|
||||
w_mtime = max(len("Modified"), *(len(r[3]) for r in rows))
|
||||
|
||||
sep = f"+{'-' * (w_name + 2)}+{'-' * (w_type + 2)}+"
|
||||
header = f"| {'Name':<{w_name}} | {'Type':<{w_type}} |"
|
||||
sep = (
|
||||
f"+{'-' * (w_name + 2)}"
|
||||
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]
|
||||
for name, ftype in rows:
|
||||
lines.append(f"| {name:<{w_name}} | {ftype:<{w_type}} |")
|
||||
for name, ftype, size_str, mtime_str in rows:
|
||||
lines.append(
|
||||
f"| {name:<{w_name}} "
|
||||
f"| {ftype:<{w_type}} "
|
||||
f"| {size_str:<{w_size}} "
|
||||
f"| {mtime_str:<{w_mtime}} |"
|
||||
)
|
||||
lines.append(sep)
|
||||
|
||||
end = offset + len(files)
|
||||
|
||||
@@ -1,134 +0,0 @@
|
||||
"""Throwaway script: test SYNO.FileStation.List::list additional field formats.
|
||||
|
||||
Run with:
|
||||
uv run python test_additional.py
|
||||
|
||||
Reads config from the default location and credentials from the OS keyring,
|
||||
exactly as the MCP server does. Tests three additional variants against a
|
||||
real share path and prints the raw DSM response for each.
|
||||
"""
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
import asyncio
|
||||
import json
|
||||
import sys
|
||||
|
||||
import httpx # noqa: F401 — used at runtime for the HTTP client reference
|
||||
|
||||
# ── config ────────────────────────────────────────────────────────────────
|
||||
|
||||
# Change this to any share that exists on your NAS (as returned by list_shares)
|
||||
TEST_PATH = "/dev"
|
||||
|
||||
# ──────────────────────────────────────────────────────────────────────────
|
||||
|
||||
|
||||
async def run() -> None:
|
||||
from mcp_synology_filestation.auth import AuthManager
|
||||
from mcp_synology_filestation.client import FileStationClient
|
||||
from mcp_synology_filestation.config import load_config
|
||||
|
||||
config = load_config()
|
||||
auth = AuthManager(config)
|
||||
|
||||
async with FileStationClient(
|
||||
config.base_url,
|
||||
config.connection.verify_ssl,
|
||||
config.connection.timeout,
|
||||
) as client:
|
||||
await client.query_api_info()
|
||||
sid = await auth.login(client)
|
||||
client.sid = sid
|
||||
|
||||
info = client._api_cache.get("SYNO.FileStation.List", {})
|
||||
print(
|
||||
f"SYNO.FileStation.List — path={info.get('path')!r} "
|
||||
f"v{info.get('minVersion')}-v{info.get('maxVersion')}\n"
|
||||
)
|
||||
|
||||
variants: list[tuple[str, dict]] = [
|
||||
(
|
||||
"1 — no additional",
|
||||
{
|
||||
"folder_path": TEST_PATH,
|
||||
"offset": 0,
|
||||
"limit": 5,
|
||||
"sort_by": "name",
|
||||
"sort_direction": "asc",
|
||||
},
|
||||
),
|
||||
(
|
||||
'2 — additional="size,time" (comma-separated string)',
|
||||
{
|
||||
"folder_path": TEST_PATH,
|
||||
"offset": 0,
|
||||
"limit": 5,
|
||||
"sort_by": "name",
|
||||
"sort_direction": "asc",
|
||||
"additional": "size,time",
|
||||
},
|
||||
),
|
||||
(
|
||||
'3 — additional=json.dumps(["size","time"]) (JSON array)',
|
||||
{
|
||||
"folder_path": TEST_PATH,
|
||||
"offset": 0,
|
||||
"limit": 5,
|
||||
"sort_by": "name",
|
||||
"sort_direction": "asc",
|
||||
"additional": json.dumps(["size", "time"]),
|
||||
},
|
||||
),
|
||||
]
|
||||
|
||||
for label, params in variants:
|
||||
print(f"{'─' * 60}")
|
||||
print(f"Variant {label}")
|
||||
print(f" params: {params}")
|
||||
try:
|
||||
# Call the low-level HTTP layer directly so we see the raw body,
|
||||
# bypassing any error-mapping in client.request()
|
||||
api = "SYNO.FileStation.List"
|
||||
method = "list"
|
||||
api_info = client._api_cache[api]
|
||||
version = api_info["maxVersion"]
|
||||
url = f"{client._base_url}/webapi/{api_info['path']}"
|
||||
|
||||
req_params: dict = {
|
||||
"api": api,
|
||||
"version": str(version),
|
||||
"method": method,
|
||||
"_sid": client.sid,
|
||||
}
|
||||
req_params.update(params)
|
||||
|
||||
http = client._http
|
||||
resp = await http.get(url, params=req_params)
|
||||
body = resp.json()
|
||||
print(f" HTTP {resp.status_code}")
|
||||
print(f" success: {body.get('success')}")
|
||||
if body.get("success"):
|
||||
data = body.get("data", {})
|
||||
files = data.get("files", [])
|
||||
print(f" total: {data.get('total')} returned: {len(files)}")
|
||||
if files:
|
||||
first = files[0]
|
||||
print(f" first file keys: {list(first.keys())}")
|
||||
add = first.get("additional")
|
||||
print(f" first file additional: {add!r}")
|
||||
else:
|
||||
print(f" RAW error body: {body!r}")
|
||||
except Exception as exc:
|
||||
print(f" EXCEPTION: {exc!r}")
|
||||
print()
|
||||
|
||||
await auth.logout(client)
|
||||
|
||||
|
||||
if __name__ == "__main__":
|
||||
try:
|
||||
asyncio.run(run())
|
||||
except Exception as e:
|
||||
print(f"Fatal: {e}", file=sys.stderr)
|
||||
sys.exit(1)
|
||||
@@ -79,7 +79,7 @@ async def test_list_shares_success(config: AppConfig) -> None:
|
||||
result = await tools["list_shares"]()
|
||||
|
||||
assert "data" in result
|
||||
assert "/data" in result # share path, not volume path
|
||||
assert "/data" in result # share path, not volume path
|
||||
assert "/volume1/data" not in result
|
||||
assert "photos" in result
|
||||
assert "2 share(s) found" in result
|
||||
@@ -121,15 +121,27 @@ async def test_list_shares_dsm_error(config: AppConfig) -> None:
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_list_dir_success(config: AppConfig) -> None:
|
||||
"""list_dir returns a formatted table with name and type columns."""
|
||||
"""list_dir returns a formatted table with name, type, size, and modified columns."""
|
||||
client = MagicMock()
|
||||
client.request = AsyncMock(
|
||||
return_value={
|
||||
"total": 3,
|
||||
"files": [
|
||||
{"name": "documents", "isdir": True},
|
||||
{"name": "photo.jpg", "isdir": False},
|
||||
{"name": "readme.txt", "isdir": False},
|
||||
{
|
||||
"name": "documents",
|
||||
"isdir": True,
|
||||
"additional": {"size": 0, "time": {"mtime": 1700000000}},
|
||||
},
|
||||
{
|
||||
"name": "photo.jpg",
|
||||
"isdir": False,
|
||||
"additional": {"size": 2_048_000, "time": {"mtime": 1710000000}},
|
||||
},
|
||||
{
|
||||
"name": "readme.txt",
|
||||
"isdir": False,
|
||||
"additional": {"size": 512, "time": {"mtime": 1720000000}},
|
||||
},
|
||||
],
|
||||
}
|
||||
)
|
||||
@@ -142,6 +154,11 @@ async def test_list_dir_success(config: AppConfig) -> None:
|
||||
assert "readme.txt" in result
|
||||
assert "dir" in result
|
||||
assert "file" in result
|
||||
# Size column: dirs show "-", files show human-readable size
|
||||
assert "2 MB" in result or "1 MB" in result # photo.jpg ~2 MB
|
||||
assert "512 B" in result # readme.txt
|
||||
# Modified column present
|
||||
assert "Modified" in result
|
||||
assert "Showing 1–3 of 3 item(s)" in result
|
||||
|
||||
|
||||
@@ -153,7 +170,11 @@ async def test_list_dir_pagination(config: AppConfig) -> None:
|
||||
return_value={
|
||||
"total": 200,
|
||||
"files": [
|
||||
{"name": f"file{i}.txt", "isdir": False}
|
||||
{
|
||||
"name": f"file{i}.txt",
|
||||
"isdir": False,
|
||||
"additional": {"size": 100, "time": {"mtime": 1700000000}},
|
||||
}
|
||||
for i in range(100)
|
||||
],
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user