Repository navigation
Conversation
7 security tests needed real DNS through validate_url, and 11 skillgen tests read blobs from origin/v8 and died when the checkout didn't have it.
|
Thanks for the pull request, @bercedev. A maintainer will review it soon. Want to talk it through while it is in review? Come join us on our Discord server. For longer-form discussion there is also GitHub Discussions. A couple of things that speed up review: make sure the test suite passes on Python 3.10 and 3.13, and that the change keeps extraction deterministic. |
There was a problem hiding this comment.
Graphify reviewed this change.
No issues found by static checks — no coupling regressions or blocking issues in the code graph. That is not the same as safe to merge: see what was not checked.
Not checked: tests were not run; no formal proof of the changed code.
Graphify review — findings
Stops two test suites from depending on the environment: the URL/fetch tests no longer need a network, and the skillgen audit/roundtrip tests skip cleanly when origin/v8 isn't in the checkout. - test_security.py: a new public_dns fixture stubs socket.getaddrinfo to return a public IP. The validate_url and safe_fetch* tests that reach the host-resolution step use it, so they no longer do real DNS lookups. - test_skillgen.py: a needs_origin_v8 marker, backed by gen._v8_available(), skips every test that reads v8 blobs (audit-coverage, roundtrip, sanctioned-lines, always-on byte-faithfulness) on forks or tarball checkouts. CI uses fetch-depth: 0, so they still run there.
No blocking issues surfaced.
Analysis details — impact, health, verification
Impact & health
Graphify review
Impact — 232 functions depend on the 232 functions this change touches.
Health — grade A; 1 existing hotspot(s) in the area this change touches (pre-existing, not introduced here):
test_audit_catches_a_dropped_non_allowlisted_heading()— 0 callers, 6 callees (medium)
Verification — 232 functions in the blast radius were not formally verified this run (proofs are advisory here).
Gate & verification
graphify gate
PASS — objectively clean (no health regressions, tests not run — proofs not run this pass (advisory)). Grounded, not self-assessed.
Advisory (not blocking):
- verification_scope: 232 function(s) in the blast radius were not formally verified this run
Test selection
Test selection
2 of 469 test file(s) selected (0%) via static blast radius.
tests/test_security.py— impact, changed-testtests/test_skillgen.py— impact, changed-test
Selection is safe under the controlled-regression assumption; always-run tests + a periodic full run are the backstops. Advisory — it never changes the check verdict.
What does this PR do?
On a clean v8,
uv run pytest testshas 18 failures when the checkout has no network or noorigin/v8, for example a fork whereoriginisn't this repo. CONTRIBUTING says tests must not depend on the contributor's environment, so this makes them not depend on it.test_security.py:validate_urlresolves the host, soexample.comandarxiv.orgneeded real DNS and failed withValueError: DNS resolution failed. A newpublic_dnsfixture fakessocket.getaddrinfothe waytest_ollama.pyalready does. It's only on those 7, the SSRF tests aren't touched.test_skillgen.py: they read blobs fromorigin/v8and died withSystemExit: error: could not read origin/v8:.... They now skip withneeds_origin_v8, which uses the samegen._v8_available()probe the CLI validators already skip on.Nothing changes under
graphify/ortools/.Limitation: if CI ever lost
fetch-depth: 0, these 11 would skip and not fail. The CLI validators inskillgen-checkalready behave that way.Type of change
Verification & Invariants
Invariant: a test result doesn't depend on the network or on local refs. With
origin/v8present nothing gets skipped, so the audits still guard what they did.How was this tested?
Graphify-specific checklist
uv run python -m tools.skillgen --bless) when changing their source fragments.Note: I used Claude Code to write this and ran the tests above locally.