ci: add graphify structural impact analysis to PR review and structure audit#567
ci: add graphify structural impact analysis to PR review and structure audit#567andreatgretel wants to merge 8 commits intomainfrom
Conversation
…e audit Add a graphify-based AST analysis tool that builds a directed graph of the codebase (~2s, no LLM calls) to detect architectural impact. Integrates into both the PR review workflow (pre-computed before claude runs) and the Wednesday structure audit (with week-over-week diff). PR review: extracts changed files against the full codebase graph, reports risk level (LOW/MEDIUM/HIGH), god nodes affected, import direction violations, and cross-package dependencies. Output saved to /tmp and read by the review agent. Structure audit: produces god node rankings, cross-package edge summary table, import violation detection, and graph diff against previous week's cached graph. Baselines saved for runner memory trend tracking.
- Fix KeyError: god_nodes() returns 'degree' not 'edges' (3 call sites) - Fix deduped vs raw violation count inconsistency in baselines.json - Security: run structural_impact.py from base-branch checkout so fork PRs cannot inject code that executes with GH_TOKEN in scope - Add --repo-root flag so the tool resolves package paths correctly when invoked from a different checkout directory - Replace make install-dev + .venv with lightweight /tmp/graphify-venv (only graphifyy needed, saves ~2min CI per PR review) - Add graphify-out/ to .gitignore (9MB graph cache is CI-only)
andreatgretel
temporarily deployed
to
agentic-ci
GitHub Actions
Inactive
SummaryPR #567 adds a graphify-based AST structural-impact tool and wires it into two CI workflows:
Retroactive validation against 3 merged PRs is cited in the description; signal correlation with actual critical findings looks plausible. FindingsSecurity — correctly handled, one residual concernThe critical security story (tool must run from trusted base-branch code, not PR code) is executed well:
One residual concern in CHANGED_PY=$(gh pr diff "$PR_NUMBER" --name-only | grep '\.py$' || true)
...
--changed-files $CHANGED_PY \
mapfile -t CHANGED_PY < <(gh pr diff "$PR_NUMBER" --name-only | grep '\.py$' || true)
...
--changed-files "${CHANGED_PY[@]}"This also fixes a latent bug: if Correctness
Style / project conventions
Test coverage
Performance
Minor / nits
VerdictApprove with minor follow-ups. The core design — trusted-path execution of a pure-AST tool with Recommended before merge:
Recommended after merge: No blocking issues; security model is correct; retroactive validation supports the signal claim. |
Greptile SummaryThis PR integrates a
|
| Filename | Overview |
|---|---|
| .agents/tools/structural_impact.py | New 411-line AST graph analysis tool; all previously flagged bugs (dedup truncation, risk reason, changed_node_ids derivation) addressed in prior commits; remaining code logic is correct. |
| .github/workflows/agentic-ci-pr-review.yml | Adds structural impact analysis step; security model correct (tool read from base-branch checkout); graphifyy pinned to 0.4.23; continue-on-error guards against analysis failures; path handling and mapfile usage correct. |
| .github/workflows/agentic-ci-daily.yml | Adds conditional graphifyy install for structure suite and extends cache path to include graphify-out/ for week-over-week diffs; changes are minimal and correct. |
| .agents/recipes/pr-review/recipe.md | Adds step to read pre-computed structural impact file and append it to the review output; integration instructions are clear and correct. |
| .agents/recipes/structure/recipe.md | Adds Section 4 with graphify run instructions, god node tracking guidance, and baseline update steps; template is coherent and consistent with tool's output format. |
| .agents/skills/review-code/SKILL.md | Adds Step 3.5 describing how the agent should consume the structural impact file; guidance is well-structured and clearly scoped (HIGH/MEDIUM/LOW risk calibration). |
| .gitignore | Adds graphify-out/ to .gitignore; correct since the cache directory is CI-only and persisted via actions/cache. |
Sequence Diagram
sequenceDiagram
participant GHA as GitHub Actions
participant BC as base-agents/ checkout
participant WS as PR workspace
participant Agent as Review Agent
GHA->>GHA: gh pr diff --name-only | grep .py → /tmp/changed-py.txt
GHA->>BC: sparse-checkout .agents/recipes + .agents/tools (base branch)
GHA->>BC: python base-agents/.agents/tools/structural_impact.py
Note over BC,WS: --repo-root points to PR workspace<br/>tool reads PR source, runs from base branch
BC->>WS: extract(all_source_files) → build graph G
BC->>BC: derive changed_node_ids from G.nodes[source_file]
BC->>BC: assess risk (god nodes / violations / clusters)
BC->>GHA: write /tmp/structural-impact-{pr}.md
GHA->>Agent: start review agent
Agent->>Agent: read /tmp/structural-impact-{pr}.md
Agent->>Agent: calibrate review depth (HIGH→extra pass 2 scrutiny)
Agent->>Agent: /review-code {pr_number}
Agent->>Agent: append Structural Impact section to review file
Reviews (4): Last reviewed commit: "feat(ci): warn when changed files touch ..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Pin graphifyy==0.4.23 in both CI workflows to prevent breaking changes from unpinned installs. Fix _dedup() label truncation at 30 chars that could merge distinct entities sharing a common prefix.
…filenames Replace unquoted $CHANGED_PY word-split with mapfile + array expansion to prevent glob expansion and correctly handle filenames containing spaces or special characters.
Derive changed_node_ids from the already-built graph by matching source_file paths instead of running a separate extraction pass. Removes implicit dependency on graphify ID stability across independent extractions. Fix MEDIUM risk reason to reflect the actual trigger (cluster spread vs high-connectivity entity) instead of always reporting cluster count.
…stale artifacts Split the workflow step to isolate GH_TOKEN from graphifyy execution, preventing a compromised package release from exfiltrating write-scoped tokens. Scan both edge directions in _cross_package_edges so inbound dependents and violations where the changed node is the target are visible. Detect deleted files and report them as a risk signal. Include relation type in dedup key so distinct edge types between the same labels are not collapsed. Clean stale /tmp artifacts before running analysis to prevent reruns from reading old reports.
|
Thanks for putting this together, @andreatgretel — the retroactive validation against three merged PRs is a really nice way to make the case for the signal quality. SummaryAdds a graphify-based AST analyzer at FindingsWarnings — Worth addressing
from dataclasses import dataclass
from typing import Any
@dataclass(frozen=True, slots=True)
class _Analysis:
graph: Any
communities: dict[int, list[str]]
cohesion: dict[str, Any]
god_nodes: list[dict[str, Any]]
def _build_graph(files: list[Path]) -> _Analysis: ...
def _cross_package_edges(G: Any, node_ids: set[str] | None = None) -> ...: ...Call sites become
Suggestions — Take it or leave it
- name: Install graphify
if: matrix.suite == 'structure'
run: |
python -m venv /tmp/graphify-venv
/tmp/graphify-venv/bin/python -m pip install graphifyy==0.4.23 2>&1 | tail -3Then invoke
What Looks Good
VerdictShip it (with nits) — Only Warnings around type annotations and the hardcoded package list, plus a handful of Suggestions. Nothing blocking, but the type annotations are worth addressing in this PR since they set the convention for the new file. Everything else can land as-is or in a follow-up. This review was generated by an AI assistant. |
…w except, isolate daily graphify
- structural_impact.py:
- replace bare _build_graph dict return with frozen _Analysis dataclass
- add G: Any annotation on _cross_package_edges (STYLEGUIDE: all params typed)
- hoist `from graphify.export import to_json` and
`from networkx.readwrite import json_graph` to module top
(no perf justification for deferred import)
- narrow `except Exception` in graph-diff fallback to
(JSONDecodeError, KeyError, TypeError, OSError)
- agentic-ci-daily.yml: install graphifyy into /tmp/graphify-venv instead of
the project .venv, matching agentic-ci-pr-review.yml. Keeps graphify's
transitives (networkx) out of the project venv permanently.
- structure/recipe.md: invoke the tool via /tmp/graphify-venv/bin/python
to match the workflow change.
A new package under packages/ that isn't in _PACKAGE_SUBDIRS is silently absent from the graph - the analyzer would falsely report LOW risk with 0 entities. Add a _Note line in the changed-files report when any changed or deleted file lives under packages/<unknown>/, so the failure mode the analyzer is supposed to surface isn't itself silent. _KNOWN_PACKAGE_DIRS is derived from _PACKAGE_SUBDIRS so future additions stay in sync without a second source of truth.
|
Thanks @nabinchha, this was a careful review and the type-annotation push set the right precedent for the file. Almost everything went into two follow-up commits on the branch: 508f119 — STYLEGUIDE-cited fixes + daily venv isolation
07b0e74 — unknown-package hedge
Deferred to #601 — unit tests for |
2 participants
📋 Summary
Add a graphify-based AST analysis tool that builds a
directed graph of the codebase (~6s, zero LLM cost) to surface architectural risk before the
review agent starts. Integrates into both the PR review workflow (pre-computed context for the
agent) and the Wednesday structure audit (week-over-week graph diff).
Hat tip to @nabinchha for suggesting graphify as the right tool for this.
🔗 Related Issue
Closes #472 (Phase 4 — structural analysis)
🔄 Changes
✨ Added
structural_impact.py—standalone analysis tool with two modes:
--changed-files(PR review): extracts changed AST entities against the full codebase graph,reports risk level (LOW/MEDIUM/HIGH), god nodes affected, import direction violations, and
cross-package dependencies
--full(structure audit): god node rankings, cross-package edge summary table, importviolation detection, and graph diff against the previous week's cached graph
review-codeskill —agent reads the pre-computed structural impact to calibrate review depth (HIGH risk = extra
scrutiny on blast radius and backward compatibility)
graphify-out/added to.gitignore(9MB graph cache is CI-only, persisted via actions/cache)🔧 Changed
agentic-ci-pr-review.yml—new step installs graphifyy in a lightweight temp venv and runs structural analysis before the
agent starts. Tool is read from the base-branch checkout (
base-agents/) so fork PRs cannotinject code that runs with
GH_TOKENin scope. Base-branch checkout expanded from.agents/recipesto also include.agents/toolsagentic-ci-daily.yml—conditional graphifyy install for the structure suite;
graphify-out/added to the cache pathfor week-over-week graph diffs
pr-reviewrecipe —agent reads
/tmp/structural-impact-{{pr_number}}.mdand appends findings to the reviewstructurerecipe —new Section 4 with graphify instructions, baseline tracking, and week-over-week diff guidance
🔍 Attention Areas
agentic-ci-pr-review.yml—the security model:
structural_impact.pyruns frombase-agents/(base-branch sparsecheckout), not from the PR's files, to prevent fork PRs from injecting code that executes
with secrets in scope. The
--repo-rootflag ensures the tool still analyzes the PR'ssource code correctly despite living in a different checkout
structural_impact.py—new 361-line tool; pure AST analysis, no DD imports at runtime. Depends on
graphifyy(tree-sitter based) andnetworkx(transitive)📊 Retroactive validation on 3 merged PRs
Ran graphify retroactively against PRs #502, #545, and #557 to validate the signal quality:
DatasetBuilder(88 deps) +ExecutionGraph(89 deps) flagged as highest-riskconfig.__getattr__()calls into engineIn all 3 cases, the most critical review findings correlated with the highest-connectivity
entities in the graphify report. The risk level (LOW/MEDIUM/HIGH) matched actual review
effort needed.
🧪 Testing
structural_impact.pysmoke-tested in both modes (--changed-files,--full)against the live DD codebase
--repo-rootflag tested from external checkout directory (simulates CIbase-agents/path)graphifyy==0.4.23✅ Checklist
.agents/tooling, not package architecture