Skip to content

Commit 596f4e2

Browse files
authored
Merge pull request #5 from Modsofthenation/cursor/review-accuracy-ccb4
Score tests on the impact path and prefer generated OpenAPI clients
2 parents a5f4a75 + ab99fa0 commit 596f4e2

12 files changed

Lines changed: 533 additions & 55 deletions

File tree

src/loadpath/architecture/rules.py

Lines changed: 50 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -296,19 +296,40 @@ def _task_idempotency(store: GraphStore, changed_ids: set[str] | None) -> list[F
296296
return out
297297

298298

299+
REL_FIELD_TYPES = {
300+
"ForeignKey",
301+
"OneToOneField",
302+
"ManyToManyField",
303+
"GenericForeignKey",
304+
"GenericRelation",
305+
}
306+
307+
299308
def _nplusone(store: GraphStore) -> list[Finding]:
300309
out: list[Finding] = []
310+
fields = list(store.nodes([NodeType.FIELD]))
311+
fields_by_name: dict[str, list[dict]] = {}
312+
for field in fields:
313+
fields_by_name.setdefault(field["name"], []).append(field)
301314
for node in store.nodes():
302315
hits = (node.get("extra") or {}).get("nplusone") or []
316+
owner_app = (node.get("extra") or {}).get("app")
303317
for hit in hits:
304-
accessed = ", ".join(hit.get("accessed") or []) or "related fields"
318+
accessed = list(hit.get("accessed") or [])
319+
related, conf = _related_accesses(accessed, fields_by_name, owner_app)
320+
if not related:
321+
continue
322+
hit = dict(hit)
323+
hit["accessed"] = related
324+
hit["confidence"] = conf
325+
accessed_s = ", ".join(related)
305326
fix = hit.get("suggested_fix") or ".select_related()"
306327
out.append(
307328
Finding(
308329
rule="queryset_nplusone",
309330
severity=RuleSeverity.WARNING,
310331
message=(
311-
f"{node['name']} loops `{hit.get('loop_var')}` over a queryset and touches {accessed} "
332+
f"{node['name']} loops `{hit.get('loop_var')}` over a queryset and touches {accessed_s} "
312333
f"without {fix} ({node.get('file_path')}:{hit.get('line')})"
313334
),
314335
node_id=node["id"],
@@ -319,6 +340,33 @@ def _nplusone(store: GraphStore) -> list[Finding]:
319340
return out
320341

321342

343+
def _related_accesses(
344+
accessed: list[str], fields_by_name: dict[str, list[dict]], owner_app: str | None
345+
) -> tuple[list[str], str]:
346+
related: list[str] = []
347+
unknown = False
348+
for name in accessed:
349+
matches = fields_by_name.get(name) or []
350+
if owner_app:
351+
scoped = [f for f in matches if (f.get("extra") or {}).get("app") == owner_app]
352+
if scoped:
353+
matches = scoped
354+
if not matches:
355+
related.append(name)
356+
unknown = True
357+
continue
358+
if any(_is_relation(f) for f in matches):
359+
related.append(name)
360+
return related, ("medium" if unknown else "high")
361+
362+
363+
def _is_relation(field: dict) -> bool:
364+
extra = field.get("extra") or {}
365+
if extra.get("relation"):
366+
return True
367+
return extra.get("field_type") in REL_FIELD_TYPES
368+
369+
322370
def _missing_index(store: GraphStore) -> list[Finding]:
323371
out: list[Finding] = []
324372
fields_by_name: dict[str, list[dict]] = {}

src/loadpath/extractors/django_boot.py

Lines changed: 121 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,15 +2,136 @@
22

33
from __future__ import annotations
44

5+
import json
56
import os
7+
import subprocess
68
import sys
79
from pathlib import Path
810

911
from loadpath.config import LoadpathConfig
1012
from loadpath.types import Edge, EdgeType, ExtractedGraph, Node, NodeType, node_id
1113

14+
BOOT_JSON_MARKER = "__LOADPATH_BOOT_JSON__"
15+
1216

1317
def try_boot_models(repo_root: Path, config: LoadpathConfig) -> ExtractedGraph:
18+
"""Boot Django in a subprocess so django.setup() is not process-global."""
19+
if os.environ.get("LOADPATH_BOOT_INPROCESS") == "1":
20+
return _boot_inprocess(repo_root, config)
21+
return _boot_subprocess(repo_root, config)
22+
23+
24+
def _boot_subprocess(repo_root: Path, config: LoadpathConfig) -> ExtractedGraph:
25+
src_root = Path(__file__).resolve().parents[2]
26+
env = os.environ.copy()
27+
env["LOADPATH_BOOT_INPROCESS"] = "1"
28+
env["PYTHONPATH"] = str(src_root) + os.pathsep + env.get("PYTHONPATH", "")
29+
payload = json.dumps(
30+
{
31+
"repo_root": str(repo_root.resolve()),
32+
"django_root": config.django_root,
33+
}
34+
)
35+
code = (
36+
"import io,json,sys\n"
37+
"from contextlib import redirect_stdout\n"
38+
"from pathlib import Path\n"
39+
"from loadpath.config import load_config\n"
40+
"from loadpath.extractors.django_boot import _boot_inprocess\n"
41+
"meta=json.loads(sys.argv[1])\n"
42+
"root=Path(meta['repo_root'])\n"
43+
"cfg=load_config(root)\n"
44+
"cfg.django_root=meta['django_root']\n"
45+
"cfg.boot_django=True\n"
46+
"buf=io.StringIO()\n"
47+
"with redirect_stdout(buf):\n"
48+
" g=_boot_inprocess(root,cfg)\n"
49+
"print(" + repr(BOOT_JSON_MARKER) + " + json.dumps("
50+
"{'nodes':[n.to_row() for n in g.nodes],"
51+
"'edges':[e.to_row() for e in g.edges],'residuals':g.residuals}))\n"
52+
)
53+
try:
54+
proc = subprocess.run(
55+
[sys.executable, "-c", code, payload],
56+
capture_output=True,
57+
text=True,
58+
timeout=45,
59+
env=env,
60+
cwd=str(repo_root),
61+
)
62+
except subprocess.TimeoutExpired:
63+
graph = ExtractedGraph()
64+
graph.residuals.append("django.setup() skipped: boot subprocess timed out")
65+
return graph
66+
if proc.returncode != 0:
67+
graph = ExtractedGraph()
68+
err = (proc.stderr or proc.stdout or "unknown error").strip().splitlines()
69+
tail = err[-1] if err else "unknown error"
70+
graph.residuals.append(f"django.setup() skipped: {tail}")
71+
return graph
72+
data = _parse_boot_payload(proc.stdout)
73+
if data is None:
74+
graph = ExtractedGraph()
75+
graph.residuals.append("django.setup() skipped: boot subprocess returned invalid JSON")
76+
return graph
77+
return _graph_from_boot_data(data)
78+
79+
80+
def _graph_from_boot_data(data: dict) -> ExtractedGraph:
81+
graph = ExtractedGraph()
82+
graph.residuals.extend(data.get("residuals") or [])
83+
try:
84+
for row in data.get("nodes") or []:
85+
extra = row.get("extra") or {}
86+
if isinstance(extra, str):
87+
extra = json.loads(extra)
88+
graph.nodes.append(
89+
Node(
90+
id=row["id"],
91+
type=NodeType(row["type"]),
92+
name=row["name"],
93+
qualified_name=row["qualified_name"],
94+
file_path=row.get("file_path"),
95+
start_line=row.get("start_line"),
96+
end_line=row.get("end_line"),
97+
context=row.get("context"),
98+
extra=extra if isinstance(extra, dict) else {},
99+
)
100+
)
101+
for row in data.get("edges") or []:
102+
extra = row.get("extra") or {}
103+
if isinstance(extra, str):
104+
extra = json.loads(extra)
105+
graph.edges.append(
106+
Edge(
107+
src=row["src"],
108+
dst=row["dst"],
109+
type=EdgeType(row["type"]),
110+
confidence=float(row.get("confidence") or 1),
111+
extra=extra if isinstance(extra, dict) else {},
112+
)
113+
)
114+
except (KeyError, TypeError, ValueError, json.JSONDecodeError) as exc:
115+
graph = ExtractedGraph()
116+
graph.residuals.append(f"django.setup() skipped: boot payload malformed ({exc})")
117+
return graph
118+
119+
120+
def _parse_boot_payload(stdout: str | None) -> dict | None:
121+
text = stdout or ""
122+
idx = text.rfind(BOOT_JSON_MARKER)
123+
blob = text[idx + len(BOOT_JSON_MARKER) :] if idx >= 0 else text
124+
blob = blob.strip().splitlines()[0] if blob.strip() else ""
125+
if not blob:
126+
return None
127+
try:
128+
data = json.loads(blob)
129+
except json.JSONDecodeError:
130+
return None
131+
return data if isinstance(data, dict) else None
132+
133+
134+
def _boot_inprocess(repo_root: Path, config: LoadpathConfig) -> ExtractedGraph:
14135
graph = ExtractedGraph()
15136
settings_mod = _discover_settings_module(repo_root, config.django_root)
16137
if not settings_mod:

src/loadpath/extractors/react.py

Lines changed: 23 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -236,12 +236,22 @@ def edge(src: str, dst: str, etype: EdgeType, confidence: float = 1.0, extra: di
236236
continue
237237
line = source[: m.start()].count("\n") + 1
238238
norm = normalize_url_template(url)
239+
generated_file = "/generated/" in f"/{rel}/" or "openapi" in Path(rel).stem.lower()
240+
qname = f"client:{rel}:{norm}"
241+
if any(n.qualified_name == qname for n in graph.nodes):
242+
continue
239243
client = add(
240244
NodeType.API_CLIENT,
241245
norm,
242-
f"client:{norm}",
246+
qname,
243247
line,
244-
{"raw": url, "inferred": True, "feature": feature, "file": rel},
248+
{
249+
"raw": url,
250+
"inferred": not generated_file,
251+
"generated": generated_file,
252+
"feature": feature,
253+
"file": rel,
254+
},
245255
)
246256
for owner in hooks or components:
247257
edge(owner.id, client.id, EdgeType.CALLS)
@@ -253,14 +263,22 @@ def edge(src: str, dst: str, etype: EdgeType, confidence: float = 1.0, extra: di
253263
url = m.group(1)
254264
line = source[: m.start()].count("\n") + 1
255265
norm = normalize_url_template(url)
256-
if any(n.qualified_name == f"client:{norm}" for n in graph.nodes):
266+
generated_file = "/generated/" in f"/{rel}/" or "openapi" in Path(rel).stem.lower()
267+
qname = f"client:{rel}:{norm}"
268+
if any(n.qualified_name == qname for n in graph.nodes):
257269
continue
258270
add(
259271
NodeType.API_CLIENT,
260272
norm,
261-
f"client:{norm}",
273+
qname,
262274
line,
263-
{"raw": url, "inferred": True, "feature": feature, "file": rel},
275+
{
276+
"raw": url,
277+
"inferred": not generated_file,
278+
"generated": generated_file,
279+
"feature": feature,
280+
"file": rel,
281+
},
264282
)
265283

266284
for m in ROUTE_JSX_RE.finditer(source):

src/loadpath/index.py

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,8 @@
1414

1515
PY_SKIP = {"migrations"} # still extract migrations, just not skip
1616
INDEX_EXTENSIONS = {".py", ".ts", ".tsx", ".js", ".jsx"}
17+
# Bump when extractor/stitch node identity changes so incremental indexes rebuild.
18+
INDEX_REVISION = "3"
1719

1820

1921
def default_db_path(repo_root: Path) -> Path:
@@ -80,6 +82,7 @@ def _sidecar_digest(repo_root: Path, config: LoadpathConfig) -> str:
8082
digest.update(rel.encode())
8183
digest.update(path.read_bytes())
8284
digest.update(_config_digest(repo_root).encode())
85+
digest.update(INDEX_REVISION.encode())
8386
return digest.hexdigest()
8487

8588

src/loadpath/review/confidence.py

Lines changed: 8 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -40,14 +40,17 @@ def score_confidence(
4040

4141
tested_ids: set[str] = set()
4242
impact_ids = {n["id"] for n in impact_nodes}
43-
all_edges = list(store.edges())
44-
for e in list(impact_edges) + all_edges:
45-
if e["type"] == EdgeType.TESTED_BY.value:
43+
for e in impact_edges:
44+
if e["type"] != EdgeType.TESTED_BY.value:
45+
continue
46+
if e["src"] in impact_ids and e["dst"] in impact_ids:
4647
tested_ids.add(e["src"])
4748

48-
# A sink is covered if it, or a producer within two hops (view/serializer/hook/page), is tested.
49+
# A sink is covered if it, or a producer within two hops on THIS path, is tested.
4950
inbound: dict[str, list[str]] = {}
50-
for e in all_edges:
51+
for e in impact_edges:
52+
if e["src"] not in impact_ids or e["dst"] not in impact_ids:
53+
continue
5154
inbound.setdefault(e["dst"], []).append(e["src"])
5255
inbound.setdefault(e["src"], []).append(e["dst"])
5356

src/loadpath/review/engine.py

Lines changed: 30 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,7 @@
55
from pathlib import Path
66
from uuid import uuid4
77

8-
from loadpath.architecture.rules import evaluate
8+
from loadpath.architecture.rules import _related_accesses, evaluate
99
from loadpath.config import LoadpathConfig, load_config
1010
from loadpath.graph.store import GraphStore
1111
from loadpath.index import default_db_path, index_drift, index_repo
@@ -243,6 +243,9 @@ def collect_residuals(store: GraphStore, impact_nodes: list[dict], diff: DiffSet
243243
for line in stored.splitlines():
244244
if any(f and f in line for f in impact_files) or any(n and str(n) in line for n in impact_names):
245245
residuals.append(line)
246+
fields_by_name: dict[str, list[dict]] = {}
247+
for field in store.nodes([NodeType.FIELD]):
248+
fields_by_name.setdefault(field["name"], []).append(field)
246249
for n in impact_nodes:
247250
extra = n.get("extra") or {}
248251
if extra.get("get_serializer_class"):
@@ -254,12 +257,28 @@ def collect_residuals(store: GraphStore, impact_nodes: list[dict], diff: DiffSet
254257
if extra.get("queryset_in_serializer"):
255258
residuals.append(f"Queryset inside serializer {n['qualified_name']}")
256259
for hit in extra.get("nplusone") or []:
257-
accessed = ", ".join(hit.get("accessed") or []) or "related fields"
260+
accessed = list(hit.get("accessed") or [])
261+
related, _ = _related_accesses(accessed, fields_by_name, extra.get("app"))
262+
if not related:
263+
continue
258264
residuals.append(
259-
f"N+1 {accessed} in {n.get('file_path')}:{hit.get('line')}{hit.get('suggested_fix')}"
265+
f"N+1 {', '.join(related)} in {n.get('file_path')}:{hit.get('line')}{hit.get('suggested_fix')}"
260266
)
261267
residuals.extend(_test_field_residuals(impact_nodes, diff))
262268
residuals.extend(_react_path_residuals(impact_nodes, diff))
269+
ids = {n["id"] for n in impact_nodes}
270+
for e in store.edges():
271+
if e["src"] not in ids or e["dst"] not in ids:
272+
continue
273+
extra = e.get("extra") or {}
274+
if extra.get("overlap"):
275+
residuals.append(
276+
f"Inferred serializer/Zod overlap fields={extra['overlap']}"
277+
)
278+
if extra.get("superseded_by_generated"):
279+
residuals.append(
280+
f"String URL stitch {extra.get('react')} superseded by a generated OpenAPI client"
281+
)
263282
seen = set()
264283
out = []
265284
for r in residuals:
@@ -269,6 +288,11 @@ def collect_residuals(store: GraphStore, impact_nodes: list[dict], diff: DiffSet
269288
return out
270289

271290

291+
def _serious_evolution_notes(notes: list[str]) -> list[str]:
292+
tokens = ("hotspot", "silo", "crosses a bounded", "cross-context", "temporal coupling")
293+
return [n for n in notes if any(tok in n.lower() for tok in tokens)]
294+
295+
272296
def suggested_reviewers(config: LoadpathConfig, impact_nodes: list[dict]) -> list[str]:
273297
owners: list[str] = []
274298
for n in impact_nodes:
@@ -358,10 +382,11 @@ def run_review(
358382
residuals = collect_residuals(store, impact_nodes, diff)
359383
evolution = analyze_evolution(repo_root, diff, impact_nodes, config)
360384
confidence = score_confidence(store, impact_nodes, impact_edges, scoped, residuals)
361-
if evolution.get("notes") and confidence["level"] == "high":
385+
serious = _serious_evolution_notes(evolution.get("notes") or [])
386+
if serious and confidence["level"] == "high":
362387
confidence["level"] = "medium"
363388
reasons = list(confidence.get("reasons") or [])
364-
reasons = [evolution["notes"][0], *reasons][:3]
389+
reasons = [serious[0], *reasons][:3]
365390
confidence["reasons"] = reasons
366391
boot = store.get_meta("django_boot") or "off"
367392
if boot == "failed" and confidence["level"] == "high":

0 commit comments

Comments
 (0)