From 974be13f4c30206fb9049cff3d7a459125696fd0 Mon Sep 17 00:00:00 2001 From: Howard Enos Date: Thu, 25 Jun 2026 12:23:43 -0700 Subject: [PATCH] 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) --- .claude/skills/synology/SKILL.md | 22 +++-- .../skills/synology/scripts/syno_client.py | 89 +++++++++++++------ errorlog.md | 4 + 3 files changed, 79 insertions(+), 36 deletions(-) diff --git a/.claude/skills/synology/SKILL.md b/.claude/skills/synology/SKILL.md index 6497991e..7a45a033 100644 --- a/.claude/skills/synology/SKILL.md +++ b/.claude/skills/synology/SKILL.md @@ -1,15 +1,12 @@ --- name: synology description: > - Control a Synology NAS end-to-end via two surfaces: the DSM Web API (auth + full - API discovery + any API method — system, storage, shares, users/groups, packages, - services, FileStation, connections) and an SSH backend for the syno* CLI the Web - API doesn't expose (filesystem ACLs, low-level share/user/group internals). Reads - run freely; mutating calls (set/create/delete/start/stop/reboot/shutdown/...) are - gated behind --confirm. Default device is the Cascades NAS (cascadesDS, 192.168.0.120, - admin); point at another client's NAS with --vault. Triggers: synology, diskstation, - DSM, cascadesDS, NAS, control the synology, synology drive, hyper backup, active backup, - synology package/service/share/user, reboot the nas, synology acl. + Control a Synology NAS via the DSM Web API (auth + API discovery + any method: system, + storage, shares, users/groups, packages, services, FileStation) plus an SSH backend for + the syno* CLI (ACLs, low-level internals). Reads run free; mutating calls gated behind + --confirm. Default device Cascades cascadesDS; --vault for another client's NAS. Triggers: + synology, diskstation, DSM, cascadesDS, NAS, synology drive, hyper/active backup, synology + package/service/share/user, reboot the nas, synology acl. --- # synology — Synology DSM control (Web API + SSH) @@ -143,6 +140,13 @@ VPN-down connect error surfaced to the user, a method refused for lack of --conf hint on the FileStation 400/407 denial. **`--confirm`/`--vault` after the subcommand were rejected by argparse** (every documented gated-write example, e.g. `call X set k=v --confirm`, would have failed) — moved to a shared parent parser so both flags now work before AND after the subcommand. +- **Code-review hardening (2026-06-25, /code-review high):** `SynoError` now carries the DSM `code` + + a `handled` flag; `call()` no longer logs eagerly — the top-level handler logs only genuine + unhandled failures, so the handled FileStation denial (and VPN-down connect errors) no longer + pollute `errorlog.md`. FileStation-denial detection is numeric (`code in (400,407)`), not a + substring match, and the SSH hint now also fires on the generic `call` path (not just `ls`). + `services` falls back `get`->`list` on 103 for older DSM builds. BrokenPipe flush moved inside + the try so small piped output can't leak a shutdown traceback. - **DSM 7.2.x reboot/shutdown 103:** the Web-API `reboot`/`shutdown` can return error 103 on some builds. Fallback is the SSH `reboot|shutdown --confirm` recipe (`synoshutdown -r|-s`). - **Param-schema caveat:** method names + versions in the reference are device-authoritative, but many diff --git a/.claude/skills/synology/scripts/syno_client.py b/.claude/skills/synology/scripts/syno_client.py index efcc5ac1..94267875 100644 --- a/.claude/skills/synology/scripts/syno_client.py +++ b/.claude/skills/synology/scripts/syno_client.py @@ -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//\" --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//\" --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: diff --git a/errorlog.md b/errorlog.md index 0d674dee..1ca9a457 100644 --- a/errorlog.md +++ b/errorlog.md @@ -17,6 +17,10 @@ Categories (the `[type]` tag): _(none)_ = skill/command execution failure · +2026-06-25 | Howard-Home | wiki-compile/gururmm | [correction] characterized SPEC-030 software uninstall as SHIPPED/working capability; correct is BETA, merged+deployed but NOT guaranteed to work (many uninstallers fail: AV, Launchy/AIMP, drivers) + +2026-06-25 | Howard-Home | remediation-tool | reset-password: failed to remove JIT Privileged Auth Admin role - standing privilege left behind, REMOVE MANUALLY [ctx: tenant=207fa277-e9d8-4eb7-ada1-1064d2221498 assignment=ikzke6-tKk6E1qsmSeCKE6mJ-qU1txBOtmTwQuJl0Tc-1 http=400] + 2026-06-25 | Howard-Home | synology/ls | [friction] ls arg mangled by MSYS path-conversion (/Public -> C:/Program Files/Git/Public) before Python sees it; FileStation list also 407-denied for admin anyway. Fix: use call folder_path=/x (= form not converted) or MSYS_NO_PATHCONV=1; real file browsing via SSH backend. [ctx: ref=feedback_tmp_path_windows os=windows] 2026-06-25 | Howard-Home | synology | SYNO.FileStation.List.list failed (code 400) [ctx: err={"code": 400}]