PHP interface refusal does not survive incremental rebuilds, minting a wrong edge (user stories 10 + 19)

[filipechagas]

Parent

Spec: #1 (upstream: Graphify-Labs#1682). Split out of #7 per review — this is a behavioural defect, not release documentation. Originally recorded in #5's closing comment.

Problem

Interface refusal (#5) does not survive an incremental rebuild. On a full build an interface-typed receiver is correctly refused; on an incremental rebuild where the interface's own file is unchanged, the refusal silently stops applying and the receiver binds to an unrelated same-short-named class.

This breaks two user stories from spec #1 simultaneously:

10. As a graph consumer, I want no calls edge when the receiver's type is an interface, so that the graph never guesses which implementation is bound.

19. As a user of graphify update (watch/incremental), I want the new resolution to run on incremental rebuilds too, so that the graph does not drift after edits.

US 19 is met for ordinary typed receivers — review confirmed end-to-end with a real graphify update that adding a method to a PHP controller produces a new INFERRED edge. The hole is specific to the interface-name channel, and its effect is worse than a missing edge: it mints a wrong one.

Context from #5

Recorded when #5 landed (4f17dd8):

Two items handed to #7: (1) measured hole — interface refusal does not survive incremental rebuilds (interface names have no channel into the persisted resolution context; repro at /tmp/probe_php_iface_incremental.py)

Mechanism

Interface names travel from extraction to the resolver only through per_file. The engine stamps them on the per-file result (graphify/extractors/engine.py:5180-5184):

if php_interface_names:
    # Interfaces mint no definition node, so the resolver cannot tell one
    # from a same-named class without this (#1682). Sorted for a stable
    # AST-cache payload.
    result["php_interfaces"] = sorted(php_interface_names)

and the resolver gathers them from per_file (graphify/extract.py:3141-3145):

interface_names = {
    key(name)
    for result in per_file
    for name in result.get("php_interfaces", [])
}

per_file aligns 1:1 with paths (per the comment at extract.py:2141-2143) — i.e. only the files actually dispatched for extraction this run.

The incremental widening path carries two other channels and no third: resolution_context_nodes are merged into resolution_nodes (extract.py:5634-5639) and resolution_context_edges into the resolver edge list (extract.py:5925). There is no channel for php_interfaces. So when the interface file is unchanged and therefore not dispatched, interface_names is empty, the refusal arm never fires, and the lone same-short-named class satisfies the single-definition guard.

extract()'s own docstring (extract.py:4906-4924) enumerates exactly what the context widens — the direct-call label/file indexes, the indirect_call callable guard, and the member-call resolvers — and PHP interface names are absent from that list.

Measured repro

/tmp/probe_php_iface_incremental.py (also reproduced below so it survives /tmp). Corpus is the Laravel Contracts collision: an App\Contracts\Notifier interface, an unrelated App\Support\Notifier class, and a Dispatcher with a private Notifier $notifier receiver. The second extract() dispatches only Dispatcher.php, supplying the unchanged corpus as resolution context, as watch/incremental does.

Measured output:

FULL build  -> notify calls edges: []
INCREMENTAL -> notify calls edges: [('..._app_http_dispatcher_dispatcher_go',
                                     '..._app_support_notifier_notifier_notify')]

So Dispatcher::go gains a calls edge into App\Support\Notifier::notify — a class it has nothing to do with — purely because the interface file was not re-extracted.

import pathlib
import tempfile

from graphify.extract import extract

d = pathlib.Path(tempfile.mkdtemp())
corpus = {
    "app/Contracts/Notifier.php": (
        "<?php\nnamespace App\\Contracts;\n"
        "interface Notifier {\n    public function notify(string $m): void;\n}\n"
    ),
    "app/Support/Notifier.php": (
        "<?php\nnamespace App\\Support;\n"
        "class Notifier {\n    public function notify(string $m): void {}\n}\n"
    ),
    "app/Http/Dispatcher.php": (
        "<?php\nnamespace App\\Http;\nuse App\\Contracts\\Notifier;\n"
        "class Dispatcher {\n    private Notifier $notifier;\n"
        "    public function go(): void { $this->notifier->notify('x'); }\n}\n"
    ),
}
paths = {}
for name, body in corpus.items():
    p = d / name
    p.parent.mkdir(parents=True, exist_ok=True)
    p.write_text(body)
    paths[name] = p


def notify_edges(result):
    return [
        (e["source"], e["target"]) for e in result["edges"]
        if e["relation"] == "calls" and "notify" in e["target"].lower()
    ]


full = extract(list(paths.values()), cache_root=d / "out1")
print("FULL build  -> notify calls edges:", notify_edges(full))

# Incremental: only Dispatcher.php is dispatched; everything else is context.
changed = paths["app/Http/Dispatcher.php"]
ctx_nodes = [n for n in full["nodes"] if "dispatcher" not in n["id"]]
ctx_edges = [
    e for e in full["edges"]
    if e["relation"] in ("contains", "method") and "dispatcher" not in e["source"]
]
inc = extract(
    [changed],
    cache_root=d / "out2",
    resolution_context_nodes=ctx_nodes,
    resolution_context_edges=ctx_edges,
)
print("INCREMENTAL -> notify calls edges:", notify_edges(inc))

Candidate fix directions

Both were recorded by the #5 implementer; neither has been chosen.

A. Persist PHP interface names alongside the resolution context. Give interface names their own channel through the incremental path, parallel to resolution_context_nodes/resolution_context_edges, so watch/incremental can hand back the unchanged corpus's interface names. Most direct, but widens the extract() signature — a third context parameter for one language.

B. Stamp a marker on a node the incremental path already carries. Prior art is immediately adjacent and deliberate: _callable / _callable_class (Graphify-Labs#2438) are stamped on nodes (engine.py:5177, ten lines above the php_interfaces block) and are deliberately not popped (extract.py:6059: "_callable / _callable_class are deliberately NOT popped (Graphify-Labs#2438): they …") precisely so they survive into the persisted resolution context. The resolver would then read interface names off resolution_nodes instead of per_file.

Wrinkle the fixer must resolve for direction B: a PHP interface mints no definition node, so there is no natural Notifier node to stamp. Verified — extracting the interface file alone yields only:

node: ext_notifier_php                     label='Notifier.php'
node: tmp_..._notifier_notify              label='notify()'

A file node and the interface's method node, but nothing representing the interface type. So B needs a host chosen deliberately — e.g. an attribute listing the file's interface names on the file node — rather than a per-type boolean like _callable_class. Relying on the file node's Notifier.php label would be wrong: it holds only under one-interface-per-PSR-4-file convention.

Acceptance criteria

• Incremental rebuild with the interface file unchanged and not dispatched: interface-typed receiver yields no calls edge (the probe's INCREMENTAL line matches its FULL line)
• Interface + same-short-named unrelated class, incremental: no edge into that class (PHP: interface-typed receivers refuse, even under short-name collision #5's guarantee, preserved across rebuilds)
• Interface refusal stays case-insensitive across rebuilds (PHP: interface-typed receivers refuse, even under short-name collision #5 AC)
• A regression test pins the incremental path, not just the full build — the existing PHP: interface-typed receivers refuse, even under short-name collision #5 tests all go through single full extract() calls and cannot catch this
• graphify update on a real corpus reproduces the refusal (US 19)
• Full suite green (baseline on feat/php-member-calls-1682: 3988 passed / 36 skipped)

Blocked by

None. Independent of #7 — but if this ships unfixed, #7's changelog should name it as a known limitation.

[filipechagas]

Fixed on feat/php-member-calls-1682-t6 @ 8d051c6 (not pushed, issue left open for the orchestrator)

fix(php): keep the interface refusal across incremental rebuilds (#11)

File	Change
graphify/extractors/engine.py	Also stamp _php_interfaces (the same sorted names already put on the per-file result) on the PHP file node
graphify/extract.py	New _php_context_interface_entry(); the incremental branch feeds its result to run_language_resolvers as one extra php_interfaces-only per_file entry on the scratch list
graphify/watch.py, graphify/cli.py	_php_interfaces added to the persisted-marker tuple that already carries _callable / _callable_class
tests/test_php_member_calls.py	+4 tests — incremental path through the public extract() seam
tests/test_watch.py	+3 tests — end-to-end _rebuild_code(changed_paths=...)

Direction chosen: B, and why

B (stamp a marker on a node the incremental path already carries), over A (a third extract() context parameter).

A does not avoid B's work. watch/update build their context from graph.json, which carries only nodes and links — so A still needs somewhere to persist the names across runs, i.e. the same node marker, plus a widened public extract() signature for one language. B is the strict subset: 4 files, two of them a one-name addition to an existing marker loop, no signature change, and it reuses a precedent this repo already documents as deliberate (extract.py: "_callable / _callable_class are deliberately NOT popped (Graphify-Labs#2438)").

Resolving the wrinkle from the issue: the host is the PHP file node, and the names are listed explicitly in the marker — never inferred from the Notifier.php label, which holds only under one-interface-per-file PSR-4 convention. No definition nodes changed; PHP still mints nothing for an interface.

On the read side the marker does not become a second channel in the resolver. extract() turns it back into the resolver's existing one: a synthetic php_interfaces-only per_file entry appended to the scratch list, so _resolve_php_member_calls still reads exactly one union and full/incremental agree by construction.

The measured trap: merge shadowing (worth reading before touching this path again)

My first attempt had the resolver read _php_interfaces off its all_nodes, i.e. off the merged resolution_nodes. The new tests stayed red. Cause, measured:

DBG ctx in: 5 markers: ['app_contracts_notifier'] fresh: [
  ('app_http_dispatcher',            'Dispatcher.php',           '/.../app/Http/Dispatcher.php'),
  ('app_http_dispatcher_dispatcher', 'Dispatcher',               '/.../app/Http/Dispatcher.php'),
  ('app_http_dispatcher_dispatcher_go', '.go()',                 '/.../app/Http/Dispatcher.php'),
  ('app_contracts_notifier',         'App\Contracts\Notifier',   '')]     <-- sourceless import stub
DBG interface_names set() nodes_with_marker []

extract.py merges context with fresh wins on id collision:

resolution_nodes = all_nodes + [n for n in resolution_context_nodes
                                if n.get("id") and n["id"] not in _fresh_ids]

The changed caller's use App\Contracts\Notifier; mints a sourceless import stub whose id IS the interface file node's id (app_contracts_notifier — that shared id is how the full build collapses stub onto file node). So the marker-bearing context node is dropped as a duplicate — precisely in the case the refusal is needed, since an interface-typed receiver essentially always uses the contract. Hosting a marker on a file node and reading it post-merge is therefore not viable; the fix harvests from the raw resolution_context_nodes list instead, which is what _php_context_interface_entry's docstring records.

Implication for _callable (Graphify-Labs#2438): probed, does not reproduce — no follow-up issue filed

Probe: /tmp/probe11_callable_shadow.py — for six corpora it does a full extract(), assembles watch-shaped context, re-extracts only the touched caller, then reports every context node dropped by id collision and which markers went with it.

=== php: use <Interface> (the #11 case)
    dropped ctx node id='app_contracts_notifier' label='Notifier.php' markers=['_php_interfaces']
      shadowing fresh node label='App\Contracts\Notifier' source_file='' markers=[]
=== php: use <Class> + new + member call
    dropped ctx node id='app_services_leadservice' label='LeadService.php' markers=[]
      shadowing fresh node label='App\Services\LeadService' source_file='' markers=[]
=== python: from b import handler; pool.submit(handler) (#2438)   -> no context node shadowed
=== python: from b import handler; handler() direct call          -> no context node shadowed
=== typescript: import { Service }; new Service().ping() (#2437)  -> no context node shadowed
=== java: cross-package call into an unchanged class              -> no context node shadowed

_callable/_callable_class shadowing reproduced: False

Two measurements explain why, and bound the claim:

1. Shadowing showed up only in PHP, and the shadowed node was always the file node — the use stub takes _file_node_id(target). Python/TS/Java import stubs in these corpora collide with nothing.
2. All 7 callable_def_nids.add(...) sites in engine.py stamp a class / function / method / arrow-class-field / CJS-fn nid. None stamps file_nid, so a file node can never carry _callable.

So the callability markers are not exposed to this shape of the bug: the only node the stub shadows is the one kind of node that never carries them. Not filing a follow-up — there is no reproduction to point at. What I would flag for a future reader is the general rule rather than a bug: any marker hosted on a file node is unsafe to read after the resolution_nodes merge in a language whose imports mint file-id stubs. If callability ever moves onto file nodes, or another language starts minting definition-id stubs, this becomes live.

Verification

Acceptance criteria

• Probe agrees, FULL vs INCREMENTAL — /tmp/probe_php_iface_incremental.py (copied to /tmp/probe11_iface_incremental.py): FULL -> [], INCREMENTAL -> []. Was INCREMENTAL -> [('..._dispatcher_go', '..._support_notifier_notifier_notify')].
• Interface + same-short-named class, incremental: no edge into that class — test_interface_short_name_collision_emits_no_edge_incrementally asserts the stranger's exact node id is absent.
• Case-insensitive across rebuilds — test_interface_refusal_is_case_insensitive_incrementally (private notifier $notifier).
• A test pins the incremental path — 4 in test_php_member_calls.py drive extract() with context assembled the way watch.py:1205-1240 builds it from graph.json (id/label/source_file/file_type/type + markers; contains/method edges), plus 3 in test_watch.py going end-to-end through _rebuild_code(changed_paths=[caller]).
• Real corpus — graphify extract . --code-only twice (/tmp/probe11_cli_extract.py): second run reports 2 files cached/unchanged, 1 re-extracted, and no notify edge. Pre-fix the same probe minted the wrong edge, so this is the entry point the cli.py line fixes. graphify update . twice (/tmp/probe11_cli_update.py): refusal holds, marker persisted as ('app/Contracts/Notifier.php', ['Notifier']).
• Suite green — see below.

Red before green (git stash push graphify/, run, git stash pop):

FAILED tests/test_php_member_calls.py::test_interface_refusal_survives_incremental_rebuild
FAILED tests/test_php_member_calls.py::test_interface_short_name_collision_emits_no_edge_incrementally
FAILED tests/test_php_member_calls.py::test_interface_refusal_is_case_insensitive_incrementally
3 failed, 50 passed

FAILED tests/test_watch.py::test_incremental_rebuild_keeps_php_interface_refusal
FAILED tests/test_watch.py::test_incremental_rebuild_persists_php_interface_marker
2 failed, 1 passed

The two that pass pre-fix are deliberate, not vacuous: test_class_typed_receiver_still_resolves_incrementally is the positive control (guards against "fix by refusing everything" — a class-typed receiver must still bind into its unchanged file, Graphify-Labs#2437), and test_incremental_rebuild_php_interface_legacy_graph_self_heals pins the degradation contract (a pre-marker graph.json must not crash the rebuild, and a full rebuild restores marker + refusal) — it deliberately does not assert the degraded intermediate state, since that state is the wrong edge.

The same stash also confirms the real-world break, and one nuance worth recording: graphify update . was not broken pre-fix (AGREE: True) because without a watcher-supplied changed set it re-extracts the whole corpus. Pre-fix graphify extract . --code-only gave AGREE: False with the wrong edge. So the genuinely affected entry points are graphify extract incremental and the live graphify watch loop (_rebuild_code(changed_paths=...)) — both now covered, by probe and by test respectively.

Suite — uv run --frozen --extra ollama pytest: 3994 passed / 1 failed / 36 skipped. The failure is tests/test_extract.py::test_collect_files_skips_hidden, an environment artifact of this worktree's dotted path (~/.herdr/worktrees/...) that also fails at the branch head; effective baseline here is 3987 passed / 1 failed / 36 skipped, and 3987 + 7 new tests = 3994, so nothing else moved. ruff check clean on all six files.

No deviations from the issue's constraints: no change to what definition nodes PHP mints, existing suites pass unchanged, both build paths fixed. One commit, nothing pushed, issue left open.

[filipechagas]

Fixed in 8d051c6 (channel + watch/cli persistence, direction B per the implementer's comment above) and extended in 85bb8df after merging with #12 (marker renamed _php_non_class_types, dual-read compatibility, enums/traits carried across rebuilds too). Orchestrator gate: full suite 4024/36; FULL and INCREMENTAL now agree on the repro; watch and CLI paths both pinned by tests.