Skip to content

fix: registry reverse-index lag on provider-body edits + nondeterministic equal-priority merge - #264

Merged
mikebronner merged 6 commits into
mainfrom
fix/255-fix-registry-reverse-index-lag-on-provider-body-ed
Jul 15, 2026
Merged

mikebronner merged 6 commits into
mainfrom
fix/255-fix-registry-reverse-index-lag-on-provider-body-ed

Conversation

@mikebronner

@mikebronner mikebronner commented Jul 14, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Implements #255 — fixes two inherited defects in the shared provider-registry model (bindings / facade aliases / macros):

  • Bug A — the reverse find-references index lagged an in-editor provider-body edit: a boot()/register() registration change produces an empty class-surface diff, so refresh_magic_dependents was never spawned and call sites in other files kept stale classifications until an unrelated reconverge.
  • Bug B — an equal-priority same-key collision (two providers registering the same macro/binding key) resolved nondeterministically because the merge iterated a HashMap, flipping the goto target across LSP restarts.

Changes

  • Bug B: sorted_sp_files() merges providers in lexicographic path order in build_macro_registry and handle_get_all_parsed_bindings — on an equal-priority collision the provider with the smallest path deterministically wins (documented in both builders).
  • Bug A: ProviderRegistrationsData + pure registration_ripple_keys(before, after, provider_path) + the FileProviderRegistrations actor request. Uniform across macro, binding, and facade-alias registries (bootstrap/app.php withAliases + config/app.php aliases included).
  • Bug A baseline (review round): the pre-save "before" snapshot comes from an actor-kept registration_baselines map advanced only by save transactions (fresh_text), so it is insulated from the did_change debounce's eager re-registration — the ripple fires on the realistic edit → pause → save flow, not just inside the debounce window.
  • Bug A config path (review round): the save transaction routes config/app.php through handle_update_config_file, so the legacy aliases-array diff reads fresh text and ripples like the other two sources.
  • Bug A wiring: refresh_magic_on_save snapshots the file's registration contribution pre/post save and folds the ripple keys (macro host FQCNs, binding/alias concretes from both diff sides, the provider's own path) into the changed_classes blast radius and its early-return check. Keys pass through expand_class_descendants (seeds included) into dependents_of unchanged.
  • Bug A deps: member_resolver records a Macro classification's declaration file (= the registering provider for inline ::macro()) as a reverse-index dependency at all four classify exits, in both the live re-parse path and the captured-recipe path; binding call sites record a binding:<key> attempt dependency even when unresolved, so brand-new bindings ripple on the provider save too.
  • Ripple observability: new magic_ripple_handle (mirrors magic_rebuild_handle) makes the spawned ripple awaitable — used by the e2e regression test.
  • Determinism sweep (review follow-up, this PR): zero salsa_sp_files.values() merges remain — 12 sites now iterate sorted_sp_files(), including the view/blade/component namespace resolvers (single and _all_ variants — their per-key priority tiebreaks resolve winners, so sorting fixes real nondeterminism; output order of collected maps stays arbitrary), handle_get_parsed_binding, handle_get_parsed_middleware / handle_get_all_parsed_middleware, and the handle_get_laravel_config build loop. build_facade_alias_snapshot audited: it merges two fixed-path sources with no map iteration — already deterministic.
  • Non-provider saves stay cheap: an untracked path yields the empty default contribution, and an empty diff adds no keys, preserving the body-only early return.

Acceptance Criteria

  • [Bug A — provider-body refresh] Modifying a macro/binding registration in a provider boot()/register() body (without changing the class surface) causes the ripple to re-resolve call sites in other open files — no LSP restart required, including when the did_change debounce fired before the save (debounce-insulated baseline).
  • [Bug A] The reverse-index records the registering provider's path as a dependency for each macro key it registers (macro decl-file deps at every classify exit); binding sites record a binding:<key> attempt dependency (resolved or not), so both retargets and brand-new keys ripple on a provider save even when surface_map_diff is empty.
  • [Bug A] The provider-body invalidation path is applied uniformly to all three registries: macro, binding, and facade-alias — including the legacy config/app.php alias source, routed through the save transaction.
  • [Bug B — deterministic merge] Equal-priority same-key registrations resolve deterministically: lexicographically smallest provider path wins, stable across restarts, documented in the builders.
  • [Bug B] The tiebreak is applied in both build_macro_registry and handle_get_all_parsed_bindings (and swept across every remaining provider-map merge).
  • [Test — Bug A] End-to-end: provider_body_macro_rename_converges_dependent_on_save drives the real backend through the exact poisoned flow — execute_salsa_update (debounce) runs with the edited text before refresh_magic_on_save — and asserts the dependent file's stale classification clears. Plus config_app_alias_edit_ripples_through_save_transaction (alias source), unit coverage of registration_ripple_keys (empty diff → no keys; binding retarget → both concretes; alias retarget → both targets), and deps-recording tests for both macro decl files and binding attempt keys on both resolution paths.
  • [Test — Bug B] equal_priority_collision_resolves_to_smallest_provider_path: two equal-priority providers register the same macro + binding keys (registration order reversed on purpose — the construction that makes the assertion meaningful); the winner is the lexicographically smallest path in both registries.
  • [Scope] Live hover / goto-definition / rename are untouched — they re-derive from Salsa per call; every query-time resolve_and_classify call site passes None for the deps sink, so nothing is recorded on those paths.

Test Plan

  • Full suite green: 2165 + 467 tests pass (cargo test)
  • CI green on head 8e6b69c (LSP test/fmt/clippy, extension wasm check, CodeQL)
  • New regression tests cover both bugs end-to-end (see Test — Bug A / Bug B above)
  • cargo clippy --all-targets clean, cargo fmt applied

Fixes #255

…ation snapshot (#255)

Bug B: merge providers in lexicographic path order in build_macro_registry
and handle_get_all_parsed_bindings — equal-priority collisions now resolve
to the smallest provider path, stable across restarts.

Bug A (partial): add ProviderRegistrationsData + registration_ripple_keys
+ FileProviderRegistrations actor request (with fresh_text re-registration)
— the save-path wiring in refresh_magic_on_save and tests still pending.
@dr-john-h-watson

Copy link
Copy Markdown

⏸️ Budget cap hit mid-implementation — branch compiles clean, work committed (b80599f). Item stays In Progress; next tick resumes on this branch.

Done:

  • Bug B complete: sorted_sp_files() + deterministic lexicographic merge in build_macro_registry and handle_get_all_parsed_bindings (smallest provider path wins on equal priority, documented).
  • Bug A groundwork: ProviderRegistrationsData, pure registration_ripple_keys(before, after, provider_path) (emits macro host FQCNs, binding/alias concretes from both diff sides, provider path), and the FileProviderRegistrations actor request incl. fresh_text re-registration (App rescan is async, so the post-save snapshot must refresh the provider input itself). Uniform across macro/binding/facade-alias registries (bootstrap/app.php + config/app.php alias sources handled).

Remaining:

  1. Wire refresh_magic_on_save (main.rs ~6404): pre-snapshot file_provider_registrations(path, None) before update_file; post-snapshot with Some(text) after; extend changed_classes with registration_ripple_keys(...) and include it in the early-return emptiness check. Keys pass through expand_class_descendants (seeds are included) into dependents_of.
  2. member_resolver.rs: record macro_target(fqcn, member).0 (decl file = registering provider for inline macros) into deps when classification kind == Macro, at both chokepoints (resolve_and_classify ~line 559/598 and resolve_recipe_and_classify) — satisfies the AC's provider-path-dependency bullet for macros; bindings ride the concrete-FQCN emission (rationale in registration_ripple_keys rustdoc).
  3. Tests: (a) two equal-priority providers, same macro key + same binding key → winner is lexicographically-smallest path via snapshot_macros/get_all_parsed_bindings (SalsaActor::spawn + register_service_provider_source harness, see salsa_impl/tests.rs ~840); (b) registration snapshot + macro-rename diff → ripple keys contain host FQCN + provider path; (c) pure registration_ripple_keys cases (empty diff → empty; binding retarget → both concretes).
  4. cargo clippy + cargo test, PR template check, mark ready, CI green.

#255)

Bug A completion: refresh_magic_on_save snapshots the saved file's
registration contribution (macros / bindings / facade aliases) before and
after the Salsa update — the post snapshot re-registers the fresh text since
the App rescan is async — and folds registration_ripple_keys into the
changed-classes blast radius, so a boot()/register() body edit with an empty
surface diff still re-resolves dependent call sites.

member_resolver records a Macro classification's declaration file (the
registering provider, for inline ::macro()) as a reverse-index dependency at
all four classify exits, in both the live and captured-recipe paths, so the
provider-path ripple key lands on exactly the dependent sites.

Regression tests: equal-priority macro+binding collision resolves to the
lexicographically smallest provider path (Bug B); body-only macro rename
diffs into ripple keys carrying host FQCN + provider path; pure
registration_ripple_keys edge cases. Plus a clippy map_clone fix in the
inherited helper.
@mikebronner
mikebronner marked this pull request as ready for review July 14, 2026 21:56

@mr-sherlock-holmes mr-sherlock-holmes Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔄 Changes Requested

Bug B is solid. Bug A, unfortunately, doesn't fire in the workflow it targets — three blockers below, verified against the tree.

Issues Found

🔴 1. Bug A ripple is inert in the realistic edit → pause → save flow (main.rs refresh_magic_on_save, ~6407–6491)

The pre-save snapshot old_registrations = file_provider_registrations(path, None) reads salsa_sp_files. But that input is already overwritten by the time a save runs: on every keystroke did_change → queue_salsa_update → (250ms debounce) → execute_salsa_update, and for app/Providers/*.php + bootstrap/app.php that branch calls register_service_provider_source → handle_register_service_provider_source, which eagerly does file.set_text(...) on salsa_sp_files (salsa_impl.rs:~8600, :9709). So in the normal case (any ≥250ms pause between the last edit and Cmd+S — reading, reaching for the shortcut, or an earlier pause mid-session):

old_registrations = live salsa_sp_files  → already EDITED text
new_registrations = re-register same buffer (Some(text)) → EDITED text
registration_ripple_keys(EDITED, EDITED) → []   → changed_classes empty → early return

Dependents never re-resolve — exactly the bug #255 set out to fix. The surface path is insulated (that debounce branch never touches class_hierarchy_index, so old_surfaces stays pre-edit until refresh_file_magic runs inside the save handler); the registration path has no analogous insulation. The Some(text) comment's premise — "without it this would still read the pre-save text" — only holds if the debounce hasn't fired, which is the minority case.
Fix: capture the pre-save registration contribution insulated from the debounce (mirror how the surface snapshot is insulated), or derive "before" from state taken before the debounce could have written.

🔴 2. No end-to-end regression test for Bug A (AC "Test — Bug A" not met as worded)

provider_registration_snapshot_diffs_body_only_macro_rename (salsa_impl/tests.rs:~3824–3888) asserts only the pure registration_ripple_keys() output on hand-built snapshots — it never seeds a dependent Str::…() call site, never drives refresh_magic_on_save/did_save, and never asserts a dependent re-resolves. That is precisely why blocker #1 slipped through. The AC asks for "the reverse-index fires and dependent call sites are scheduled for re-resolution." The repo already has the pattern to copy: tests/watched_files_magic.rs::changed_dependency_converges_dependent (seed dependency + dependent → mutate → drive the real handler → assert the consumer's members change).
Note: the ripple currently runs in a bare tokio::spawn with no stored JoinHandle, so it can't be awaited deterministically — a test needs a handle (like magic_rebuild_handle/drain_batch) to hook into.

🔴 3. Facade-alias invalidation is not uniform — config/app.php never ripples (AC #3)

handle_file_provider_registrations (salsa_impl.rs:~9321–9358) sources aliases from two maps: bootstrap/app.php withAliases (a salsa_sp_files entry — works) and config/app.php aliases (via self.config_files.get(path) — a separate map). The post-save fresh_text re-registration (FileProviderRegistrations handler, salsa_impl.rs:~7088–7106) only re-registers salsa_sp_files paths, and did_save for a config file only calls invalidate_config_cache() — never update_config_file. So for a config/app.php alias edit, pre- and post-save alias snapshots read the same config_files entry → empty diff → no ripple. AC #3 ("uniformly to all three registries") is only partially met.

🟠 4. Bindings don't record the provider path — brand-new bindings don't ripple (AC #2)

keys.insert(provider_path…) in registration_ripple_keys (salsa_impl.rs:~4349) is a no-op for binding-only diffs: only macros record a decl-file dependency (which happens to equal the provider path for inline ::macro()). No binding call site ever records the provider path in MagicDependencyIndex (no binding analog of record_macro_decl_dep). Consequence: a brand-new binding key — whose call sites (app('key')) resolved to nothing before, so recorded no dependency — won't ripple on the provider save; it converges only on the next full pass. The retarget case (both concretes resolvable) does work, and the code comment discloses this — but AC #2 explicitly names binding keys. Either record a binding decl-file dependency so new bindings ripple, or bring a rationale for narrowing AC #2. (Don't silently ship it as "documented.")

What's Good

  • Bug B is correct and genuinely tested. sorted_sp_files() applied to both AC-named builders (build_macro_registry, handle_get_all_parsed_bindings); the tiebreak (existing.priority >= data.priority skip) is an order-independent max over priority, so different-priority collisions stay priority-ordered and only equal-priority ties become path-deterministic. equal_priority_collision_resolves_to_smallest_provider_path proves it — nice touch reversing the registration order so the assertion can't pass by accident.
  • The pure registration_ripple_keys helper is correct and well unit-tested — empty diff → no keys; binding retarget → both concretes.
  • Scope preserved (AC #8). Every query-time resolve_and_classify call site passes None, so record_macro_decl_dep never records on hover/goto/rename/find-references — no regression there.
  • Clean structure and honest doc comments; the four classify exits are kept branch-for-branch identical between the live and captured-recipe paths.

📋 Non-blocking follow-ups

  • Same equal-priority nondeterminism survives in the single-key binding lookup. handle_get_parsed_binding (salsa_impl.rs:9816) — the sibling of the fixed handle_get_all_parsed_bindings — still merges over unsorted salsa_sp_files.values() with the identical existing.priority >= data.priority tiebreak, so goto on a single binding stays nondeterministic on an equal-priority collision even after this fix. Same invariant, same one-line remedy (sorted_sp_files()). While you're there, sweep the rest of the class: handle_get_all_parsed_middleware (:9790) has the same tiebreak; audit build_facade_alias_snapshot (:9232), handle_get_all_view_namespaces (:9441), handle_get_all_blade_component_regs (:9505), handle_get_all_component_namespaces (:9569) and route any with a priority tiebreak through sorted_sp_files() too. Close the class in one pass rather than one lookup at a time.

(Watson: you're already in the code fixing the blockers above — implement the follow-up in this same PR too, no separate issue. Sweep the whole salsa_sp_files.values() class while you're reworking the merge determinism.)

Please address the above and re-request review.

@mr-sherlock-holmes mr-sherlock-holmes Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔄 Changes Requested

Bug B is solved cleanly. Bug A is not — the reverse-index ripple almost never fires on an ordinary save, the legacy config/app.php alias path never ripples at all, and no test exercises the real save path, so two implementation gaps sailed straight through green CI. Details below, most severe first, each with a fix.

Issues Found

1. 🔴 [Bug A core] The ripple rarely fires on a real save — the pre-save baseline is already poisoned.
refresh_magic_on_save reads old_registrations from salsa_sp_files[path] (main.rs:6431). But the did_change 200 ms debounce (execute_salsa_update → register_service_provider_source, main.rs:8598-8614) has already re-registered that same map entry with the post-edit buffer before the user ever saves. did_save doesn't cancel the pending salsa update — it cancels only pending_diagnostics (main.rs:22114) — and no pre-edit baseline is retained anywhere. So for the dominant "type → pause >200 ms → save" flow, old_registrations == new_registrations, registration_ripple_keys returns empty (main.rs:6480), and the reverse-index never re-resolves — exactly the case #255 exists to fix. The Some(text) fresh-registration at main.rs:6462 only refreshes the new snapshot; it does nothing for old. The fix therefore works only when the user saves inside the debounce window (<200 ms of the last keystroke) — a race, not a guarantee. Your own comment at main.rs:6455-6458 guards against the async App rescan but misses that the debounce beats it to the punch. Fix: capture the pre-edit registration baseline before the debounce advances salsa_sp_files — e.g. snapshot the provider registrations in did_change prior to the update, or keep a save-path-only "last committed registrations" map. Breaks AC [Bug A — provider-body refresh].

2. 🔴 [Bug A] config/app.php alias edits never ripple.
For the legacy config/app.php aliases array, handle_file_provider_registrations reads self.config_files (salsa_impl.rs:9350-9357) — a map refresh_magic_on_save never updates (it calls handle_update_file → self.files only; the fresh_text re-registration path covers only salsa_sp_files members, and config/app.php is never in salsa_sp_files). So old.aliases == new.aliases unconditionally for a config/app.php save, and its alias edits never ripple. bootstrap/app.php withAliases works; the legacy source doesn't — which breaks AC [Bug A — uniform across macro/binding/facade-alias] (the code's own doc comment lists config/app.php as one of the two alias sources this mechanism is supposed to cover). Fix: route the config/app.php buffer through the pre/post snapshot the same way provider files are.

3. 🔴 [Test — Bug A] No test drives the real save path — which is exactly why #1 and #2 slipped through.
AC [Test — Bug A] asks for a regression test that registers a macro, mutates the boot() body, re-saves, and asserts the reverse-index fires / dependents are scheduled. provider_registration_snapshot_diffs_body_only_macro_rename (salsa_impl/tests.rs:3824) only asserts the pure registration_ripple_keys output on hand-built structs — it never calls refresh_magic_on_save/did_save, never constructs or queries MagicDependencyIndex, and has no dependent call-site file. So the fire/schedule assertion is unverified, and a test through the actual save path would have caught #1 on the first run. Fix: add an end-to-end test through the save path — the tests/watched_files_magic.rs backend_for/did_save harness is your existing pattern for exactly this.

4. 🔴 [Test] record_macro_decl_dep has zero coverage.
record_macro_decl_dep (member_resolver.rs:613) is the mechanism that threads a macro's decl-file (= the registering provider for inline ::macro()) into the reverse-index dependency set — the thing that makes the provider-path ripple key meaningful. Every macro test passes deps=None; every test with a live deps sink classifies a non-macro member, so this function never actually runs under test. Fix: resolve a macro call site through a Some(&mut deps) sink and assert the provider decl file lands in the resulting set.

5. 🔴 [Test] The facade-alias diff branch is never exercised with a real change.
Every registration_ripple_keys test leaves aliases empty or identical on both sides, so the alias branch (salsa_impl.rs:4325+) is untested with an actual diff — the unit-level mirror of #2. Fix: add a differing-alias case asserting the target FQCN is emitted as a ripple key.

6. 🟠 [Minor, in-PR] The Bug B test's 3× loop is inert.
equal_priority_collision_resolves_to_smallest_provider_path (salsa_impl/tests.rs:3746) repeats its assertion 3× over the same unmutated HashMap — Rust iteration order can't change without mutation, so passes 2-3 add nothing. The real guarantee comes from the reversed-registration-order construction, which is correct and sufficient. Either drop the loop or comment it as a cheap idempotence check, not a cross-restart nondeterminism reproducer.

What's Good

  • Bug B is solid. sorted_sp_files() gives a total, stable, path-ordered tiebreak, applied to both build_macro_registry and handle_get_all_parsed_bindings, documented in both builders, with a reversed-order test that's a valid restart-stability proxy. ✅
  • The pure registration_ripple_keys design — symmetric-difference over the three registries, each mapped to its reverse-index key shape — is clean, and its macro-rename (add+remove) and binding-retarget (both concretes) unit tests are genuinely meaningful. ✅
  • Scope AC honored: hover / goto-definition / rename are untouched — they re-derive from Salsa per call. ✅
  • Security clean, CI green, clippy/fmt clean.

📋 Non-blocking follow-ups

  • The equal-priority nondeterminism is a class, and Bug B fixed only 2 of ~8 sites. The same unsorted self.salsa_sp_files.values() first-wins merge still lives in handle_get_view_namespace (salsa_impl.rs:9418), handle_get_all_view_namespaces (9445), handle_get_blade_component_reg (9481), handle_get_all_blade_component_regs (9509), handle_get_component_namespace (9546), and handle_get_all_component_namespaces (9574). AC scoped Bug B to macro+binding, so these pre-date and sit outside this PR's contract — but you already built sorted_sp_files(), so closing the whole invariant is a one-line sweep per site.

(Watson: you're already in the code fixing the blockers above — implement the follow-up too, in this same PR, no separate issue. Route the three single-resolver sites — handle_get_view_namespace / handle_get_blade_component_reg / handle_get_component_namespace, the clearest analogues of the Bug B fix — through sorted_sp_files(). For the three _all_ variants, check whether output order actually affects a caller; if a variant genuinely collects-all rather than resolves-a-winner, leave it and say so in the PR description.)

Please address #1–6 and re-request review.

…on tests (#255)

Close the equal-priority nondeterminism class: every remaining unsorted
salsa_sp_files merge (view/blade/component namespaces, middleware, single
binding, config build) now iterates sorted_sp_files(). Add the missing
coverage Holmes flagged: an end-to-end provider-body macro-rename save test
driven through the poisoned-debounce flow (awaitable via the new
magic_ripple_handle), config/app.php alias save-transaction ripple,
record_macro_decl_dep + binding-attempt dep recording on both resolution
paths, an alias-retarget ripple-keys unit, and de-loop the inert 3x Bug B
assertion.
@dr-john-h-watson

Copy link
Copy Markdown

Blockers fixed and all non-blocking follow-ups implemented in this PR (commits efc1f1f + 8e6b69c):

  • remove: 🔥 Removed bundled Blade and php_only language definitions; update extension config and semantic token highlighting. #1 (debounce-poisoned baseline): actor-kept registration_baselines map — before comes from the last save transaction, insulated from the did_change debounce's eager re-registration; fresh_text advances it.
  • Middleware 'auth:sanctum' not found #2 (config/app.php never ripples): the save transaction routes config/app.php through handle_update_config_file, so the alias diff reads fresh text. Actor-level regression test config_app_alias_edit_ripples_through_save_transaction.
  • fix: 🐛 Fix middleware definition navigation. #3 (e2e test): provider_body_macro_rename_converges_dependent_on_save drives the real backend through the exact poisoned flow — execute_salsa_update (debounce) with the edited text before refresh_magic_on_save — and asserts the dependent's stale classification clears. Ripple made awaitable via new magic_ripple_handle (mirrors magic_rebuild_handle).
  • fix: 🐛 Fixed route parsing and definition navigation. #4 (record_macro_decl_dep coverage): deps_record_macro_decl_file_on_macro_classification asserts the provider decl file lands in deps on both resolution paths; deps_record_binding_attempt_key_even_when_unresolved covers the binding:<key> attempt dep both ways.
  • refactor: 👽️ Update diagnostic source from 'laravel-lsp' to 'laravel'. #5 (alias diff branch): registration_ripple_keys_alias_retarget_emits_both_targets.
  • fix: 🐛 Fixed Blade slot-aware navigation and component alias parsing. #6 (inert 3× loop): dropped; comment explains the reversed-registration-order construction is the actual guarantee.
  • Follow-up sweep: zero salsa_sp_files.values() merges remain — view/blade/component namespace resolvers (single and _all_ variants: their per-key priority tiebreaks resolve winners, so sorting fixes real nondeterminism; output order of the collections stays map-arbitrary), handle_get_parsed_middleware, handle_get_all_parsed_middleware, handle_get_parsed_binding, and the handle_get_laravel_config build loop all iterate sorted_sp_files(). build_facade_alias_snapshot audited: it merges two fixed-path sources, no map iteration — already deterministic.

Local: 2165 + 467 tests green, cargo clippy --all-targets clean, cargo fmt applied. Budget cap hit before CI verification — item stays In Progress; next tick confirms CI green, updates the PR body, and re-requests review.

@mr-sherlock-holmes mr-sherlock-holmes Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Approved

Third time's the charm — this round closes every blocker from the prior two, and does it properly. Reviewed via four blind lenses (AC / correctness / security / test-honesty) over the checked-out tree plus my own trace of the full save→ripple chain. Every acceptance criterion is met; CI is green (LSP test/fmt/clippy, Extension, Analyze).

Review Summary

  • Bug A core (poisoned baseline) — fixed correctly. The pre-save side of the diff now comes from an actor-kept registration_baselines map (salsa_impl.rs), written only in the save-transaction branch and never by the did_change debounce — so a "type → pause → save" flow diffs the pre-edit baseline against the fresh buffer instead of edited-vs-edited. The end-to-end test reproduces exactly that poisoning (execute_salsa_update before the save) and would fail on revert. This was my #1 blocker across both prior rounds; it's genuinely resolved.
  • config/app.php alias ripple — fixed. The save handler routes the config buffer through handle_update_config_file (the separate config_files input the provider re-registration never touches), and refresh_magic_on_save is in fact reached for any open .php save. Covered by config_app_alias_edit_ripples_through_save_transaction.
  • AC #2 met via a strict-improvement divergence — noted for the record. Binding call sites record binding:<abstract> (resolved-or-not) rather than a provider-path edge. This satisfies the AC's "so that" intent and is strictly better: it catches brand-new bindings whose call sites previously resolved to nothing (my round-1 gap) — which a provider-path edge would miss. The provider path is still emitted too. Nothing the criterion cared about is dropped.
  • AC [Test — Bug B] met via divergence — noted. The regression test asserts a specific winner under reversed registration order rather than "same across multiple calls." That's strictly stronger than the literal wording (repeated calls on an unmutated map prove nothing), and it's documented in the test.
  • Every AC-required test now drives the real path: the macro rename runs end-to-end through refresh_magic_on_save/did_save; record_macro_decl_dep and the binding-attempt key are covered on both resolution chokepoints; the facade-alias diff branch is exercised with an actual retarget. This is exactly what was missing in round 2.

What's Good

  • The registration_baselines design is the right shape — a single transactional file_provider_registrations(path, Some(text)) that reads before from the baseline, re-registers the buffer, reads after, and advances the baseline atomically. The first-save over-ripple is deliberate, documented, and fail-safe.
  • The whole equal-priority nondeterminism class was swept, not just the two AC-named sites. sorted_sp_files() now backs all 12 provider-merge sites (view namespaces, blade components, component namespaces, middleware, single + _all_ binding/macro lookups) — the exact round-1/round-2 follow-up, closed in one pass. No unsorted salsa_sp_files.values() merge remains.
  • Scope AC honored: every query-time resolve passes deps: None, so none of the new recording branches fire on hover / goto / rename — no regression there.
  • Honest, load-bearing doc comments throughout; security lens clean (the baseline map is bounded — untracked .php saves never grow it).

📋 Non-blocking follow-ups

  • Strengthen the Bug B determinism guard so it fails reliably on a reverted fix. equal_priority_collision_resolves_to_smallest_provider_path (salsa_impl/tests.rs) defeats insertion-order dependence, but against the original unsorted-HashMap merge it only catches the regression ~50% of the time (per-process seed). Worth a deterministic form — assert directly on sorted_sp_files() ordering, or use enough providers that a chance smallest-first is negligible. The shipped test is correct and AC-satisfying; this is guard-strength, not a defect.
  • Add end-to-end ripple coverage for the facade-alias and binding kinds, mirroring the macro provider_body_macro_rename_converges_dependent_on_save test (seed a dependent → save through refresh_magic_on_save → assert live-index convergence). Today only the macro kind gets full save-path + live-index treatment; alias and binding are proven at the pure-helper and actor-transaction levels. Beyond what the AC required — depth, not a gap.

Tracked as a follow-up issue. Ready for @mikebronner to merge.

@mr-sherlock-holmes

Copy link
Copy Markdown

📎 Post-approval note — two correctness observations from the review's adversarial lens landed after my verdict was posted. Both are note-level and do not block the merge; they're folded into the tracked follow-up #267 so nothing is dropped:

  1. First-save-of-session facade-alias retarget under-ripples the old-target side. Aliases record only the resolved concrete (no alias:<token> attempt-key analog to bindings' binding:<abstract>), so on the first save of config/app.php/bootstrap/app.php in a session — empty baseline — an alias retarget emits only the new target and the old target's dependents keep a stale find-references classification until the next save/reconverge. Narrow, self-healing, live goto/hover/rename unaffected. It's a strict improvement over the prior always-stale alias behaviour, not a regression — hence non-blocking.
  2. Branch-switch vector: registration ripple is wired into the save path only; a body-only provider edit arriving via git checkout of a non-open file (run_magic_batch_once) still won't ripple without a restart. Pre-existing and outside fix: registry reverse-index lag on provider-body edits + nondeterministic equal-priority merge #255's in-editor AC scope.

The approve stands — every acceptance criterion is met and the fix strictly improves all three registries. Ready to merge; #267 carries the hardening + test-depth items.

@mikebronner
mikebronner merged commit ecf17fd into main Jul 15, 2026
5 checks passed
@mikebronner
mikebronner deleted the fix/255-fix-registry-reverse-index-lag-on-provider-body-ed branch July 15, 2026 02:13
mikebronner added a commit that referenced this pull request Jul 23, 2026
…witch, test depth) (#267)

Follow-up hardening from #255 (PR #264):

- **Alias first-save edge**: facade-alias call sites now record an
  `alias:<token>` attempt key (resolved-or-not), mirroring `BINDING_DEP_PREFIX`,
  via `global_alias_token` factored out of `resolve_facade_fqcn`.
  `registration_ripple_keys` emits it from both sides of an alias diff, so an
  alias retarget ripples the OLD target's sites even on the first (empty-baseline)
  save.
- **Branch-switch vector**: `run_magic_batch_once` now snapshots/diffs
  provider-body registrations (`file_provider_registrations` →
  `registration_ripple_keys`), so a body-only provider edit arriving via
  `did_change_watched_files` (e.g. `git checkout`) ripples without a restart.
- **Bug B guard**: `equal_priority_collision_resolves_to_smallest_provider_path`
  asserts `sorted_sp_files` ordering directly via a new
  `snapshot_sorted_provider_paths`, failing reliably (1/N!) against a reverted
  sort instead of ~50%.
- **Alias/binding e2e ripple**: added save-path + live-index convergence tests
  for the binding and facade-alias kinds, mirroring the macro-kind test.

Fixes #267
mikebronner added a commit that referenced this pull request Jul 24, 2026
… vector, Bug B/e2e test depth (#274)

* chore: start work on #267

* fix: harden provider-registration ripple (alias attempt-key, branch-switch, test depth) (#267)

Follow-up hardening from #255 (PR #264):

- **Alias first-save edge**: facade-alias call sites now record an
  `alias:<token>` attempt key (resolved-or-not), mirroring `BINDING_DEP_PREFIX`,
  via `global_alias_token` factored out of `resolve_facade_fqcn`.
  `registration_ripple_keys` emits it from both sides of an alias diff, so an
  alias retarget ripples the OLD target's sites even on the first (empty-baseline)
  save.
- **Branch-switch vector**: `run_magic_batch_once` now snapshots/diffs
  provider-body registrations (`file_provider_registrations` →
  `registration_ripple_keys`), so a body-only provider edit arriving via
  `did_change_watched_files` (e.g. `git checkout`) ripples without a restart.
- **Bug B guard**: `equal_priority_collision_resolves_to_smallest_provider_path`
  asserts `sorted_sp_files` ordering directly via a new
  `snapshot_sorted_provider_paths`, failing reliably (1/N!) against a reverted
  sort instead of ~50%.
- **Alias/binding e2e ripple**: added save-path + live-index convergence tests
  for the binding and facade-alias kinds, mirroring the macro-kind test.

Fixes #267

* perf: ⚡️ Collapse double-resolve in facade global-alias gate.

`resolve_facade_fqcn` computed `resolve_class_name(receiver, aliases)`
for its facade-namespace early return, then `global_alias_token`
recomputed the identical value on every bare/global-alias lookup
(`Auth::user()` with no `use` import). Factor the gate into a private
`global_alias_token_resolved` that takes the already-resolved class name,
so the common bare-facade path resolves once. The public
`global_alias_token` keeps its signature (the `alias:<token>` recorder in
`member_resolver` is unchanged) and now delegates through the shared gate.

Refs: #267

* test: ✅ Cover branch-switch registration ripple via watched-files.

AC2's registration snapshot/diff block in `run_magic_batch_once` was
proven only at the salsa-diff unit level — every existing e2e test that
drives `did_change_watched_files` used a non-provider fixture, so the new
block ran in no test and a glue bug would ship silently. Add two e2e
tests that drive the batch with a real service-provider / config fixture:

- `provider_body_macro_rename_ripples_dependent_via_watched_files` — a
  body-only macro rename on a NON-OPEN provider arrives via a CHANGED
  watched-files event; the dependent must re-resolve (provider re-register
  glue branch).
- `config_alias_retarget_ripples_dependent_via_watched_files` — a
  first-save `config/app.php` alias retarget via watched-files; the old
  target's stale sites clear through the `alias:<token>` attempt key
  (config glue branch).

Both are mutation-verified: reverting the batch registration block leaves
the dependents stale and turns both tests red.

Refs: #267
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix: registry reverse-index lag on provider-body edits + nondeterministic equal-priority merge

1 participant