feat(validator): add no-telemetry-import check (PRINCIPLE 10 guarantee)#763
feat(validator): add no-telemetry-import check (PRINCIPLE 10 guarantee)#763justinmclean wants to merge 1 commit into
Conversation
Add check apache#21 (SOFT advisory) to skill-and-tool-validator that flags network-calling imports (requests, httpx, aiohttp, urllib.request, http.client, socket) in substrate:* tool source files under tools/*/src/. Only contract:* adapter tools and the egress-gateway proxy are declared egress surfaces; all other substrate tools must stay network-free to uphold PRINCIPLE 10's guarantee of zero default outbound calls. Also add a Declared egress surfaces section to tools/egress-gateway/tool.md that states the default-zero guarantee, lists the declared egress surfaces, and cross-references the new validator check. Generated-by: Claude (claude-sonnet-4-6)
potiuk
left a comment
There was a problem hiding this comment.
Thanks @justinmclean — a useful guardrail for PRINCIPLE 10, and the happy-path implementation is correct (multi-capability +-split handling, egress-gateway skip, line-anchored regex so comments/strings don't match). My one substantive point is that the scan scope (src/ only) is narrower than the guarantee egress-gateway/tool.md states — details inline, along with a couple of pattern/coverage nits. All non-blocking; the check is safe to ship as a SOFT advisory, but I'd like the scope and the doc wording to agree before it lands.
This review was drafted by an AI-assisted tool and confirmed by a Magpie maintainer. The findings below are observations, not blockers; a Magpie maintainer — a real person — will take the next look at the PR. If you think a finding is mis-applied, please reply on the PR and a maintainer will weigh in.
More on how Magpie handles maintainer review: CONTRIBUTING.md.
| if any(e.startswith(_ADAPTER_CONTRACT_PREFIX) for e in entries): | ||
| continue # declared contract:* adapter — network is expected | ||
|
|
||
| src_dir = tool_dir / "src" |
There was a problem hiding this comment.
Scope vs. the stated guarantee. The scan only descends into tools/<name>/src/, but several substrate tools keep Python at the tool root outside src/ — e.g. pr-management-stats/{dashboard,reference}.py, security-tracker-stats-dashboard/{fetch_roster,fetch_prs,render}.py, dashboard-generator/reference.py. A network import in any of those isn't caught, yet egress-gateway/tool.md promises "a substrate tool that accidentally grows a network import is caught before it is merged." The fetch_*.py files are exactly the surface most likely to grow egress. Consider rglob-ing all *.py under tool_dir (skipping tests/) rather than only src/ — or, if src-only is intentional, soften the guarantee wording in tool.md to match.
| (re.compile(r"^\s*(?:import|from)\s+urllib\.request\b"), "urllib.request"), | ||
| (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"), |
There was a problem hiding this comment.
This pattern is ^\s*import\s+socket\b, so from socket import socket, AF_INET slips through (the other libs use (?:import|from) and are fine). Worth a ^\s*from\s+socket\s+import\b companion. Separately, the list is deliberately tight — urllib3, smtplib, ftplib, boto3, paramiko, grpc, google-cloud all bypass; fine for a heuristic SOFT check, but a # non-exhaustive comment would set expectations. (Minor false-positive note: a tool using socket for local IPC / socket.getfqdn() is flagged as "telemetry" — tolerable given SOFT + --strict-only.)
| 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. |
There was a problem hiding this comment.
This is the strongest form of the guarantee, but check #21 currently scans only each tool's src/ dir — tool-root .py files aren't scanned — and the check is SOFT (advisory unless --strict), so an accidental import merges cleanly without --strict. Either broaden the scan (see the __init__.py comment) or soften this to "flagged by check #21 (SOFT advisory)" so the doc doesn't over-promise.
| ) | ||
|
|
||
|
|
||
| class TestValidateNoTelemetryImports: |
There was a problem hiding this comment.
Nice happy-path suite. A few gaps worth closing: the non-trivial from urllib import …, request regex is untested; aiohttp and http.client have patterns but no test; there's no test for a multi-capability tool (substrate:sandbox + contract:tracker) being skipped; and test_no_src_directory_skipped actually codifies the src-only behavior — a test asserting a tool-root .py import is caught would pin down the scope finding either way.
Summary
Add check #21 (SOFT advisory) to skill-and-tool-validator that flags network-calling imports (requests, httpx, aiohttp, urllib.request, http.client, socket) in substrate:* tool source files under tools/*/src/.
Only contract:* adapter tools and the egress-gateway proxy are declared egress surfaces; all other substrate tools must stay network-free to uphold PRINCIPLE 10's guarantee of zero default outbound calls.
Also add a Declared egress surfaces section to tools/egress-gateway/tool.md that states the default-zero guarantee, lists the declared egress surfaces, and cross-references the new validator check.
Generated-by: Claude (claude-sonnet-4-6)
Type of change
.claude/skills/<name>/) — eval fixtures updated belowtools/<system>/*.md)tools/*/withpyproject.toml)docs/,README.md,CONTRIBUTING.md)projects/_template/)prek, workflows, validators)Test plan
prek run --all-filespassesuv run pytest/ruff check/mypypasses(
PYTHONPATH=tools/skill-evals/src python3 -m skill_evals.runner tools/skill-evals/evals/<skill>/)(a regression test for the bug fixed / the behaviour added — see CONTRIBUTING.md)
RFC-AI-0004 compliance
<PROJECT>,<tracker>,<upstream>,<security-list>) used in all skill / tool prose (thecheck-placeholdersprek hook is the mechanical gate)Linked issues
Notes for reviewers (optional)