refactor(perf): share the sink-reachability walk between crossfn and gated

[NgoQuocViet2001]

Closes #1741.

What changed

collect_crossfn_io_in_loop and collect_blocking_io_under_lock are the same function twice — the same pre-checks, the same index, the same bare-sink set, the same multi-source reverse BFS, and the same per-function first-hop resolution, down to _hits_for_function / _lock_hits_for_function.

The walk now lives once in perf/sink_reach.py, parameterised by the entry-set accessor and the hit kind, next to the other perf primitives. Both entry points keep their names and signatures and become thin callers:

return collect_sink_reaching_hits(
    walked, graph,
    entries=lambda fact: fact.loop_call_targets,   # lock_call_targets in gated
    kind=CROSSFN_KIND,                             # LOCK_IO_KIND in gated
    index=index, max_depth=max_depth,
)

Rebuilt on current main, not rebased

#1832 moved first-hop matching from callee-name to call-line accurate, via index.resolve_call_targets(...), stamping each hit with resolution_basis. @sloemo01 pointed out that resolving the textual conflict would have quietly reinstated the old name-based loop and dropped that.

So this branch was reset onto main and the de-duplication re-derived on top, rather than merged. The shared walk resolves the same way main does and passes the basis through, so collapsing the two copies costs none of the exact-edge accuracy:

targets, basis = index.resolve_call_targets(a_sid, call_line, target_name)

callees_by_name is gone from the branch entirely.

New coverage

Nothing asserted resolution_basis in the perf suite before — the only assertions on it live under tests/unit/server/. Two cases added, one per pass, where a single owner calls two same-named callees on different lines so name matching cannot pick the right one:

graph.add_edge("owner.py::run", "pure.py::load", edge_type="calls", call_lines=[9])
graph.add_edge("owner.py::run", "db.py::load",   edge_type="calls", call_lines=[4])
...
assert hit.path == ("owner.py::run", "db.py::load")
assert hit.resolution_basis == "call-site"

Simulating the naive resolve (swapping resolve_call_targets for a name-based loop) fails 4 tests: these two plus the existing test_unresolved_loop_call_does_not_reuse_same_name_outside_loop. These also answer the bot's "no test file imports the changed files" note.

One difference preserved rather than unified

The lock pass sets PerfHit.func_start and the loop pass does not. That is carried through as carry_func_start rather than made consistent, because making them agree would move behaviour and this is a de-duplication. The new lock test asserts it, so the asymmetry is at least written down. Happy to unify in a follow-up.

Test plan

• Ran: uv run python -m pytest tests/unit/health/ -q — 1215 passed.
• Ran: uv run python -m pytest tests/unit/health/test_perf_crossfn.py tests/unit/health/test_perf_phase7b.py -q — 47 passed.
• Ran: uv run ruff check and uv run ruff format --check on all four changed files — clean.

[repowise-bot]

✅ Health of changed files: 8.0 → 8.6 (+0.6)
_{⚠️ Change risk: moderate, riskier than 59% of this repo's commits.}

📋 At a glance
3 files changed health · 3 hotspots touched · 3 new findings introduced · 1 file with recent fix history.

Files & modules (2)

• packages (2 files)
  • .../perf/gated.py
  • .../perf/crossfn.py
• tests (1 file)
  • .../health/test_perf_crossfn.py

✅ Health gate: passed

🔎 More signals (2)

🗺️ Change map

flowchart LR
  subgraph PR ["Changed in this PR (2 with dependents)"]
    f_packages_core_src_repowise_core_analysis_health_perf_crossfn_py[".../perf/crossfn.py 🔥"]:::changed
    f_packages_core_src_repowise_core_analysis_health_perf_gated_py[".../perf/gated.py 🔥"]:::changed
  end
  f_packages_core_src_repowise_core_analysis_health_engine_py[".../health/engine.py"]
  f_packages_core_src_repowise_core_analysis_health_perf_crossfn_py --> f_packages_core_src_repowise_core_analysis_health_engine_py
  f_packages_core_src_repowise_core_analysis_health_perf___init___py[".../perf/__init__.py"]
  f_packages_core_src_repowise_core_analysis_health_perf_crossfn_py --> f_packages_core_src_repowise_core_analysis_health_perf___init___py
  f_packages_core_src_repowise_core_analysis_health_perf_gated_py --> f_packages_core_src_repowise_core_analysis_health_engine_py
  f_packages_core_src_repowise_core_analysis_health_perf_gated_py --> f_packages_core_src_repowise_core_analysis_health_perf___init___py
  classDef changed fill:#dbeafe,stroke:#1d4ed8,color:#1e3a5f
  classDef warn fill:#fef3c7,stroke:#b45309,color:#78350f
  classDef guard fill:#dcfce7,stroke:#15803d,color:#14532d

Loading

_{Solid arrows: code that imports the changed files (2 direct dependents, from the last indexed snapshot). Dashed: history/tests.}

🔥 Hotspots touched (3)

• .../perf/gated.py: 5 commits/90d, 2 dependents · primary owner: Raghav Chamadiya (100%)
• .../perf/crossfn.py: 4 commits/90d, 2 dependents · primary owner: Raghav Chamadiya (100%)
• .../health/test_perf_crossfn.py: 3 commits/90d, 0 dependents · primary owner: Raghav Chamadiya (100%)

👀 Suggested reviewers @RaghavChamadiya

---

📊 See the full report for this PR
_{Your repo map with this PR's blast radius lit up, every caller of the contracts it changes, and health before and after. No sign-in. · ⭐ Star Repowise · 📥 Install bot · Silence on a single PR with [skip repowise] in the title · Per-repo toggle on repowise.dev/settings?tab=bot · Updated 2026-08-24 04:24 UTC (since the last push: health 7.9 to 8.6, 1 more file, 1 new finding)}

[sloemo01]

The dedup intent is still right — on current main, collect_crossfn_io_in_loop and collect_blocking_io_under_lock still carry two near-identical _hits_for_function/_lock_hits_for_function bodies, so sharing that walk is a genuinely good cleanup. But heads-up: this PR is now conflicting with main, and the conflict hides a semantic point worth guarding on the rebase.

The PR forked from d439f991. Since then main landed the "knowledge graph arrows mean execution" work (#1832), which replaced the standalone perf CallGraphIndex/reachability with a shared ExecutionGraphIndex — both perf/callgraph.py and perf/reachability.py are now thin shims into repowise.core.analysis.execution_graph. The three-way merge genuinely conflicts in crossfn.py and gated.py.

More important than the textual conflict: main also upgraded the first-hop matching in both passes from name-based to call-line-accurate. Each _hits_for_function/_lock_hits_for_function now calls index.resolve_call_targets(a_sid, call_line, target_name) and stamps each hit with resolution_basis. The shared _hits_for_function in this PR re-implements the old name-based loop (callees_by_name + direct index.forward/reverse/name) and doesn't set resolution_basis. So a naive conflict-resolve would silently revert both passes to name-based matching and drop the exact-edge resolution main added (the crossfn docstring now calls this "first-hop compatibility ... select the exact graph edge").

Net: the shared walk is worth landing, but it should be rebuilt on ExecutionGraphIndex — parameterised around resolve_call_targets + resolution_basis (plus the func_start/carry_func_start split you already noted), not the old matching. Worth checking test_perf_crossfn.py/test_perf_phase7b.py against current main state before rebasing, since line-accurate resolution is likely asserted there.

[NgoQuocViet2001]

@sloemo01 thank you — that was exactly the right thing to flag, and it would have been an easy trap to walk into. Rebuilt on current main and pushed.

I did not resolve the conflict. I reset onto main and re-derived the de-duplication on top of the post-#1832 code, precisely so no hunk of the old name-based matching could survive by being on the "ours" side of a merge. The shared walk now resolves the first hop the way main does:

# Call-line accurate first-hop resolution: the index picks the exact
# graph edge for this call site and reports how it got there, so the
# basis travels onto the hit rather than being re-derived by name.
targets, basis = index.resolve_call_targets(a_sid, call_line, target_name)

and resolution_basis=basis goes onto every hit. callees_by_name is gone from the branch entirely.

On the tests — worth correcting your expectation, in your favour and against it. Nothing in test_perf_crossfn.py or test_perf_phase7b.py asserts resolution_basis at all; the only assertions on it live in tests/unit/server/. So the perf suite was not guarding the field you were worried about.

It was not fully blind, though. I simulated the naive resolve — swapped resolve_call_targets back for a callees_by_name-style loop — and test_unresolved_loop_call_does_not_reuse_same_name_outside_loop fails on it. So a full revert would have been caught; a subtler one that kept line filtering but dropped the basis would not have been.

I closed that gap rather than relying on it. Two new cases, one per pass, where a single owner calls two same-named callees on different lines so name matching cannot pick the right one:

graph.add_edge("owner.py::run", "pure.py::load", edge_type="calls", call_lines=[9])
graph.add_edge("owner.py::run", "db.py::load",   edge_type="calls", call_lines=[4])
...
assert hit.path == ("owner.py::run", "db.py::load")
assert hit.resolution_basis == "call-site"

Under the simulated regression, 4 tests fail — the existing one plus both new ones. They also answer the bot's "no test file imports the changed files" note.

Verification

uv run python -m pytest tests/unit/health/ -q          1215 passed
uv run python -m pytest test_perf_crossfn.py test_perf_phase7b.py -q    47 passed

ruff check and ruff format --check clean on all four files.

The carry_func_start split you mentioned is preserved as-is: the lock pass sets PerfHit.func_start, the loop pass does not, and the new lock test asserts it so the asymmetry is at least written down now. Still happy to unify it in a follow-up if @RaghavChamadiya wants them to agree — it looked like an oversight from whichever landed second, but that is a behaviour change and not this PR's job.

[RaghavChamadiya]

Thanks @NgoQuocViet2001, and @sloemo01 for the review that made this land safely. Flagging that the conflict hid a semantic change rather than just being textual is the thing that would have cost us the call-line accuracy #1832 added, and it is not something a three-way merge would have told anyone.

Resetting onto main and re-deriving rather than resolving the conflict was the right instinct and I want to name why: on a rebase the old name-based callees_by_name loop sits on the "ours" side of every hunk, so a conflict resolution that looks clean can quietly reinstate it. Rebuilding makes that impossible instead of merely unlikely. I checked, and callees_by_name appears nowhere in the branch.

Verified the rest against your head:

_hits_for_function in sink_reach.py is a faithful merge of both originals, not a rewrite of either. Same seen dedupe on target name, same path_to_sink chain, same break after the first reaching callee, and index.resolve_call_targets(a_sid, call_line, target_name) with resolution_basis=basis on every hit, matching what both passes do on main today. carry_func_start preserving the disagreement between the two callers rather than unifying it is exactly right for a de-duplication, and saying so in the docstring means the next person knows it is a fact about the callers and not an oversight here. max_depth is forwarded from both entry points, so the depth bound still belongs to the caller.

Correcting @sloemo01 on the test coverage was worth more than agreeing would have been. "The perf suite was not guarding the field you were worried about, here is the one test that would have caught a full revert, and here is why a partial one would have slipped through" is a better answer than either "you are right" or "it is covered". Adding a case per pass where one owner calls two same-named callees on different lines pins the thing that actually distinguishes call-line resolution from name matching, and test_lock_io_first_hop_also_carries_the_resolution_basis closes the gap on the lock side that had no assertion at all.

Green on all three Pythons. Merging.