From 812943b3edc44803d551c00ece8d059ac424861d Mon Sep 17 00:00:00 2001 From: Teknium <127238744+teknium1@users.noreply.github.com> Date: Thu, 3 Sep 2026 09:42:58 -0700 Subject: [PATCH] =?UTF-8?q?review-fix(suppress-audit):=20whatsapp=20adapte?= =?UTF-8?q?r,=20skill=5Fmanager=5Ftool,=20skill=5Fusage=20=E2=80=94=20rest?= =?UTF-8?q?ore=20BASE=20exception=20semantics?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- plugins/platforms/whatsapp/adapter.py | 12 +++++++----- tools/skill_manager_tool.py | 28 +++++++++++++++++++-------- tools/skill_usage.py | 12 ++++++++++-- 3 files changed, 37 insertions(+), 15 deletions(-) diff --git a/plugins/platforms/whatsapp/adapter.py b/plugins/platforms/whatsapp/adapter.py index b5126136e1..9d2c84bb72 100644 --- a/plugins/platforms/whatsapp/adapter.py +++ b/plugins/platforms/whatsapp/adapter.py @@ -80,12 +80,14 @@ def _kill_port_process(port: int) -> None: if pid <= 0 or not _pid_looks_like_node_bridge(pid): logger.warning("[whatsapp] Not killing PID %s on port %d: process is not a node bridge (or identity unverifiable)", pid, port) continue - with suppress(subprocess.SubprocessError, OSError): # ProcessLookupError/PermissionError are OSError subclasses - if not _IS_WINDOWS: - os.kill(pid, signal.SIGTERM) - continue + if _IS_WINDOWS: from hermes_cli._subprocess_compat import windows_hide_flags - subprocess.run(["taskkill", "/PID", str(pid), "/F"], capture_output=True, stdin=subprocess.DEVNULL, timeout=5, creationflags=windows_hide_flags()) + # Only SubprocessError is swallowed per-PID; an OSError (e.g. taskkill missing) aborts the scan. + with suppress(subprocess.SubprocessError): + subprocess.run(["taskkill", "/PID", str(pid), "/F"], capture_output=True, stdin=subprocess.DEVNULL, timeout=5, creationflags=windows_hide_flags()) + else: + with suppress(OSError): # ProcessLookupError/PermissionError are OSError subclasses + os.kill(pid, signal.SIGTERM) def _bridge_pid_is_ours(pid: int, session_path: Path, expected_start) -> bool: diff --git a/tools/skill_manager_tool.py b/tools/skill_manager_tool.py index c8e2f2e850..539a56291c 100644 --- a/tools/skill_manager_tool.py +++ b/tools/skill_manager_tool.py @@ -240,15 +240,27 @@ def _find_skill_in_other_profiles(name: str) -> List[Tuple[str, Path]]: return matches _active = _skills_dir() active_dir = _active.resolve() if _active.exists() else _active - # Every profile's skills dir EXCEPT the active one (already searched). - candidates: List[Tuple[str, Path]] = [("default", root / "skills")] - with suppress(OSError): - if (root / "profiles").is_dir(): - candidates += [(e.name, e / "skills") for e in (root / "profiles").iterdir() if e.is_dir()] + # Every profile's skills dir EXCEPT the active one (already searched). A candidate whose + # path cannot be resolved is skipped (not a fatal error); is_dir() checks stay unguarded. + candidates: List[Tuple[str, Path]] = [] + with suppress(OSError, RuntimeError): + if (root / "skills").resolve() != active_dir: + candidates.append(("default", root / "skills")) + if (root / "profiles").is_dir(): + with suppress(OSError): + for entry in (root / "profiles").iterdir(): + if not entry.is_dir(): + continue + try: + if (entry / "skills").resolve() == active_dir: + continue + except (OSError, RuntimeError): + continue + candidates.append((entry.name, entry / "skills")) for profile_name, skills_dir in candidates: - with suppress(OSError, RuntimeError): - if skills_dir.resolve() == active_dir or not skills_dir.is_dir(): - continue + if not skills_dir.is_dir(): + continue + with suppress(OSError): hit = next((d for d in _iter_skill_dirs(skills_dir) if d.name == name), None) if hit is not None: matches.append((profile_name, hit)) # one match per profile is enough diff --git a/tools/skill_usage.py b/tools/skill_usage.py index ba6c95d13a..bd3f582908 100644 --- a/tools/skill_usage.py +++ b/tools/skill_usage.py @@ -264,7 +264,11 @@ def is_curation_eligible(skill_name: str, skill_path: Optional[Path] = None) -> def _is_curator_managed_record(record: Any) -> bool: """``created_by`` is a curator-management OPT-IN flag, not proof of authorship (``curator adopt`` flips it); - the key name is kept because it lives in every user's ``.usage.json``.""" + the key name is kept because it lives in every user's ``.usage.json``. + + NAMING (issue #67140): the on-disk field is ``created_by``, which reads like provenance but is consumed + as a **curator-management opt-in policy flag**. The two are not the same question: + """ return isinstance(record, dict) and (record.get("created_by") == "agent" or record.get("agent_created") is True) @@ -521,7 +525,11 @@ def set_state(skill_name: str, state: str) -> None: def set_pinned(skill_name: str, pinned: bool) -> bool: - """False when the write did not land (not curation-eligible).""" + """False when the write did not land (not curation-eligible). + + (skill not curation-eligible), True on success — so callers can report failure instead of a false + success (issue #92993). + """ return _set_field(skill_name, "pinned", bool(pinned))