diff --git a/tools/egress-gateway/tool.md b/tools/egress-gateway/tool.md index af788957..76ed2099 100644 --- a/tools/egress-gateway/tool.md +++ b/tools/egress-gateway/tool.md @@ -113,11 +113,19 @@ action (PRINCIPLE 10). This guarantee rests on two pillars: all other `substrate:*` tools are network-free by design. 2. **The validator enforces the invariant.** `tools/skill-and-tool-validator/` - check #21 (`no-telemetry-import`) scans the source of every `substrate:*` - tool for network-calling imports (`requests`, `httpx`, `aiohttp`, - `urllib.request`, `http.client`, `socket`) and flags any hit as a SOFT - advisory. A substrate tool that accidentally grows a network import is - caught before it is merged. + check #21 (`no-telemetry-import`) scans every Python file owned by a + `substrate:*` tool — under `src/` and at the tool root alike, excluding + `tests/` — for network-calling imports (`requests`, `httpx`, `aiohttp`, + `urllib.request`, `http.client`, `socket`, in both `import x` and + `from x import` form) and flags any hit as a SOFT advisory. A substrate + tool that accidentally grows a network import is surfaced in the + validator's output before it is merged. + + Two limits worth stating plainly. The check is **advisory unless + `--strict`**, so an accidental import can merge on a run that does not + pass the flag — it is a tripwire the reviewer reads, not a gate that + stops the merge. And the library list is deliberately non-exhaustive: + it catches the common accidents, not a determined author. The declared egress surfaces as of this writing: diff --git a/tools/skill-and-tool-validator/src/skill_and_tool_validator/__init__.py b/tools/skill-and-tool-validator/src/skill_and_tool_validator/__init__.py index be92f262..8e720802 100644 --- a/tools/skill-and-tool-validator/src/skill_and_tool_validator/__init__.py +++ b/tools/skill-and-tool-validator/src/skill_and_tool_validator/__init__.py @@ -399,6 +399,7 @@ (re.compile(r"^\s*from\s+urllib\s+import\s+(?:\w+\s*,\s*)*request\b"), "urllib.request"), (re.compile(r"^\s*(?:import|from)\s+http\.client\b"), "http.client"), (re.compile(r"^\s*import\s+socket\b"), "socket"), + (re.compile(r"^\s*from\s+socket\s+import\b"), "socket"), ] @@ -3637,11 +3638,19 @@ def validate_no_telemetry_imports(root: Path | None = None) -> Iterable[Violatio if any(e.startswith(_ADAPTER_CONTRACT_PREFIX) for e in entries): continue # declared contract:* adapter — network is expected - src_dir = tool_dir / "src" - if not src_dir.is_dir(): - continue - - for py_path in sorted(src_dir.rglob("*.py")): + # Scan every Python file the tool owns, not just ``src/``. Several + # substrate tools keep their Python at the tool root + # (``pr-management-stats/dashboard.py``, + # ``security-tracker-stats-dashboard/render.py``, …), and a + # ``src/``-only scan silently exempted them while + # ``tools/egress-gateway/tool.md`` promised that "a substrate tool + # that accidentally grows a network import is caught before it is + # merged". ``tests/`` is excluded: a test may legitimately import a + # network module to assert it is *not* reachable. + for py_path in sorted(tool_dir.rglob("*.py")): + rel_parts = py_path.relative_to(tool_dir).parts + if "tests" in rel_parts: + continue if any(part in _LICENSE_SKIP_PATH_PARTS for part in py_path.parts): continue try: diff --git a/tools/skill-and-tool-validator/tests/test_validator.py b/tools/skill-and-tool-validator/tests/test_validator.py index f73180ef..361faf15 100644 --- a/tools/skill-and-tool-validator/tests/test_validator.py +++ b/tools/skill-and-tool-validator/tests/test_validator.py @@ -5199,3 +5199,55 @@ def test_violation_message_mentions_principle_10(self, tmp_path: Path) -> None: ) violations = list(validate_no_telemetry_imports(root)) assert any("PRINCIPLE 10" in v.message for v in violations) + + def test_tool_root_python_outside_src_is_scanned(self, tmp_path: Path) -> None: + # Several substrate tools keep Python at the tool root rather than + # under src/ (pr-management-stats/dashboard.py and friends). A + # src/-only scan exempted every one of them while tool.md promised + # the opposite. + root = self._make_tool( + tmp_path, + name="root-script-substrate", + readme=self._SUBSTRATE_README, + src_files={"root_script_substrate/__init__.py": "# SPDX-License-Identifier: Apache-2.0\n"}, + ) + tool_dir = root / "tools" / "root-script-substrate" + (tool_dir / "dashboard.py").write_text("# SPDX-License-Identifier: Apache-2.0\nimport requests\n") + violations = list(validate_no_telemetry_imports(root)) + assert len(violations) == 1 + assert violations[0].path.name == "dashboard.py" + assert "requests" in violations[0].message + + def test_from_socket_import_is_flagged(self, tmp_path: Path) -> None: + # Every sibling pattern accepts both `import x` and `from x import`; + # socket accepted only the former. + root = self._make_tool( + tmp_path, + name="from-socket-substrate", + readme=self._SUBSTRATE_README, + src_files={ + "from_socket_substrate/__init__.py": ( + "# SPDX-License-Identifier: Apache-2.0\nfrom socket import socket\n" + ) + }, + ) + violations = list(validate_no_telemetry_imports(root)) + assert len(violations) == 1 + assert "socket" in violations[0].message + + def test_tests_directory_is_not_scanned(self, tmp_path: Path) -> None: + # A test may legitimately import a network module to assert it is + # not reachable; broadening the scan must not start flagging those. + root = self._make_tool( + tmp_path, + name="tested-substrate", + readme=self._SUBSTRATE_README, + src_files={"tested_substrate/__init__.py": "# SPDX-License-Identifier: Apache-2.0\n"}, + ) + tool_dir = root / "tools" / "tested-substrate" + (tool_dir / "tests").mkdir() + (tool_dir / "tests" / "test_net.py").write_text( + "# SPDX-License-Identifier: Apache-2.0\nimport requests\nfrom socket import socket\n" + ) + violations = list(validate_no_telemetry_imports(root)) + assert violations == []