Repository navigation
feat(cli): add cochange, check (with SARIF) and dead-code graph analyses - #4544
omsenjalia wants to merge 3 commits into
Conversation
Three read-only commands over graph.json, each in its own module: `graphify cochange` (graphify/cochange.py) Compares `git log --relative --name-only` co-change with graph edges. Hidden coupling = file pairs that changed together >= --min-count times but share no graph edge; hotspots = commits x linked files. Only files already in the graph are considered, so the corpus boundary stays the detection/.graphifyignore one. Bulk commits are skipped. git runs as an argv list; the record separator uses git's %x1e escape because a raw control byte in argv breaks on Windows. `graphify check` (graphify/rules.py) graphify-rules.json declares forbidden dependencies (fnmatch globs, optional relation filter) and whether import cycles are allowed. Exit 0 clean / 1 violations / 2 bad input; --sarif writes SARIF 2.1.0 for GitHub code scanning. Fail-closed: only EXTRACTED code-to-code edges count unless "include_inferred": true; AMBIGUOUS edges and document mentions never count (counting everything produced 45 bogus hits on graphify's own graph, the default 0). Cycles reuse find_import_cycles. `graphify dead-code` (graphify/deadcode.py) Forward reachability over usage edges from non-callable code nodes and entry-point-shaped callables; unreached AST callables are candidates. Catches islands that only call each other. Conservative where the AST pass leaves calls unresolved: nested functions and methods inherit liveness from their parent, overriding methods / dunders / main / test names and test files are never candidates, documents never make code live, AMBIGUOUS edges still count as usage. On graphify itself it finds deduplicate_by_label, prune_dangling_edges, _auto_follow_symlinks and node_in_section, which have no callers. Heuristic limits are documented in each module docstring. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Thanks for the pull request, @omsenjalia. 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.
Worth a look — the grounded gate found no coupling regressions or blocking issues, but 3 advisory finding(s) below merit a look before merge.
Not checked: tests were not run; formal verification proved 0 of 2 changed function(s) (1 sampled, not proven, 1 not verified).
Formal verification. PR-changed functions: 1/2 verified (0 proven, 1 may-equivalent, 0 distinguished) · 1 not verified (1 vacuous).
Not verified on this run: dispatch\_command (vacuous: never exercised).
Graphify review — findings
Adds three graph-analysis subcommands: cochange compares git history against graph edges to surface hidden coupling and churn hotspots, check enforces architecture rules from graphify-rules.json (exit 1 on violations, 2 on an unreadable graph or rules, optional SARIF output), and dead-code lists callables no live code reaches. The check path only catches ValueError and RuntimeError from loading rules, and cochange leans on git log plus _norm path matching, so an unreadable rules file, a missing git binary, or Windows-style source_file paths can break those commands outside their documented exits. Forbid rules in check_rules do not account for edge direction on undirected graphs, so a gate that fails the build can flag the wrong side of a dependency, and the new help text in _run_cli documents all three commands.
Worth a look
- Forbid rules ignore edge direction on undirected graphs —
graphify/rules.py:86· Escalate · high- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
- check command lets unreadable rules OSErrors escape instead of exiting 2 —
graphify/cli.py:1860· Escalate · medium- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
- Windows-style source_file paths are not normalized to POSIX —
graphify/cochange.py:39· Escalate · medium- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
Review partial — this diff was larger than one review pass covers, so later files were not reviewed; some findings may be missing.
Analysis details — impact, health, verification
Impact & health
Graphify review
Impact — 496 functions depend on the 174 functions this change touches.
Health — this change adds coupling hotspots:
- worse:
dispatch_command()— 5 callers, 143 callees - worse:
main()— 113 callers, 3 callees - new:
dead_code_candidates()— 6 callers, 4 callees - new:
cochange_analysis()— 6 callers, 3 callees
Verification — 496 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: 496 function(s) in the blast radius were not formally verified this run
Test selection
Test selection
40 of 435 test file(s) selected (9%) via static blast radius.
tests/test_affected_cli.py— impacttests/test_agents_platform.py— impacttests/test_check_rules.py— impact, changed-testtests/test_cli_export.py— impacttests/test_cli_help.py— changed-testtests/test_cloud_cta.py— impacttests/test_cochange.py— impact, changed-testtests/test_codebuddy.py— impacttests/test_compact_graph_json.py— impacttests/test_dead_code.py— impact, changed-testtests/test_dedup_shrink_refuses_force_write.py— impacttests/test_devin.py— impacttests/test_explain_cli.py— impacttests/test_explain_same_file_tie.py— impacttests/test_extract_cli.py— impacttests/test_global_add_tag_inference.py— impacttests/test_god_nodes_cli.py— impacttests/test_hollow_chunks_arm_shrink_guard.py— impacttests/test_hook_guard_token_match.py— impacttests/test_hook_out_of_project_paths.py— impacttests/test_hook_strict.py— impacttests/test_incomplete_build_guard.py— impacttests/test_install.py— impacttests/test_install_references.py— impacttests/test_install_version_warning.py— impacttests/test_lookup_exit_codes.py— impacttests/test_merge_chunks_validation.py— impacttests/test_multigraph_diagnostics.py— impacttests/test_no_dedup_flag.py— impacttests/test_opencode_plugin_refresh.py— impacttests/test_partial_cache.py— impacttests/test_path_cli.py— impacttests/test_query_cli.py— impacttests/test_query_induced_edges.py— impacttests/test_query_seeds.py— impacttests/test_skill_auto_refresh.py— impacttests/test_skill_version_warning.py— impacttests/test_stale_prune.py— impacttests/test_truncation_full_answer.py— impacttests/test_unverified_semantic_shrink.py— impact
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.
Docs that may be stale (advisory)
BENCHMARKS.md§ Harness (lines 37-57): references changed symbolsrepoBENCHMARKS.md§ Reproducing (lines 190-203): references changed symbolsrepoCHANGELOG.md§ 0.9.81 (2026-10-08) (lines 205-223): references changed symbolsrepoCHANGELOG.md§ 0.9.55 (2026-09-05) (lines 555-569): references changed symbolsrepoCHANGELOG.md§ 0.9.49 (2026-08-24) (lines 637-660): references changed symbolsrepoCHANGELOG.md§ 0.9.47 (2026-08-19) (lines 672-688): references changed symbolsrepoCHANGELOG.md§ 0.9.38 (2026-08-09) (lines 793-801): references changed symbolsrepoCHANGELOG.md§ 0.8.36 (2026-06-08) (lines 1494-1505): references changed symbols_normCHANGELOG.md§ 0.8.16 (2026-05-22) (lines 1671-1678): references changed symbols_normCHANGELOG.md§ 0.7.7 (2026-05-05) (lines 1914-1922): references changed symbolsrepo
…and 10 more.
Formal verification
No difference found (not proven): No behavior difference found in \_run\_cli (not a proof).
The verifier ran both versions of \_run\_cli on many inputs and saw identical behavior every time. Strong evidence the change is safe, but evidence, not a proof.
Guarantee: Empirical: differential testing (both versions run on many generated inputs). A divergence on an untested input remains possible, so this is 'no counterexample found', not 'proven equivalent'.
Note: An input the sampler did not try could still differ.
Could not verify: Could not verify dispatch\_command.
The verifier did not have enough to check dispatch\_command, so it is saying so rather than guessing. No false assurance is the whole point.
Guarantee: No guarantee either way, this is an honest abstention, not a pass.
Note: Reason: no capturable inputs from the test suite; property tier: not verifiable: all 40 sampled inputs raised on both versions — the function never executed, so 'no divergence' would be vacuous (mostly SystemExit — names the real obstacle, not a sampling gap)
· 2 grounded finding(s) anchored inline below; 2 more finding(s) on lines outside this diff (see the check run).
- cochange: normalize backslashes before PurePath so a Windows-written source_file matches git's POSIX paths when run on POSIX - cochange: a missing git binary raises RuntimeError (exit 1 with a message) instead of an uncaught FileNotFoundError - check: an unwritable --sarif path exits 2 instead of a traceback - tests pin rules-file-is-a-directory -> exit 2, and that an undirected graph.json keeps dependency direction (to_json writes _src/_tgt into source/target; load_graph reads it back directed) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
check_rules matched `from` against G.edges()' first endpoint. build() returns an undirected nx.Graph by default, whose edges iterate in node insertion order, so a ui -> db call could come back as (db, ui) and the violation was silently missed when check_rules was called on an in-memory graph. Use the _src/_tgt attributes build() records (same idiom as export.py). Not a both-orders match: that would flag the allowed reverse dependency. The CLI path was unaffected (graph.json stores the true direction and load_graph reads it directed). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
What does this PR do?
It adds three read-only analysis commands over
graph.json. Each lives in its own module with its own tests, and they share only the CLI wiring. They're bundled here at my request; I'm happy to split them into three PRs if reviewers prefer.graphify cochangegraphify/cochange.pygraphify checkgraphify/rules.pygraphify-rules.json? Exits non-zero and can emit SARIF for GitHub code scanninggraphify dead-codegraphify/deadcode.pyPrior art check: I searched open and closed PRs and issues for co-change/churn/hotspots, SARIF/architecture rules/forbidden imports, and dead code/reachability. Nothing overlaps. The closest items are #2950 (ingesting external OpenGrep findings, the opposite direction from
check) and #2649 (removing some unused code by hand rather than adding an analysis).All three outputs below come from running the commands on this repository.
graphify cochangegit log --no-merges --relative --name-onlyas an argv list, with no shell.--relativelines up a graph built from a subdirectory with git's paths, andcore.quotepath=falsekeeps non-ASCII paths readable. The record separator uses git's%x1eescape because a raw control byte in argv breaks on Windows.source_fileare considered, so detection and.graphifyignorestay the single definition of the corpus.graph.json: co-change is a statistical signal, never an edge.--since,--max-commits(default 500),--min-count(3),--max-files-per-commit(30),--top(20),--graph,--json.graphify check{ "forbid": [ {"from": "src/ui/*", "to": "src/db/*", "message": "UI must go through the service layer"}, {"from": "src/core/*", "to": "src/plugins/*", "relations": ["imports_from"]} ], "no_import_cycles": true, "max_cycle_length": 5, "include_inferred": false }0no violations,1violations,2unreadable graph or invalid rules file.--sarif PATHwrites SARIF 2.1.0 thatgithub/codeql-action/upload-sarifaccepts. The rules file defaults to<root>/graphify-rules.json; use--rulesto point elsewhere.affected.DEFAULT_AFFECTED_RELATIONSminusdefines. Cycle checks reuseanalyze.find_import_cycles, so the deferred and type-only exclusions still apply.EXTRACTEDcode-to-code edges count."include_inferred": trueopts inINFERREDedges;AMBIGUOUSedges and document mentions never count. Why: on graphify's own graph, agraphify/* → tests/*rule that counted every edge produced 45 bogus violations (31 INFERRED doc references and 12 INFERREDindirect_callresolutions). The default produces 0.fnmatchon repo-relative POSIX paths, and*also matches/. Unindexed files and unresolved calls can't violate a rule. SARIF URIs are relative to the scanned root.graphify dead-codededuplicate_by_label,prune_dangling_edges,_auto_follow_symlinksandnode_in_section; a repo-wide grep confirms none of them has a caller. This PR doesn't delete them.affected._overridden_members), dunders,mainandtest*names, and anything in a test file are never candidatesAMBIGUOUSedges still count as usageVerification
pyright graphify/cli.pyreports "dispatch_command is too complex to analyze". That error is identical on unmodifiedv8and not introduced here.Determinism: every output is sorted on stable keys (file, line, label). No command writes graph or cache state.
An AI coding assistant (Claude) materially contributed to this change; disclosed via the
Co-Authored-Bytrailer.🤖 Generated with Claude Code