Skip to content

fix(validator): scan tool-root Python and 'from socket import' for telemetry - #1070

Open
potiuk wants to merge 1 commit into
apache:mainfrom
potiuk:fix/telemetry-scan-scope
Open

fix(validator): scan tool-root Python and 'from socket import' for telemetry#1070
potiuk wants to merge 1 commit into
apache:mainfrom
potiuk:fix/telemetry-scan-scope

Conversation

@potiuk

@potiuk potiuk commented Aug 5, 2026

Copy link
Copy Markdown
Member

Summary

Closes #783.

tools/egress-gateway/tool.md promised that "a substrate tool that accidentally grows a network import is caught before it is merged." For 11 Python files it did not.

  • Scan every .py the tool owns, not just tools/<name>/src/. Several substrate tools keep Python at the tool root — pr-management-stats/{dashboard,reference}.py, security-tracker-stats-dashboard/{fetch_roster,fetch_prs,render}.py, dashboard-generator/reference.py — and a src/-only scan exempted every one of them. tests/ stays excluded: a test may legitimately import a network module to assert it is unreachable.
  • Add the from socket import companion pattern. Every sibling pattern already accepted both import x and from x import; socket accepted only the former. Clearly an oversight rather than a decision.
  • Correct the tool.md guarantee so it states the real scope and the two limits it previously implied away.

Type of change

  • Skill change (.claude/skills/<name>/) — eval fixtures updated below
  • Tool / bridge contract (tools/<system>/*.md)
  • Python package (tools/*/ with pyproject.toml)
  • Groovy reference impl
  • Cross-cutting (RFC, AGENTS.md, sandbox, privacy-LLM)
  • Documentation (docs/, README.md, CONTRIBUTING.md)
  • Project template (projects/_template/)
  • CI / dev loop (prek, workflows, validators)

Test plan

  • prek run --files green on all three files (ruff, ruff-format, mypy, pytest, markdownlint, typos, lychee, skill-and-tool-validate)

  • uv run --directory tools/skill-and-tool-validator --group dev pytest passes

  • Before/after proven by planting the defects. import requests added to tools/pr-management-stats/dashboard.py and from socket import socket to reference.py — both real tool-root files:

    Validator Violations found
    original (main) 0 — both missed
    this PR 2 — both caught
  • Clean tree is 0 violations before and after, so no existing tool is newly flagged and this does not turn CI red on merge

  • Three tests added; the two behavioural ones fail on the previous implementation with assert 0 == 1

RFC-AI-0004 compliance

  • HITL — any new mutation is gated on explicit user confirmation
  • Sandbox — this is the check backing PRINCIPLE 10's zero-default-egress guarantee; the change widens enforcement to the files that were silently exempt
  • Vendor neutrality
  • Conversational + correctable
  • Write-access discipline
  • Privacy LLM — the no-telemetry invariant is what stops a substrate tool quietly acquiring an outbound call

Linked issues

Closes #783. Follow-up from the review of #763.

Notes for reviewers

On the tool.md rewording. I first considered only widening the scan and leaving the prose. That would still have overclaimed: the check is SOFT unless --strict, so an accidental import can merge on a run without the flag. The doc now says plainly that it is "a tripwire the reviewer reads, not a gate that stops the merge", and that the library list is deliberately non-exhaustive. A guarantee that overstates its own strength is worse than a narrower one stated accurately — someone reads it and stops looking.

On the tests/ exclusion. Deliberate, and the third test pins it. Broadening the scan without it would have started flagging test files that import requests precisely to assert it cannot reach anywhere — a check that fires on its own negative controls trains people to ignore it.

Verification method. The clean tree reports 0 violations both before and after, so a passing run proves nothing on its own. The planted-defect comparison in the test plan above is what establishes that the broadened scan actually reaches the new files.

…lemetry

Closes apache#783.

tool.md promised that 'a substrate tool that accidentally grows a network
import is caught before it is merged'. For 11 Python files it did not.

- Scan every .py the tool owns, not just tools/<name>/src/. Several
  substrate tools keep Python at the tool root -- pr-management-stats/
  {dashboard,reference}.py, security-tracker-stats-dashboard/
  {fetch_roster,fetch_prs,render}.py, dashboard-generator/reference.py --
  and a src/-only scan exempted all of them. tests/ stays excluded: a test
  may legitimately import a network module to assert it is unreachable.

- Add the 'from socket import' companion pattern. Every sibling pattern
  already accepted both forms; socket accepted only 'import socket'.

- Correct the tool.md guarantee. It now states the real scope and two
  limits it previously implied away: the check is advisory unless
  --strict, and the library list is non-exhaustive. It is a tripwire the
  reviewer reads, not a gate that blocks the merge.

Verified by planting both defects in real tool-root files: the original
validator reported 0 violations, the fixed one reports 2. Clean tree is 0
before and after, so no existing tool is newly flagged.

Three tests added. The two behavioural ones fail on the previous
implementation with 'assert 0 == 1'; the tests/-exclusion one is a guard
against a future scan broadening that would start flagging test files.

Generated-by: Claude Code (Opus 5)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

no-telemetry-import validator: broaden scan beyond src/ to match the PRINCIPLE 10 guarantee wording

1 participant