Harden provider-registration ripple: alias attempt-key, branch-switch vector, Bug B/e2e test depth

[mikebronner]

Summary

Implements #267 — hardening + test-depth follow-ups from Holmes's review of #255 (PR #264).

Changes

• Alias first-save edge: facade-alias call sites record an alias:<token> attempt key (resolved-or-not, lower-cased), mirroring BINDING_DEP_PREFIX. Factored global_alias_token out of resolve_facade_fqcn so the recorder and the ripple diff share one gate. registration_ripple_keys now emits alias:<token> from both sides, 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 of a non-open provider) ripples to dependents without a restart.
• Bug B guard: equal_priority_collision_resolves_to_smallest_provider_path now asserts sorted_sp_files ordering directly via a new snapshot_sorted_provider_paths handle, failing reliably (1/N!, N=6) against a reverted sort instead of ~50%.
• Alias/binding e2e ripple: added save-path + live-index convergence tests for the binding and facade-alias registration kinds, mirroring provider_body_macro_rename_converges_dependent_on_save.
• Review follow-up (Holmes on Harden provider-registration ripple: alias attempt-key, branch-switch vector, Bug B/e2e test depth #274):
  • Added e2e watched-files coverage that drives the AC2 registration block in run_magic_batch_once with real provider/config fixtures — provider_body_macro_rename_ripples_dependent_via_watched_files (provider re-register glue branch) and config_alias_retarget_ripples_dependent_via_watched_files (config/app.php glue branch + alias:<token> end-to-end). Both mutation-verified: reverting the batch block turns them red.
  • Collapsed the double-resolve on the common bare-facade path — resolve_facade_fqcn reuses its already-computed via_use via a private global_alias_token_resolved; public global_alias_token keeps its signature and delegates through the shared gate.

Acceptance Criteria

• Alias first-save attempt-key (alias:<token>, recorded resolved-or-not; retarget ripples old target)
• Branch-switch vector (run_magic_batch_once snapshots/diffs registrations, proven e2e through the watched-files path)
• Bug B regression guard fails reliably against a reverted sorted_sp_files
• Alias/binding e2e ripple coverage
• New/changed tests pass; macro-kind ripple behavior unchanged

Test Plan

• Targeted tests pass locally: equal_priority_collision_resolves_to_smallest_provider_path, registration_ripple_keys_*, config_app_alias_edit_ripples_through_save_transaction, provider_body_{macro_rename,binding_retarget,alias_first_save_retarget}_*, provider_body_macro_rename_ripples_dependent_via_watched_files, config_alias_retarget_ripples_dependent_via_watched_files
• Full cargo test suite green in CI (2794 tests; fmt + clippy clean)

Fixes #267

[Copilot]

Pull request overview

Hardens the provider-registration “magic ripple” invalidation pipeline in laravel-lsp by adding a stable facade-alias attempt key, extending registration diffing to the watched-files batch (branch-switch/external edit vector), and increasing regression-test depth/observability around deterministic provider merge ordering.

Changes:

• Add alias:<token> reverse-index attempt keys for global facade-alias receivers and emit them from registration_ripple_keys to fix the “first-save alias retarget” under-ripple edge.
• Extend run_magic_batch_once to snapshot/diff provider registrations for watched-file edits so provider-body-only changes ripple without requiring a restart.
• Strengthen Bug B regression coverage by asserting sorted_sp_files ordering directly via a new snapshot_sorted_provider_paths Salsa handle, and add e2e ripple tests for binding + alias kinds.

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
laravel-lsp/src/tests/watched_files_magic.rs	Adds end-to-end watched-files/save-path tests for binding-retarget and alias first-save retarget ripple behavior.
laravel-lsp/src/salsa_impl/tests.rs	Makes Bug B guard more robust by asserting deterministic provider ordering via a new snapshot handle and strengthens alias ripple-key unit coverage.
laravel-lsp/src/salsa_impl.rs	Emits alias:<token> in registration_ripple_keys and adds SnapshotSortedProviderPaths request + handle for test observability.
laravel-lsp/src/member_resolver.rs	Records alias:<token> attempt dependencies for global-alias facade receivers on both live and recipe classify paths.
laravel-lsp/src/main.rs	Extends watched-files batch processing to snapshot/diff provider registrations before refresh to support branch-switch/external edit ripple.
laravel-lsp/src/magic_dependency_index.rs	Introduces ALIAS_DEP_PREFIX and alias_dep_key() helper to keep key construction consistent across call-site recording and diff emission.
laravel-lsp/src/facade_resolver.rs	Factors out global_alias_token() used for both resolver gating and attempt-key recording.

Comments suppressed due to low confidence (1)

laravel-lsp/src/salsa_impl/tests.rs:3769

• This guard can still produce false-negatives if sorted_sp_files() regresses to raw HashMap iteration order (it can coincidentally match the sorted order). Increasing the number of colliding providers reduces the chance of a regression slipping by undetected.

    // Six colliding providers whose class names sort A→F. `P0` is the
    // documented winner; every provider registers the same macro + binding key
    // with a distinct concrete, so a wrong merge order is observable.
    let names = ["Aa", "Bb", "Cc", "Dd", "Ee", "Ff"];
    let providers: Vec<(PathBuf, String, String)> = names

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[mr-sherlock-holmes]

🔄 Changes Requested

Strong work overall — 4 of the 5 ACs are met with genuinely honest tests, CI is green, and the security lens is clean. One real gap blocks: the PR's headline branch-switch vector never runs in any test.

Issues Found

1. 🔴 AC2 (branch-switch vector) is untested — the one new production path is exercised end-to-end only via the save path.
The new registration snapshot/diff in run_magic_batch_once (main.rs ~7184-7206) is the whole point of the second bullet, but no test drives it:

• All three new tests (provider_body_macro_rename_converges_dependent_on_save, provider_body_binding_retarget_converges_dependent_on_save, provider_body_alias_first_save_retarget_converges_dependent in tests/watched_files_magic.rs) call refresh_magic_on_save — the interactive save path, not did_change_watched_files / run_magic_batch_once.
• Every pre-existing test that does drive did_change_watched_files/run_magic_batch_once uses non-provider fixtures (Post/model/controller), so file_provider_registrations returns default() on both snapshot sides and your new block is a no-op for them.
• Verified across the entire test tree: no test satisfies (drives the watched-files path) + (registered service provider) + (asserts a dependent converges after a registration-body edit). So the wiring you added — snapshot before from the baseline, apply fresh content, diff after, feed registration_ripple_keys into changed_classes — is proven only at the salsa-diff unit level, never through run_magic_batch_once itself. A subtle glue bug there (ordering, wrong content, wrong target set) would ship silently.

This is the core of the "branch-switch vector + test depth" deliverable, so it belongs in this PR, not a follow-up. Add an e2e test mirroring the existing watched-files tests but with a service-provider fixture: seed a dependent, deliver a provider-body registration edit through did_change_watched_files (FileChangeType::CHANGED) for a non-open provider, run the batch, assert the dependent's live index converges — at minimum for the macro kind (ideally alias/binding too, which would also pin the run_magic_batch_once path for those keys). Reuse the seed/drain_ripple/member_names skeleton from your new save-path tests.

2. 🔴 (minor, but in the new code) Redundant resolve_class_name on the common bare-facade path.
resolve_facade_fqcn computes let via_use = resolve_class_name(receiver, aliases) inline (facade_resolver.rs:211) for the early-return check, then calls global_alias_token(receiver, aliases, …) (:219), which recomputes the identical resolve_class_name(receiver, aliases) (:244) on every bare/global-alias facade lookup (e.g. Auth::user() with no use import). The extraction introduced the double-resolve. Pass the already-computed value into global_alias_token, or have it accept the token, so the common path resolves once. (Copilot flagged this independently.)

What's Good

• AC1 — alias first-save attempt-key: cleanly done. alias:<token> recorded resolved-or-not through a single shared gate (global_alias_token) consumed by both the recorder and the ripple diff, so an empty-baseline retarget ripples the old target. The provider_body_alias_first_save_retarget_converges_dependent e2e test is a genuine first-save-edge retarget that would fail if the ripple broke — not a tautology.
• AC3 — Bug B guard: now deterministic. Asserts directly on sorted_sp_files() ordering via snapshot_sorted_provider_paths with 6 reverse-registered providers, instead of leaning on chance. Solid fix. (The 1/N! framing in the comment slightly overstates precision since HashMap order isn't provably uniform — the assertion doesn't depend on it, so it's fine as-is; tighten the wording only if you're touching it.)
• AC4 — alias/binding e2e: honest coverage — real seed → save → drain → live-index convergence for both kinds.
• Security lens clean; CI all green.

📋 Non-blocking follow-ups

• Vendor providers don't ripple via the watched-files path — the new registration diff sits after the pre-existing is_vendor short-circuit in run_magic_batch_once (main.rs:7166-7206). Verified this is a deliberate, documented, pre-existing boundary (#259, "preserve the build-pass vendor exclusion"); AC2's "non-open provider" means the user's own providers, not vendor packages. Disposition: Noted — not tracked. If vendor-registration ripple is ever wanted, that's a separate product decision, not latent debt.

(Watson: the blockers above already include everything that belongs to this unit — fix both. The follow-up is unrelated and needs nothing from you.)

Please add the branch-switch test, tidy the double-resolve, and re-request review.

[mr-sherlock-holmes]

Re-dispatched here, but the branch is unchanged since my review at 14:44 — head is still 172a82c, no new commit, no push. My prior 🔄 Changes Requested stands, and both blockers are still open in the current tree:

1. 🔴 AC2 branch-switch vector still untested. The three new provider tests (provider_body_{macro_rename, binding_retarget, alias_first_save_retarget}) all drive refresh_magic_on_save — the save path. Every test that drives did_change_watched_files / run_magic_batch_once still uses a Post/controller fixture, so the new registration snapshot/diff block in run_magic_batch_once (main.rs) executes in no test. That block is the headline of the second AC — it needs an e2e watched-files test with a service-provider fixture.
2. 🔴 Double-resolve still present. resolve_facade_fqcn computes resolve_class_name at facade_resolver.rs:211, then global_alias_token recomputes the identical resolve_class_name at :244 on the common bare-facade path (Auth::user() with no use). Pass the already-resolved value through so the common path resolves once.

Nothing new to review, so I'm not re-submitting a verdict — that would only spend a bounce round toward escalation without any code change to judge. Moving this back to In Progress. Watson: the standing review carries the full detail; add the watched-files provider test and collapse the double-resolve, then re-request review.

[dr-john-h-watson]

Both standing blockers fixed (both belong to this unit); no unrelated cosmetics touched.

1. 🔴 AC2 branch-switch vector now has e2e coverage through run_magic_batch_once. Added two watched-files tests in tests/watched_files_magic.rs that drive did_change_watched_files (FileChangeType::CHANGED) with a real service-provider / config fixture — the save path never runs:

• provider_body_macro_rename_ripples_dependent_via_watched_files — a body-only macro rename on a non-open provider (baseline primed exactly as the sibling save-path tests do; refresh_magic_on_save only reads the documents map, so the provider stays non-open and the watched event isn't skipped). Exercises the provider re-register glue branch.
• config_alias_retarget_ripples_dependent_via_watched_files — a first-save config/app.php alias retarget via watched-files, clearing the old target's stale sites through the alias:<token> attempt key. Exercises the config/app.php glue branch (distinct from the provider one) and pins AC1's attempt key end-to-end through the batch.

Both are mutation-verified: reverting the batch registration snapshot/diff block leaves the dependents stale ({"slugify"} / {"boom"}) and turns both red.

2. 🔴 Double-resolve collapsed. resolve_facade_fqcn now reuses the via_use it already computed by passing it into a new private global_alias_token_resolved; the public global_alias_token keeps its signature (the member_resolver recorder is unchanged) and delegates through the shared gate. The common bare-facade path (Auth::user() with no use) resolves once.

Full suite green locally (2794 tests), cargo fmt/cargo clippy clean. The non-blocking vendor-ripple note needed nothing. Re-requesting review.

[Copilot]

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.

[mr-sherlock-holmes]

✅ Approved

Both round-1 blockers are resolved cleanly, and the whole unit holds up.

Review Summary

• Round-1 blocker #1 (AC2 branch-switch vector was untested) — fixed. Two new end-to-end tests in tests/watched_files_magic.rs now drive the edit-under-test through did_change_watched_files → run_magic_batch_once (the watched-files batch), not the save path, against real service-provider fixtures:
  • provider_body_macro_rename_ripples_dependent_via_watched_files — a body-only macro rename on a non-open provider (a git checkout); the class-surface diff is empty, so only the new registration snapshot/diff block in main.rs (~7184-7207) can ripple it.
  • config_alias_retarget_ripples_dependent_via_watched_files — a first-pass (empty-baseline) config/app.php alias retarget through the batch, pinning the alias:<token> attempt key end-to-end. Baseline is empty, so alias:cache is provably the sole key reaching the stale site.
    Both assertions are load-bearing — verified by reverting the main.rs registration block and watching them fail, then restoring. drain_batch (magic_rebuild_handle) is confirmed distinct from drain_ripple (magic_ripple_handle).
• Round-1 blocker #2 (double-resolve on the bare-facade path) — fixed. global_alias_token_resolved is factored out to accept an already-resolved class name; resolve_facade_fqcn threads the via_use it already computed into it, so Auth::user()-style bare lookups resolve once. global_alias_token remains a thin wrapper. Behaviour is identical across all branches (root-qualified, FACADE_NAMESPACE, backslash-containing, namespaced-bare) — the pre-resolved value passed is exactly resolve_class_name(receiver, aliases), matching the doc contract, and the 16 pre-existing facade tests pass unchanged.

Acceptance Criteria — 5/5 met

• AC1 alias first-save attempt-key ✅ — alias:<token> recorded resolved-or-not at the call site through a single shared gate; empty-baseline retarget ripples the old target. E2e-proven.
• AC2 branch-switch vector ✅ — see above; the new production path is now exercised through the batch itself, not just the save path.
• AC3 Bug B guard ✅ — asserts directly on sorted_sp_files() full ordering with 6 reverse-registered providers; deterministic (≈1/6! chance pass on a reverted sort vs. the old ~50%).
• AC4 alias/binding e2e ripple ✅ — honest seed → save → drain → live-index convergence for both kinds.
• AC5 tests pass / no macro-kind regression ✅ — CI green (LSP test, clippy, fmt, analyze all pass); full suite green including the unmodified macro ripple test.

Security lens clean. Every test is honest and load-bearing — no tautologies, correct paths.

📋 Non-blocking follow-ups

• Vendor-package provider registration edits don't ripple via the watched-files batch — the new registration block sits after the pre-existing is_vendor continue in run_magic_batch_once. This is the deliberate, documented, pre-existing #259 boundary ("preserve the build-pass vendor exclusion"); AC2's "non-open provider" means the user's own providers, not vendor packages. An intentional product boundary, not latent debt. Disposition: Noted — not tracked.
• The alias e2e test covers config/app.php but not bootstrap/app.php — both alias sources merge into the same out.aliases via handle_file_provider_registrations, so there's no distinct untested branch; a bootstrap/app.php variant would be redundant breadth over an already-proven, file-agnostic mechanism. AC1's intent is met. Disposition: Noted — not tracked.

Ready for @mikebronner to merge.