From 0c112a9ea601cba2aaad3de4b97f2e1cacd43696 Mon Sep 17 00:00:00 2001 From: Ben Meadors Date: Fri, 17 Apr 2026 18:20:29 -0500 Subject: [PATCH] Trunk and semgrep fixes --- .claude/commands/diagnose.md | 2 +- .claude/commands/repro.md | 2 +- .github/prompts/mcp-diagnose.prompt.md | 2 +- .github/prompts/mcp-repro.prompt.md | 2 +- .trunk/configs/.bandit | 28 +++++++++++++++++- mcp-server/README.md | 2 +- mcp-server/run-tests.sh | 29 ++++++++++++------- mcp-server/src/meshtastic_mcp/admin.py | 1 + mcp-server/src/meshtastic_mcp/cli/_fwlog.py | 1 + mcp-server/src/meshtastic_mcp/config.py | 2 +- .../src/meshtastic_mcp/serial_session.py | 2 +- mcp-server/src/meshtastic_mcp/server.py | 3 +- mcp-server/src/meshtastic_mcp/userprefs.py | 5 +++- .../tests/admin/test_owner_survives_reboot.py | 1 - mcp-server/tests/conftest.py | 26 +++++++---------- mcp-server/tests/mesh/_receive.py | 9 ++++-- mcp-server/tests/mesh/test_direct_with_ack.py | 2 +- mcp-server/tests/mesh/test_mesh_formation.py | 1 - .../tests/monitor/test_boot_log_no_panic.py | 2 -- mcp-server/tests/tool_coverage.py | 13 +++++---- 20 files changed, 85 insertions(+), 50 deletions(-) diff --git a/.claude/commands/diagnose.md b/.claude/commands/diagnose.md index e4dbe7962..45aa937a5 100644 --- a/.claude/commands/diagnose.md +++ b/.claude/commands/diagnose.md @@ -26,7 +26,7 @@ Call the meshtastic MCP tool bundle and format a structured health report for on 4. **Render per-device report** as: - ``` + ```text [nrf52 @ /dev/cu.usbmodem1101] fw=2.7.23.bce2825, hw=RAK4631 owner : Meshtastic 40eb / 40eb region/band : US, channel 88, LONG_FAST diff --git a/.claude/commands/repro.md b/.claude/commands/repro.md index 7ebf9ccd4..52dcf222b 100644 --- a/.claude/commands/repro.md +++ b/.claude/commands/repro.md @@ -23,7 +23,7 @@ Re-run a single pytest node ID N times in isolation, track pass rate, and surfac 4. **Track a small structured tally**: - ``` + ```text attempt 1: PASS (42s) attempt 2: FAIL (128s) ← firmware log 200-line tail captured attempt 3: PASS (39s) diff --git a/.github/prompts/mcp-diagnose.prompt.md b/.github/prompts/mcp-diagnose.prompt.md index 188392043..c86826030 100644 --- a/.github/prompts/mcp-diagnose.prompt.md +++ b/.github/prompts/mcp-diagnose.prompt.md @@ -28,7 +28,7 @@ This prompt assumes the meshtastic MCP server is registered with your VS Code Co 4. **Render per-device report** as a compact block: - ``` + ```text [nrf52 @ /dev/cu.usbmodem1101] fw=2.7.23.bce2825, hw=RAK4631 owner : Meshtastic 40eb / 40eb region/band : US, channel 88, LONG_FAST diff --git a/.github/prompts/mcp-repro.prompt.md b/.github/prompts/mcp-repro.prompt.md index 62ffcbc52..be2963c33 100644 --- a/.github/prompts/mcp-repro.prompt.md +++ b/.github/prompts/mcp-repro.prompt.md @@ -25,7 +25,7 @@ Equivalent of `.claude/commands/repro.md`. Use when the operator says "that one 4. **Tally** results as you go: - ``` + ```text attempt 1: PASS (42s) attempt 2: FAIL (128s) ← fw log captured attempt 3: PASS (39s) diff --git a/.trunk/configs/.bandit b/.trunk/configs/.bandit index d286ded89..c70e7743b 100644 --- a/.trunk/configs/.bandit +++ b/.trunk/configs/.bandit @@ -1,2 +1,28 @@ [bandit] -skips = B101 \ No newline at end of file +# Rule IDs: https://bandit.readthedocs.io/en/latest/plugins/index.html +# +# B101 assert_used +# pytest assertions + internal invariants; required for pytest. +# B110 try_except_pass +# best-effort cleanup paths (atexit handlers, pubsub unsubscribe, +# session-end file close, socket shutdown). Logging inside the +# except block would be worse than the silent pass — teardown is +# already at end-of-session and the surrounding caller has context. +# B112 try_except_continue +# defensive loops over flaky sources (pubsub handlers, device +# re-enumeration polls). One failed iteration shouldn't abort the loop. +# B404 import_subprocess +# mcp-server wraps PlatformIO, esptool, nrfutil, picotool, and the +# pytest test-runner — subprocess is a load-bearing import here, not +# a smell. The "consider possible security implications" advisory is +# redundant given the file-level review already applied. +# B603 subprocess_without_shell_equals_true +# all subprocess calls use a static argv list; `shell=False` is the +# default and we never string-interpolate user input into the command. +# B606 start_process_with_no_shell +# same invariant as B603 — running a binary via argv list (not +# `shell=True`) is the safe pattern bandit is asking for. +# +# Higher-severity checks (B102 exec_used, B301 pickle, B307 eval, +# B602 shell=True, etc.) remain enabled. +skips = B101,B110,B112,B404,B603,B606 \ No newline at end of file diff --git a/mcp-server/README.md b/mcp-server/README.md index d5ceb6822..3f1fecdf1 100644 --- a/mcp-server/README.md +++ b/mcp-server/README.md @@ -148,7 +148,7 @@ _The tool tables below document 38 currently registered MCP server tools._ ## Layout -``` +```text mcp-server/ ├── pyproject.toml ├── README.md diff --git a/mcp-server/run-tests.sh b/mcp-server/run-tests.sh index 87dacd119..292e6e3a2 100755 --- a/mcp-server/run-tests.sh +++ b/mcp-server/run-tests.sh @@ -25,7 +25,7 @@ SCRIPT_DIR="$(cd "$(dirname "$0")" && pwd)" cd "$SCRIPT_DIR" VENV_PY="$SCRIPT_DIR/.venv/bin/python" -if [ ! -x "$VENV_PY" ]; then +if [[ ! -x $VENV_PY ]]; then echo "error: $VENV_PY not found or not executable." >&2 echo " Bootstrap the venv first:" >&2 echo " cd $SCRIPT_DIR && python3 -m venv .venv && .venv/bin/pip install -e '.[test]'" >&2 @@ -42,7 +42,7 @@ USERPREFS_SIDECAR="$USERPREFS_PATH.mcp-session-bak" # If conftest.py's atexit hook didn't fire (SIGKILL, kernel panic, OS # restart), the sidecar is the ground truth. Self-heal before running so we # don't bake the previous run's dirty state into this run's firmware. -if [ -f "$USERPREFS_SIDECAR" ]; then +if [[ -f $USERPREFS_SIDECAR ]]; then echo "[pre-flight] found $USERPREFS_SIDECAR from a prior abnormal exit;" >&2 echo " restoring userPrefs.jsonc before starting." >&2 cp "$USERPREFS_SIDECAR" "$USERPREFS_PATH" @@ -54,7 +54,12 @@ fi # it at the end, which may not be what the operator wants. if command -v git >/dev/null 2>&1; then cd "$FIRMWARE_ROOT" - if [ -n "$(git status --porcelain userPrefs.jsonc 2>/dev/null)" ]; then + # Capture the git status into a local first — SC2312 flags command + # substitution inside `[[ -n ... ]]` because the exit code of `git + # status` is masked. A two-step assignment makes the failure path + # explicit (non-git, missing file) and keeps the bracket test clean. + _git_status_porcelain="$(git status --porcelain userPrefs.jsonc 2>/dev/null || true)" + if [[ -n $_git_status_porcelain ]]; then echo "[pre-flight] warning: userPrefs.jsonc has uncommitted changes." >&2 echo " Tests will snapshot THIS state and restore to it" >&2 echo " at teardown. If that's not intended, run:" >&2 @@ -68,7 +73,7 @@ fi # Per-machine default so repeated runs from the same operator land on the # same PSK (makes --assume-baked valid across invocations). Operator can # override with an explicit env var if they want isolation (e.g. CI). -if [ -z "${MESHTASTIC_MCP_SEED-}" ]; then +if [[ -z ${MESHTASTIC_MCP_SEED-} ]]; then WHO="$(whoami 2>/dev/null || echo anon)" HOST="$(hostname -s 2>/dev/null || echo host)" export MESHTASTIC_MCP_SEED="mcp-${WHO}-${HOST}" @@ -133,7 +138,7 @@ resolve_env() { upper="$(echo "$role" | tr '[:lower:]' '[:upper:]')" local var="MESHTASTIC_MCP_ENV_${upper}" eval "local override=\${$var:-}" - if [ -n "$override" ]; then + if [[ -n $override ]]; then echo "$override" else echo "$default" @@ -144,12 +149,12 @@ NRF52_PORT="$(echo "$ROLES_JSON" | "$VENV_PY" -c 'import json,sys; print(json.lo ESP32S3_PORT="$(echo "$ROLES_JSON" | "$VENV_PY" -c 'import json,sys; print(json.loads(sys.stdin.read()).get("esp32s3", ""))')" DETECTED="" -if [ -n "$NRF52_PORT" ]; then +if [[ -n $NRF52_PORT ]]; then NRF52_ENV="$(resolve_env nrf52 rak4631)" export MESHTASTIC_MCP_ENV_NRF52="$NRF52_ENV" DETECTED="${DETECTED} nrf52 @ ${NRF52_PORT} -> env=${NRF52_ENV}\n" fi -if [ -n "$ESP32S3_PORT" ]; then +if [[ -n $ESP32S3_PORT ]]; then ESP32S3_ENV="$(resolve_env esp32s3 heltec-v3)" export MESHTASTIC_MCP_ENV_ESP32S3="$ESP32S3_ENV" DETECTED="${DETECTED} esp32s3 @ ${ESP32S3_PORT} -> env=${ESP32S3_ENV}\n" @@ -165,6 +170,7 @@ for _arg in "$@"; do case "$_arg" in --assume-baked) _bake_mode="skip (--assume-baked)" ;; --force-bake) _bake_mode="force (--force-bake)" ;; + *) ;; # any other arg: pass-through; bake mode unchanged esac done @@ -172,7 +178,7 @@ echo "mcp-server test runner" echo " firmware root : $FIRMWARE_ROOT" echo " seed : $MESHTASTIC_MCP_SEED" echo " bake : $_bake_mode" -if [ -n "$DETECTED" ]; then +if [[ -n $DETECTED ]]; then echo " detected hub :" printf "%b" "$DETECTED" else @@ -185,7 +191,7 @@ echo # PASS/FAIL — every hardware test would SKIP with "role not present". We # narrow to tests/unit explicitly so the summary reads as "no hardware, # unit suite only" instead of "big skip count looks suspicious". -if [ -z "$DETECTED" ] && [ "$#" -eq 0 ]; then +if [[ -z $DETECTED && $# -eq 0 ]]; then echo "[pre-flight] no supported devices detected; running unit tier only." echo exec "$VENV_PY" -m pytest tests/unit -v --report-log=tests/reportlog.jsonl @@ -204,7 +210,7 @@ fi # skipping half the hardware tests with "not baked with session profile" # errors. Power users who know their hardware is current and want to shave # those seconds can pass `--assume-baked` explicitly. -if [ "$#" -eq 0 ]; then +if [[ $# -eq 0 ]]; then set -- tests/ \ --html=tests/report.html --self-contained-html \ --junitxml=tests/junit.xml \ @@ -220,9 +226,10 @@ _has_report_log=0 for _arg in "$@"; do case "$_arg" in --report-log | --report-log=*) _has_report_log=1 ;; + *) ;; # any other arg: no-op; loop continues esac done -if [ "$_has_report_log" -eq 0 ]; then +if [[ $_has_report_log -eq 0 ]]; then set -- "$@" --report-log=tests/reportlog.jsonl fi diff --git a/mcp-server/src/meshtastic_mcp/admin.py b/mcp-server/src/meshtastic_mcp/admin.py index 23cbaef27..d84c96dda 100644 --- a/mcp-server/src/meshtastic_mcp/admin.py +++ b/mcp-server/src/meshtastic_mcp/admin.py @@ -208,6 +208,7 @@ def set_config(path: str, value: Any, port: str | None = None) -> dict[str, Any] set_config("device.role", "ROUTER") set_config("mqtt.enabled", True) set_config("mqtt.address", "mqtt.example.com") + """ segments = [s for s in path.split(".") if s] if not segments: diff --git a/mcp-server/src/meshtastic_mcp/cli/_fwlog.py b/mcp-server/src/meshtastic_mcp/cli/_fwlog.py index 7294ce2cd..7db20f81c 100644 --- a/mcp-server/src/meshtastic_mcp/cli/_fwlog.py +++ b/mcp-server/src/meshtastic_mcp/cli/_fwlog.py @@ -46,6 +46,7 @@ class FirmwareLogTailer(threading.Thread): wait_s: How long to poll for the file's creation before giving up. Default 30 s; pytest collection on a cold cache can be slow. + """ def __init__( diff --git a/mcp-server/src/meshtastic_mcp/config.py b/mcp-server/src/meshtastic_mcp/config.py index c9020636a..7ece10032 100644 --- a/mcp-server/src/meshtastic_mcp/config.py +++ b/mcp-server/src/meshtastic_mcp/config.py @@ -18,7 +18,7 @@ class ConfigError(RuntimeError): def firmware_root() -> Path: - """The root of the Meshtastic firmware repo. + """Resolve the root of the Meshtastic firmware repo. Resolution order: 1. `MESHTASTIC_FIRMWARE_ROOT` env var. diff --git a/mcp-server/src/meshtastic_mcp/serial_session.py b/mcp-server/src/meshtastic_mcp/serial_session.py index ce354301a..b9c71d1d0 100644 --- a/mcp-server/src/meshtastic_mcp/serial_session.py +++ b/mcp-server/src/meshtastic_mcp/serial_session.py @@ -19,7 +19,7 @@ import uuid from dataclasses import dataclass, field from typing import Any -from . import boards, config, pio +from . import boards, config _BUFFER_MAX_LINES = 10_000 _POLL_NEW_PORT_TIMEOUT_S = 3.0 diff --git a/mcp-server/src/meshtastic_mcp/server.py b/mcp-server/src/meshtastic_mcp/server.py index 20925bf30..7cb8db65e 100644 --- a/mcp-server/src/meshtastic_mcp/server.py +++ b/mcp-server/src/meshtastic_mcp/server.py @@ -205,7 +205,7 @@ def userprefs_get() -> dict[str, Any]: @app.tool() def userprefs_set(prefs: dict[str, Any]) -> dict[str, Any]: - """Persistently merge `prefs` into userPrefs.jsonc (uncommenting keys). + """Merge `prefs` into userPrefs.jsonc persistently (uncommenting keys). Existing active values not in `prefs` are kept. To remove a key from the active set, call `userprefs_reset` (restores the MCP backup if present) @@ -275,6 +275,7 @@ def userprefs_testing_profile( long_name: optional owner long name stamped into the build. disable_mqtt: disable MQTT module + uplink/downlink (default True). disable_position: disable GPS + smart-position broadcasts (default False). + """ return userprefs_mod.build_testing_profile( psk_seed=psk_seed, diff --git a/mcp-server/src/meshtastic_mcp/userprefs.py b/mcp-server/src/meshtastic_mcp/userprefs.py index 351696d11..59d7165f9 100644 --- a/mcp-server/src/meshtastic_mcp/userprefs.py +++ b/mcp-server/src/meshtastic_mcp/userprefs.py @@ -69,6 +69,7 @@ def _parse_jsonc_state(text: str) -> dict[str, Any]: "commented": {key: string_value, ...}, # commented examples "order": [key, ...] # source order for round-trip } + """ active: dict[str, str] = {} commented: dict[str, str] = {} @@ -415,12 +416,14 @@ def build_testing_profile( isolation). region: short code from `KNOWN_REGIONS`. modem_preset: short code from `KNOWN_MODEM_PRESETS`. - short_name / long_name: optional owner identity stamps. + short_name: optional owner short-name stamp (≤4 chars). None = unset. + long_name: optional owner long-name stamp. None = unset. disable_mqtt: if True (default), disables the MQTT module and the uplink/downlink bridge on the primary channel — so private test traffic never leaks to a public broker. disable_position: if True, disables GPS + position broadcasts — useful when test devices sit on a bench without antennas. + """ if region not in KNOWN_REGIONS: raise ValueError( diff --git a/mcp-server/tests/admin/test_owner_survives_reboot.py b/mcp-server/tests/admin/test_owner_survives_reboot.py index 87070bdda..eda83f4a0 100644 --- a/mcp-server/tests/admin/test_owner_survives_reboot.py +++ b/mcp-server/tests/admin/test_owner_survives_reboot.py @@ -21,7 +21,6 @@ def test_owner_survives_reboot( ) -> None: """Runs once per connected role — proves the reboot-persistence round-trip works on each device independently, not just one.""" - role = baked_single["role"] port = baked_single["port"] pre = info.device_info(port=port, timeout_s=8.0) diff --git a/mcp-server/tests/conftest.py b/mcp-server/tests/conftest.py index 685f4cf3d..4597fac46 100644 --- a/mcp-server/tests/conftest.py +++ b/mcp-server/tests/conftest.py @@ -35,7 +35,10 @@ import pytest # Ensure the MCP server is on `sys.path` without requiring installation in # development mode for every checkout (we DO install in .venv but this makes -# `pytest tests/` work from a fresh clone too). +# `pytest tests/` work from a fresh clone too). The path mutation must +# happen before `meshtastic_mcp.*` imports below — hence the `noqa: E402` +# markers on those imports (ruff's "module-level import not at top of file" +# rule doesn't understand path-bootstrapping patterns). _HERE = pathlib.Path(__file__).resolve().parent _MCP_SRC = _HERE.parent / "src" if str(_MCP_SRC) not in sys.path: @@ -44,18 +47,9 @@ if str(_MCP_SRC) not in sys.path: # Default firmware root: the repo this mcp-server/ lives inside. os.environ.setdefault("MESHTASTIC_FIRMWARE_ROOT", str(_HERE.parent.parent)) -from meshtastic_mcp import ( - admin, -) -from meshtastic_mcp import ( # noqa: E402 (import after path setup) - devices as devices_module, -) -from meshtastic_mcp import ( - flash, - info, - serial_session, - userprefs, -) +from meshtastic_mcp import admin # noqa: E402 +from meshtastic_mcp import devices as devices_module # noqa: E402 +from meshtastic_mcp import info, serial_session, userprefs # noqa: E402 from . import tool_coverage # noqa: E402 @@ -407,7 +401,7 @@ def baked_mesh( session_seed: str, request: pytest.FixtureRequest, ) -> dict[str, Any]: - """Verifies that both roles are baked with the session `test_profile`. + """Verify that both roles are baked with the session `test_profile`. Does NOT reflash. `test_00_bake.py` is responsible for applying the bake; this fixture just checks the result by connecting to each device and @@ -696,7 +690,7 @@ class _SerialCapture: @pytest.fixture def serial_capture(hub_devices: dict[str, str], request: pytest.FixtureRequest) -> Any: - """Returns a `_SerialCapture` factory. + """Return a `_SerialCapture` factory. Usage: cap = serial_capture("esp32s3") @@ -929,7 +923,7 @@ def pytest_runtest_makereport(item: pytest.Item, call: pytest.CallInfo[Any]) -> # Attach serial captures (if the test used `serial_capture`) caps = getattr(item, "_serial_captures", None) if caps: - for i, cap in enumerate(caps): + for cap in caps: try: lines = _run_with_timeout(lambda c=cap: c.snapshot(max_lines=2000), 5.0) except Exception as exc: diff --git a/mcp-server/tests/mesh/_receive.py b/mcp-server/tests/mesh/_receive.py index a21866e1d..d037f103b 100644 --- a/mcp-server/tests/mesh/_receive.py +++ b/mcp-server/tests/mesh/_receive.py @@ -101,13 +101,16 @@ class ReceiveCollector: pass def snapshot(self) -> list[dict[str, Any]]: - """Current list of collected packets (thread-safe copy).""" + """Return a thread-safe copy of the list of collected packets.""" with self._lock: return list(self._packets) def log_snapshot(self) -> list[str]: - """Captured firmware log lines (only populated if `capture_logs=True` - AND the device has `security.debug_log_api_enabled=True`).""" + """Return captured firmware log lines. + + Only populated if `capture_logs=True` AND the device has + `security.debug_log_api_enabled=True`. + """ with self._lock: return list(self._log_lines) diff --git a/mcp-server/tests/mesh/test_direct_with_ack.py b/mcp-server/tests/mesh/test_direct_with_ack.py index eb331e9f1..38ea7df36 100644 --- a/mcp-server/tests/mesh/test_direct_with_ack.py +++ b/mcp-server/tests/mesh/test_direct_with_ack.py @@ -86,7 +86,7 @@ def test_direct_with_ack_roundtrip( # sufficient on its own with fresh keys; the retry is purely # an airtime-collision safety net. got = None - for attempt in range(2): + for _attempt in range(2): packet = tx_iface.sendText( unique, destinationId=rx_node_num, diff --git a/mcp-server/tests/mesh/test_mesh_formation.py b/mcp-server/tests/mesh/test_mesh_formation.py index 992994ea1..9ec37abca 100644 --- a/mcp-server/tests/mesh/test_mesh_formation.py +++ b/mcp-server/tests/mesh/test_mesh_formation.py @@ -11,7 +11,6 @@ from __future__ import annotations from typing import Any import pytest -from meshtastic_mcp import info from meshtastic_mcp.connection import connect diff --git a/mcp-server/tests/monitor/test_boot_log_no_panic.py b/mcp-server/tests/monitor/test_boot_log_no_panic.py index 0f4d9f741..2e9bab028 100644 --- a/mcp-server/tests/monitor/test_boot_log_no_panic.py +++ b/mcp-server/tests/monitor/test_boot_log_no_panic.py @@ -7,8 +7,6 @@ misconfig), this test fails within a minute of reboot. from __future__ import annotations -import os -import re import time from typing import Any diff --git a/mcp-server/tests/tool_coverage.py b/mcp-server/tests/tool_coverage.py index 3050254e2..b91bd4039 100644 --- a/mcp-server/tests/tool_coverage.py +++ b/mcp-server/tests/tool_coverage.py @@ -14,7 +14,7 @@ from __future__ import annotations import json import pathlib -from typing import Any, Callable +from typing import Any _counts: dict[str, int] = {} _installed = False @@ -108,14 +108,17 @@ def install() -> None: for tool_name, (module_path, attr) in _TOOL_MAP.items(): # Defense in depth: if someone mutates `_TOOL_MAP` at runtime # (shouldn't happen; it's module-level) the whitelist catches it. + # `module_path` is a key from the hardcoded `_TOOL_MAP` dict and + # is gated above by membership in `_allowed_modules` (itself + # derived from the same literal values). There is no path for + # untrusted input to reach the `import_module` call below; the + # Semgrep suppression must sit on the line immediately preceding + # the call (multi-line comment blocks between comment and call + # break the rule's scope detection). if module_path not in _allowed_modules: continue try: # nosemgrep: python.lang.security.audit.non-literal-import.non-literal-import - # `module_path` is a key from the hardcoded `_TOOL_MAP` dict - # and is gated above by membership in `_allowed_modules` - # (itself derived from the same literal values). There is no - # path for untrusted input to reach this call. mod = importlib.import_module(module_path) except ImportError: continue