diff --git a/docs/CODE_QUALITY_INDUSTRIAL_TOOLS_2026-03-25.md b/docs/CODE_QUALITY_INDUSTRIAL_TOOLS_2026-03-25.md new file mode 100644 index 0000000..2fc15cc --- /dev/null +++ b/docs/CODE_QUALITY_INDUSTRIAL_TOOLS_2026-03-25.md @@ -0,0 +1,113 @@ +# Code Quality Report: Industrial Tools +> Date: 2026-03-25 +> Scope: `tools/industrial/bom_analyzer.py`, `tools/industrial/opcua_mcp.py`, `tools/industrial/mqtt_mcp.py`, `tools/industrial/nodered_connector.py`, `tools/apify_mcp.py` + +## Summary Table + +| File | Lines | Issues | Critical | High | Medium | Low | +|------|-------|--------|----------|------|--------|-----| +| `tools/industrial/bom_analyzer.py` | 940 | 3 | 0 | 0 | 1 | 2 | +| `tools/industrial/opcua_mcp.py` | 329 | 2 | 0 | 0 | 1 | 1 | +| `tools/industrial/mqtt_mcp.py` | 388 | 2 | 0 | 0 | 1 | 1 | +| `tools/industrial/nodered_connector.py` | 315 | 2 | 0 | 1 | 0 | 1 | +| `tools/apify_mcp.py` | 374 | 4 | 1 | 1 | 1 | 1 | +| **Total** | **2346** | **13** | **1** | **2** | **4** | **6** | + +## Fixes Applied + +Three bugs were fixed directly in the codebase: + +1. **CRITICAL** `tools/apify_mcp.py` line 284 -- `monitor_updates` used Python's built-in `hash()` for content change detection. `hash()` is randomized across process restarts (PYTHONHASHSEED), so every server restart would report every monitored URL as "changed". Replaced with `hashlib.md5().hexdigest()` for deterministic hashing. + +2. **HIGH** `tools/apify_mcp.py` line 310 -- File handle leak: `sum(1 for _ in open(dataset_file))` opens a file without closing it. Wrapped in `with` statement. + +3. **HIGH** `tools/industrial/nodered_connector.py` line 266 -- `int(self.headers.get("Content-Length", 0))` would raise `ValueError` on malformed headers (e.g. `Content-Length: chunked` or empty string). Wrapped in try/except. + +--- + +## Detailed Findings + +### `tools/industrial/bom_analyzer.py` (940 lines) + +**Architecture:** CLI tool with argparse, CSV parsing, LCSC knowledge base, DFM checking, report generation. Well-structured, comprehensive. + +| # | Severity | Category | Description | +|---|----------|----------|-------------| +| 1 | Medium | Dead code | `detect_delimiter()` lines 210-213: `for delim in [",", ";", "\t"]: ... pass` loop body is empty. The loop iterates but does nothing before falling through to `csv.Sniffer`. Appears to be leftover scaffolding for a heuristic that was never implemented. | +| 2 | Low | Style | `Optional` import from `typing` (line 30) is never used. All optional types use `X \| None` syntax. | +| 3 | Low | Fragility | `generate_report()` hardcodes date `2026-03-25` (line 771). Should use `datetime.date.today()` or accept a parameter. | + +**Positives:** Good use of dataclasses, clean CLI with subcommands, solid CSV parsing with multi-EDA column normalization, LCSC price API with proper error handling, thorough DFM checking. + +--- + +### `tools/industrial/opcua_mcp.py` (329 lines) + +**Architecture:** MCP stdio server for OPC-UA. Clean separation between asyncua-backed implementation and stub fallback. + +| # | Severity | Category | Description | +|---|----------|----------|-------------| +| 1 | Medium | Security | `opcua_write` tool allows writing arbitrary values to OPC-UA nodes with no confirmation or allowlist. The description says "use with caution" but there is no programmatic safeguard. Consider adding an env-var opt-in like `OPCUA_WRITE_ENABLED=1`. | +| 2 | Low | Unused import | `sys` (line 9) is imported but never used. | + +**Positives:** Proper depth/breadth limiting on browse, subscription duration capped at 30s, clean fallback stubs, good JSON serialization of OPC-UA types. + +--- + +### `tools/industrial/mqtt_mcp.py` (388 lines) + +**Architecture:** MCP stdio server for MQTT. Uses paho-mqtt with threading for connect/subscribe, asyncio sleep for duration control. + +| # | Severity | Category | Description | +|---|----------|----------|-------------| +| 1 | Medium | Security | `mqtt_publish` allows publishing to any topic with no allowlist or confirmation. Same concern as OPC-UA write -- an env-var gate (`MQTT_PUBLISH_ENABLED`) would add a safety layer for production brokers. | +| 2 | Low | Unused import | `sys` (line 9) is imported but never used. | + +**Positives:** Proper broker string parsing with port fallback, buffer size limits, retained message fetching in history, duration caps on subscriptions. + +--- + +### `tools/industrial/nodered_connector.py` (315 lines) + +**Architecture:** HTTP bridge between Node-RED and Mascarade. Supports aiohttp (async) and stdlib fallback. Clean msg format translation. + +| # | Severity | Category | Description | +|---|----------|----------|-------------| +| 1 | High | Bug | **FIXED.** `Content-Length` parsing in stdlib fallback server could crash on malformed headers. | +| 2 | Low | Robustness | Stdlib fallback `do_POST` (line 278) calls `asyncio.run()` per request, which creates a new event loop each time. This works but is inefficient under load. The aiohttp path does not have this issue. | + +**Positives:** Dual-mode server (aiohttp + stdlib), clean Node-RED msg format helpers, batch message support, graceful httpx fallback to stub mode. + +--- + +### `tools/apify_mcp.py` (374 lines) + +**Architecture:** MCP stdio server for web scraping via Apify actors with direct-scrape fallback. Includes dataset feeding for Mascarade training. + +| # | Severity | Category | Description | +|---|----------|----------|-------------| +| 1 | Critical | Bug | **FIXED.** `monitor_updates` used `hash()` for change detection. Python's `hash()` is non-deterministic across restarts, causing false positives on every restart. | +| 2 | High | Bug | **FIXED.** `handle_feed_dataset` leaked file handle when counting lines. | +| 3 | Medium | Security | `_run_actor()` (line 85) embeds `APIFY_API_TOKEN` in URL query string: `?token={APIFY_TOKEN}`. If an exception includes the URL (which httpx does by default), the token leaks into error responses sent back via MCP. Consider using an `Authorization` header instead. | +| 4 | Low | Unused import | `sys` (line 10) is imported but never used. | + +**Positives:** Good fallback chain (Apify -> direct scrape -> urllib), HTML stripping for text extraction, StackExchange API integration, JSONL dataset output format. + +--- + +## Recommendations + +### Immediate (no-effort) +- Remove unused `sys` imports from `opcua_mcp.py`, `mqtt_mcp.py`, and `apify_mcp.py` +- Remove unused `Optional` import from `bom_analyzer.py` +- Remove dead `for delim in ...` loop in `bom_analyzer.py:detect_delimiter()` + +### Short-term (low-effort, high-value) +- Add `OPCUA_WRITE_ENABLED` and `MQTT_PUBLISH_ENABLED` env-var gates to prevent accidental writes on production systems +- Move Apify token from URL query parameter to `Authorization: Bearer` header +- Replace hardcoded date in `bom_analyzer.py` report with `datetime.date.today()` + +### Medium-term +- Add input URL validation in `apify_mcp.py` scraping tools (reject non-http(s) URLs, private IPs) +- Add rate limiting to MQTT wildcard topic discovery (`#` subscribe) to avoid flooding +- Consider adding a `--dry-run` flag to `bom_analyzer.py suggest` for CI pipelines diff --git a/tools/apify_mcp.py b/tools/apify_mcp.py index 4881a53..fce6feb 100644 --- a/tools/apify_mcp.py +++ b/tools/apify_mcp.py @@ -4,6 +4,7 @@ from __future__ import annotations import asyncio +import hashlib import json import os import re @@ -281,7 +282,7 @@ async def handle_monitor_updates(args: dict) -> str: cache_dir.mkdir(parents=True, exist_ok=True) cache_file = cache_dir / re.sub(r"[^\w]", "_", url)[:100] text = await _direct_scrape(url) - content_hash = str(hash(text)) + content_hash = hashlib.md5(text.encode()).hexdigest() changed = False if cache_file.exists(): old_hash = cache_file.read_text().strip() @@ -307,7 +308,8 @@ async def handle_feed_dataset(args: dict) -> str: dataset_file = dataset_dir / "scraped.jsonl" with open(dataset_file, "a") as f: f.write(json.dumps(entry, ensure_ascii=False) + "\n") - count = sum(1 for _ in open(dataset_file)) + with open(dataset_file) as fcount: + count = sum(1 for _ in fcount) return json.dumps({"status": "ok", "domain": domain, "file": str(dataset_file), "total_entries": count}) diff --git a/tools/industrial/nodered_connector.py b/tools/industrial/nodered_connector.py index 1c2538b..7877a33 100644 --- a/tools/industrial/nodered_connector.py +++ b/tools/industrial/nodered_connector.py @@ -263,7 +263,10 @@ def _run_stdlib_server() -> None: self._json({"error": "Not found"}, 404) return - length = int(self.headers.get("Content-Length", 0)) + try: + length = int(self.headers.get("Content-Length", 0)) + except (ValueError, TypeError): + length = 0 raw = self.rfile.read(length) try: body = json.loads(raw)