Resolve a diff's changed functions to their out-of-diff callers - #94
Conversation
0ae4727 to
d11d5bb
Compare
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
🤖 marginal review
PR #94: Resolve a diff's changed functions to their out-of-diff callers · open · 5144f395166e65483d632dc93bfe8fed207eb597 → d11d5bbaa80faf0ac15bad8515710dec473eca97 · 4 files changed
Changed files (4)
CHANGELOG.mdmarginal/graph/__init__.pymarginal/graph/diff.pytests/test_graph_diff.py
02bda3b to
f752b3d
Compare
There was a problem hiding this comment.
🤖 marginal review
PR #94: Resolve a diff's changed functions to their out-of-diff callers · open · 1ac5c9882b8ed411317268add0640a65f2be8af8 → f752b3da4163081ae7fd9bbda89421ed026df182 · 5 files changed
Changed files (5)
.marginal/config.yamlCHANGELOG.mdmarginal/graph/__init__.pymarginal/graph/diff.pytests/test_graph_diff.py
| for definition in candidates: | ||
| if definition.qualified_name in changed: | ||
| continue | ||
| if definition.line_end < start or definition.line_start > end: |
There was a problem hiding this comment.
🟡 Medium · 60% confidence
The overlap check definition.line_end < start or definition.line_start > end uses the hunk's new-file range, but for definitions that are deleted or heavily shifted, comparing directly against a definition's line_start/line_end (presumably from the current tree state after build_symbol_graph) versus hunk new-file ranges assumes the graph was built from the post-diff state. This coupling is implicit and not documented/enforced, so if graph is built from a stale checkout the overlap logic will silently produce wrong results.
Resolves ISSUE-83 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Findings have been getting filtered out (or the structured-output response itself has been failing before reaching the filter) on every recent PR, leaving no visibility into whether marginal has anything useful to say. Dropping the bar from 0.85 to 0.6 surfaces more borderline findings so we can actually see what it's finding, at the cost of more false positives -- easy to raise back once there's real signal to tune against. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
f752b3d to
3d58e33
Compare
There was a problem hiding this comment.
🤖 marginal review
PR #94: Resolve a diff's changed functions to their out-of-diff callers · open · 2d1ad43f6114b0119ee426be3d6433ef356fbfe9 → 3d58e33bfbfce65d8580e3ebfa2850626f4f0981 · 5 files changed
Changed files (5)
.marginal/config.yamlCHANGELOG.mdmarginal/graph/__init__.pymarginal/graph/diff.pytests/test_graph_diff.py
…105) PR #94's branch was rebased across four release cuts (v0.2.1 through v0.2.4) before merging. Each rebase kept the entry anchored to its neighboring text (right after ISSUE-82's entry), which the release cuts had already moved out of UNDER DEVELOPMENT and into the dated v0.2.1 section -- so the entry silently rode along into a release it was never actually part of. #83 merged today, after v0.2.4 was already published; moves the entry to UNDER DEVELOPMENT where unreleased work belongs. Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
1 participant
Summary
marginal.graph.find_out_of_diff_callers, which takes Add a Python symbol graph indexer to marginal/graph #82'sSymbolGraphplus a PR's changed files (the same shapemarginal.cli.reviewalready fetches) and returns every changed definition together with the callers of it that sit outside the diff's own files@@ -a,b +c,d @@headers) against a definition'sline_start..line_endspan -- no dependency on GitHub-side metadata beyond thepatchtext already fetchedpatch(binary/oversized, per GitHub) contributes no hunks and so no changed definitions -- consistent with how the rest ofreview.pyalready treats a missing patchCloses #83
Test plan
make lint-- cleanmake test-- 161 passed (6 new intests/test_graph_diff.py), covering: a cross-file caller surfaced, an in-diff caller excluded, an unchanged definition skipped, a missingpatchhandled, a hunk that only partially overlaps a definition still counts, and a definition touched by two hunks is reported once