synology: code-review hardening + trim over-budget description
Addresses 5 verified findings from /code-review high: - SynoError carries DSM code + handled flag; call() no longer logs eagerly. Top-level handler logs only genuine unhandled failures, so the handled FileStation denial + VPN-down connect errors stop polluting errorlog.md (was a CLAUDE.md rule violation: don't log handled conditions). - FileStation-denial detection is numeric (code in 400/407), not substring. - SSH hint now also fires on the generic `call` path, not just `ls`. - `services` falls back get->list on 103 for older DSM builds (multi-device). - BrokenPipe flush moved inside try so small piped output can't leak a traceback. - Trim SKILL description 755->515 chars (was the longest of 32 skills; self-check registry-budget WARN). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -87,7 +87,13 @@ AUTH_ERRORS = {
|
||||
|
||||
|
||||
class SynoError(Exception):
|
||||
pass
|
||||
# code = the DSM error code (when known) so callers compare numerically, not by
|
||||
# substring on the message. handled = an expected/converted condition (e.g. the
|
||||
# FileStation file-privilege denial) that must NOT be written to errorlog.md.
|
||||
def __init__(self, msg, code=None, handled=False):
|
||||
super().__init__(msg)
|
||||
self.code = code
|
||||
self.handled = handled
|
||||
|
||||
|
||||
class SynoClient:
|
||||
@@ -222,10 +228,12 @@ class SynoClient:
|
||||
err = r.get("error") or {}
|
||||
code = err.get("code")
|
||||
hint = GENERIC_ERRORS.get(code, "")
|
||||
_log_skill_error(f"{api}.{method} failed (code {code})",
|
||||
context=f"err={json.dumps(err)[:120]}")
|
||||
# Do NOT log here -- call() can't know if the caller will handle this
|
||||
# (e.g. the `ls`/FileStation denial). The top-level handler logs only
|
||||
# genuinely-unhandled failures, so handled conversions don't pollute errorlog.
|
||||
raise SynoError(f"{api}.{method} -> error {code}"
|
||||
+ (f" ({hint})" if hint else "") + f" {json.dumps(err)[:200]}")
|
||||
+ (f" ({hint})" if hint else "") + f" {json.dumps(err)[:200]}",
|
||||
code=code)
|
||||
return r.get("data", r)
|
||||
|
||||
|
||||
@@ -235,6 +243,23 @@ def is_mutating(method):
|
||||
and not m.startswith(("getinfo", "list", "get", "info", "query", "load", "enum", "status"))
|
||||
|
||||
|
||||
# FileStation file-op denial: a SYNO.FileStation.* call returning 400 (web API) / 407
|
||||
# (on-box) when the account lacks FileStation file privileges (e.g. the built-in admin
|
||||
# on cascadesDS). list_share works; real file browsing must go via the SSH backend.
|
||||
def _filestation_denial(api, err):
|
||||
return api.startswith("SYNO.FileStation") and getattr(err, "code", None) in (400, 407)
|
||||
|
||||
|
||||
def _filestation_hint(err):
|
||||
return SynoError(
|
||||
f"{err}\n [HINT] FileStation file-listing is not permitted for this account "
|
||||
f"(built-in admin lacks FileStation file privileges on this device). Use the SSH "
|
||||
f"backend for real file browsing:\n"
|
||||
f" bash .claude/skills/synology/scripts/syno-ssh.sh run "
|
||||
f"\"ls -la /volume1/<share>/<subpath>\" --confirm",
|
||||
code=getattr(err, "code", None), handled=True)
|
||||
|
||||
|
||||
# ============================ CLI ============================
|
||||
def _print(obj):
|
||||
print(json.dumps(obj, indent=2, ensure_ascii=False, default=str))
|
||||
@@ -345,36 +370,37 @@ def main(argv):
|
||||
elif args.cmd == "packages":
|
||||
_print(c.call("SYNO.Core.Package", "list", additional='["status","installed_info"]'))
|
||||
elif args.cmd == "services":
|
||||
# DSM 7.2.x: SYNO.Core.Service enumerates via `get` (not `list`); returns {"service":[...]}
|
||||
_print(c.call("SYNO.Core.Service", "get"))
|
||||
# DSM 7.2.x enumerates SYNO.Core.Service via `get` (returns {"service":[...]});
|
||||
# older builds expose `list`. Try get, fall back to list on 103 (method missing).
|
||||
try:
|
||||
_print(c.call("SYNO.Core.Service", "get"))
|
||||
except SynoError as e:
|
||||
if e.code == 103:
|
||||
_print(c.call("SYNO.Core.Service", "list"))
|
||||
else:
|
||||
raise
|
||||
elif args.cmd == "connections":
|
||||
_print(c.call("SYNO.Core.CurrentConnection", "list"))
|
||||
elif args.cmd == "ls":
|
||||
if args.path:
|
||||
try:
|
||||
try:
|
||||
if args.path:
|
||||
_print(c.call("SYNO.FileStation.List", "list", folder_path=args.path,
|
||||
additional='["size","owner","time","perm"]'))
|
||||
except SynoError as e:
|
||||
# FileStation file-browsing (`list`) is denied for an account without
|
||||
# FileStation file privileges -- on-box it is 407, surfaced here as 400.
|
||||
# `list_share` (share roots) still works; for actual files use the SSH backend.
|
||||
if " 400" in str(e) or " 407" in str(e):
|
||||
raise SynoError(
|
||||
f"{e}\n [HINT] FileStation file-listing is not permitted for this "
|
||||
f"account (built-in admin lacks FileStation file privileges on this "
|
||||
f"device). Use the SSH backend for real file browsing:\n"
|
||||
f" bash .claude/skills/synology/scripts/syno-ssh.sh run "
|
||||
f"\"ls -la /volume1/<share>/<subpath>\" --confirm")
|
||||
raise
|
||||
else:
|
||||
_print(c.call("SYNO.FileStation.List", "list_share",
|
||||
additional='["size","owner","perm"]'))
|
||||
else:
|
||||
_print(c.call("SYNO.FileStation.List", "list_share",
|
||||
additional='["size","owner","perm"]'))
|
||||
except SynoError as e:
|
||||
raise _filestation_hint(e) if _filestation_denial("SYNO.FileStation.List", e) else e
|
||||
elif args.cmd == "call":
|
||||
params = _kv(args.params)
|
||||
if is_mutating(args.method):
|
||||
guard(f"{args.api}.{args.method}")
|
||||
_print(c.call(args.api, args.method, version=args.version,
|
||||
post=args.post or is_mutating(args.method), **params))
|
||||
try:
|
||||
_print(c.call(args.api, args.method, version=args.version,
|
||||
post=args.post or is_mutating(args.method), **params))
|
||||
except SynoError as e:
|
||||
# same FileStation denial guidance on the documented power-tool path
|
||||
raise _filestation_hint(e) if _filestation_denial(args.api, e) else e
|
||||
elif args.cmd == "pkg-start":
|
||||
guard("pkg-start")
|
||||
_print(c.call("SYNO.Core.Package.Control", "start", post=True, id=args.id))
|
||||
@@ -391,6 +417,11 @@ def main(argv):
|
||||
ap.print_help()
|
||||
return 2
|
||||
except SynoError as e:
|
||||
# Log only genuine, unhandled API failures. handled=True (e.g. the FileStation
|
||||
# file-privilege denial) and login errors (already logged inline, code=None) are
|
||||
# skipped so routine/expected conditions never pollute errorlog.md.
|
||||
if e.code is not None and not e.handled:
|
||||
_log_skill_error(str(e).splitlines()[0], context=f"code={e.code}")
|
||||
print(f"[ERROR] {e}", file=sys.stderr)
|
||||
return 1
|
||||
finally:
|
||||
@@ -403,9 +434,13 @@ def main(argv):
|
||||
|
||||
if __name__ == "__main__":
|
||||
try:
|
||||
sys.exit(main(sys.argv[1:]))
|
||||
rc = main(sys.argv[1:])
|
||||
# Flush inside the try so a broken downstream pipe (e.g. `apis | head`) surfaces
|
||||
# HERE and is caught -- not at interpreter shutdown, which would still print a
|
||||
# 'BrokenPipeError ignored' traceback for small block-buffered output.
|
||||
sys.stdout.flush()
|
||||
sys.exit(rc)
|
||||
except BrokenPipeError:
|
||||
# downstream pipe closed (e.g. `apis | head`) -- exit cleanly, no traceback
|
||||
try:
|
||||
sys.stdout.close()
|
||||
except Exception:
|
||||
|
||||
Reference in New Issue
Block a user