feat: ✨ Modular monoliths: module-local config, providers, and Livewire namespaces

[marlonbasten]

Fixes #297

In a modular monolith (app/{Parent}/{Module}/ with per-module composer.json, config/, providers, views, and translations), the LSP saw none of it: config('module-group.key') was "not found", module-provider loadViewsFrom/loadTranslationsFrom/Blade::directive/Livewire::addNamespace registrations were invisible. This implements the design from #297, including the acceptance criteria added there.

The setting

"laravel-lsp": { "settings": {
  "modules": {
    "paths": ["app/Common/*", "app/*/*"],   // default: [] (off)
    "livewireRegistrars": ["loadLivewireComponentsFrom"]
  }
} }

Strictly opt-in, per the issue's explicit-paths proposal (composer-merge-plugin auto-detection stays out of scope). Unset, empty, or a stale glob that matches nothing on disk behaves exactly like today — pinned by a diff test asserting identical completion output — and a malformed glob entry is a per-entry no-op, never a failure.

Module config

Each module's config/{group}.php merges into the top-level group through one shared helper pair: config_group_files owns discovery and merge ORDER, config_lookup/config_key_locator own value and position resolution over that order — no surface carries its own merge. Documented precedence, mirroring the runtime array_replace_recursive: the project file merges first, then each module in glob-match order, so the last-merged declaration of a key wins and nested arrays merge per key rather than replacing. The same rule applies uniformly to a module group colliding with a core group (app.php) and to two modules defining the same group — each case has its own value-asserting test.

A module-only key resolves on every surface, one test per surface: completion, hover, goto-definition, the "Config not found" diagnostic (with a negative control: it fires again without modules.paths), find-references, rename, and the code lens. Values are obtained by the same static parsing as root config files — a fixture whose values are exit(...)/exec(...) expressions resolves as text, proving nothing is ever executed. No new dependencies.

Module providers — composer-driven

Discovery is driven by each module composer.json's extra.laravel.providers — the list Laravel actually boots via the merged manifests — never by filename convention. Tests pin both directions: a *ServiceProvider.php the manifest doesn't name is NOT indexed, and a provider it names under a non-conventional filename IS. FQCNs resolve through the manifest's own PSR-4 mapping (longest prefix first), with a bounded basename walk as fallback.

An indexed module provider contributes exactly what an app/Providers one does:

• loadViewsFrom registers a view namespace — proven by a fixture reachable through no other mechanism, with a negative control (call removed → resolution fails) and an unlisted-provider control.
• loadTranslationsFrom registers a translation namespace through the Salsa translation layer (the refactor: route translation resolution through Salsa instead of direct fs reads #293/refactor: ♻️ Route translation resolution through Salsa #328 architecture): module providers enter the same provider-file set as app/Providers, ordered before it — so on a namespace conflict the last-registered module wins and an app registration overrides both, matching the config rule. ns::file.key completion now walks every registered namespace, including projects whose only catalogues live under one (no root lang/ at all). Negative-control and collision tests included.
• Blade::directive registrations are discovered — regression-pinned with its own negative control.
• Livewire::addNamespace is extracted in all three argument forms, one test each: positional, named in declared order, named reversed. Configurable wrapper methods (modules.livewireRegistrars) recognize app-defined helpers that call it internally. <livewire:ns::component> resolution is covered by the AC trio: a component reachable only via the namespace resolves, resolution fails when the registration is removed, and a genuinely missing component under the valid namespace still fails. Non-literal path/namespace arguments skip registration without error.

Deliberate deviations from the AC, called out

• Livewire::addNamespace is extracted with the three-parameter signature (namespace, classNamespace, classPath) rather than the AC's two-argument namespace:/path: shape — the three-parameter form is what the modular-monolith layout that motivated the issue actually calls, and the AC's example shape extracts nothing against real code. Unknown parameters (e.g. lazy: true, or anything a future Livewire adds) skip that argument, never the registration.
• Namespaced ns::key translation completion is unconditional, not gated behind modules.paths: it closes the pre-existing refactor: route translation resolution through Salsa instead of direct fs reads #293/refactor: ♻️ Route translation resolution through Salsa #328 gap for every provider-registered namespace, vendor packages included. Called out in docs/configuration.md as a scope addition rather than arriving silently under the opt-in banner; the opt-in diff test covers translation completion alongside config keys.

Glob shapes

app/Common/* and app/*/* are covered by distinct fixtures proving they match at different depths. .. segments are rejected (no escaping the root).

Docs

docs/configuration.md documents the full setting path, type, default-off opt-in guarantee, a config example matching the issue's JSON block, the merge/precedence rules, and livewireRegistrars.

Tests

Full suite: 2562 lib + 575 integration tests green (49 new assertions-bearing tests across config, livewire_namespaces, livewire_resolver, and four integration files; none skip-marked), clippy -D warnings clean. One fixture reproduces the issue's full modular-monolith layout.

[mikebronner]

Reviewed this in depth, including running the suite and probing the new helpers directly. Stating the good part first, because it is substantial: full suite verified green locally (2562 lib + 575 integration), cargo clippy --all-targets -- -D warnings clean, and I could not make anything panic — every malformed input I threw at it degraded gracefully. Broken composer.json, providers given as a string instead of an array, missing extra, absent manifest, .. in a pattern, absolute patterns, trailing slashes, ./ prefixes, doubled slashes — all handled, none crash. Config merge order is deterministic (children sorted within each * expansion, stable across repeated runs), which is exactly right and easy to get wrong.

The issues below are about guarantees that don't hold, not code that falls over. I'd like all of them addressed before merge. One pre-existing item I found while reviewing has been split out to #340 and is explicitly not being asked of this PR.

1. Livewire::addNamespace drops the whole registration on any unknown named argument

collect_arguments returns None when a named argument doesn't match a known parameter, and the ? at the call site discards the entire registration:

// Fully valid, extracted as: []
Livewire::addNamespace(
    namespace: 'common-ui',
    classNamespace: 'App\\Common\\UI\\Livewire',
    classPath: __DIR__.'/../Livewire',
    lazy: true,
);

One extra argument silently disables the namespace, with no diagnostic and no log line. Any future Livewire parameter does the same. An unrecognized named argument should skip that argument, not the call.

Separately, and not blocking: the AC describes a two-argument shape (namespace:, path:) while the implementation requires three (namespace, classNamespace, classPath). Both AC forms currently extract nothing. Your shape is very likely the correct one — it matches real code in the layout that motivated the issue — but it is an undisclosed divergence from the AC, and #335 set a good precedent by calling its deviations out explicitly. A sentence in the PR description would close it.

2. Containment: a fourth private copy of an invariant that was deliberately consolidated

expand_module_dirs gates results with a bare lexical dir.starts_with(root). path_containment.rs exists precisely to prevent that — its module docs open with:

The check used to live in three independent copies … each with its own fallback behaviour that could drift apart (issue #156). They are consolidated here.

That module is the accumulated answer to issues #55, #130, #134, #145, #155, #156, #201, #226 and #228, with four public entry points and a documented fail-open/fail-closed policy for each. The new code reintroduces the exact drift it was written to end.

The doc comment also promises something the code doesn't deliver:

anything escaping the root (e.g. via ..) is dropped

The .. half is true and I couldn't break it. The symlink half is not: Path::is_dir() follows symlinks and starts_with is purely textual, so a symlink under a matched module directory pointing outside the project is followed, and its config/*.php becomes a live config source. Reproduced against expand_module_dirs + config_group_files.

Requested approach — trust configured paths, gate discovered ones:

• modules.paths entries are explicit user configuration, so following symlinks there is correct and should stay. Composer path repositories symlink local packages, sometimes to targets outside the repo, and refusing those would break local package development. Drop the dead starts_with line and document the intent instead of claiming containment. Keep the .. rejection — a pattern escaping the root is a config error, not a deliberate symlink.
• resolve_provider_class_file's walkdir fallback is the other case: those paths are discovered, not configured, so per the #228 convention each entry should go through path_containment::path_within_root_walk_entry.

That split — configured vs discovered — is the distinction path_containment's entry points already encode.

While in there: expand_module_dirs follows symlinks (via is_dir()) but the walkdir fallback uses the default follow_links(false), so a symlinked module's provider is visible to one and invisible to the other. Worth making consistent.

3. No file watcher covers module directories

build_watchers never receives the module dirs. The comment on the new config watcher says module configs are

covered by the PSR-4 app/** glob below when composer maps app/, and this root glob is their floor otherwise

The first clause is true. The second is not — {root}/config/**/*.php cannot match {root}/Modules/Blog/config/app.php. Since modules.paths accepts arbitrary globs, a module tree outside any PSR-4 root (the Modules/* shape) gets no watcher for its config, providers, views, or lang files: edits go unseen until the server restarts.

Requested approach — one watcher pair per expanded module dir, mirroring the PSR-4 loop that already exists a few lines away:

for src_root in psr4_roots {
    let glob = format!("{}/**/*.php", glob_base(src_root));
    …
}

The same loop over module dirs (plus the *.blade.php companion) covers all four resource types, is bounded by the actual module count, and introduces no new concept. The timing works out: pull_and_apply_settings() runs at main.rs:22244 and watcher registration at main.rs:22343 — same initialized flow, settings first — so the expanded dirs are already available when watchers are built.

A root-level {root}/**/config/**/*.php would also work and is cheaper than it looks (vendor/**/*.php is already watched unconditionally), but it fixes only the config third of the gap and would be the first watcher in the set that isn't scoped to a known subtree.

Loose end either way: pull_and_apply_settings also runs on did_change_configuration (main.rs:23122), so changing modules.paths mid-session leaves watchers stale. Either unregister and re-register, or document that it needs a restart.

4. No logging anywhere in the module path

The AC requires a malformed glob entry to be "logged, not a crash". There is no warn!/info!/debug! in expand_module_dirs or module_dirs_for. The no-op half holds; the logged half doesn't.

This matters more than it sounds, because the most likely user typo is silent:

"app/**"   ->  []              (matches a directory literally named `**`)
"app/*"    ->  ["app/A"]
"app/*/*"  ->  ["app/A/B"]

A working config and a typo'd one are indistinguishable from the user's seat. At minimum: log the expanded directory count per pattern, and warn on a pattern that matches nothing.

5. Within-file namespace conflict resolves backwards from runtime

out.entry(prefix).or_insert(reg) keeps the first addNamespace for a prefix. PHP executes both statements and the last one wins in Livewire's registry. Confirmed with a two-call fixture: the extractor reports the first registration's path.

Cross-provider ordering is documented as last-registered-wins; within a single file it's the opposite. The module doc comment states the current behaviour ("First registration wins on prefix conflict, matching provider boot order within a file"), but boot order within a file means the later statement overwrites the earlier one.

6. The opt-in guarantee test compares "off" against "off"

unset_and_stale_glob_produce_identical_output runs modules.paths unset against a stale glob — modules inactive in both cases — over get_all_config_keys() alone. It proves a stale glob equals unset, which is one of the AC's two clauses, but it never compares against pre-change behaviour and it doesn't touch the surface that actually changed.

Which brings up the substantive part: the new namespaced-catalogue block in TranslationCache::completion_keys calls vendor_namespaces() unconditionally. Every project now receives ns::key completions from every namespace-registering vendor package, whether or not modules.paths is set. That is a real behaviour change with the feature off, which the AC specifically forbade.

I think the change itself is right — it's the #293/#328 gap — so I'm not asking you to revert it. I am asking for one of: gate it behind the modules setting, or keep it unconditional and say so plainly in the PR description and docs/configuration.md as a deliberate scope addition rather than something that arrives under an opt-in banner. Either way the diff test should cover translation completion, not just config keys.

7. .rev() is duplicated in both consumers

config_group_files returns ascending precedence, and config_lookup and config_key_locator each call .rev() independently. The AC asked the shared helper to own precedence, not just discovery. A third consumer that forgets the reverse inverts precedence silently, with no test to catch it. Returning descending order from the helper (or exposing an explicit config_group_files_by_precedence) removes the trap.

8. Non-leaf config lookups return one file's subtree, not a merge

resolve_value_with_source returns the first file in which the key resolves. That is correct for leaf keys and for per-key nested resolution — group.a from a module and group.b from the project both resolve, which is what the PR documents. But config('group') for a parent array split across files returns only the winning file's subtree rather than the merged array.

This may well be acceptable; array_replace_recursive semantics for a whole-array hover is a bigger job. I'd like either a test pinning the current behaviour with a comment saying it's intentional, or the merge — your call, but not left undecided.

Not in scope

#340 — translation completion previewing the alphabetically-first locale rather than the configured one — is pre-existing on main. This PR reuses the same rule for namespaced catalogues, which is the right call for consistency. It'll be fixed separately.

[marlonbasten]

@mikebronner Thank you so much for the review! I should've implemented everything now!

[mikebronner]

Round two — and it starts with two acknowledgments, because both are owed.

First: all eight items from the previous review were followed exactly, and
well. I verified each against the tree rather than the commit messages — the
argument-level skips, the gated provider walk, the per-module watcher pairs,
the logging, last-wins within a file, the disclosed translation scan, the
helper-owned precedence, the pinned non-leaf subtree. All real, all tested,
clippy-clean after the upstream merge. Thank you for the care in that round;
the 1–8 enumeration in the fix commit matches what actually landed, which is
rarer than it should be.

Second: this round exists because I swept the full diff instead of
re-checking my own eight items — a sweep I should have done the first time.
Most of what follows was already in the tree when I wrote the first review
and I missed it then. Three items are more than misses: one bug was induced
by my own instruction, one site I explicitly (and wrongly) vouched for, and
two of my "here are the places to fix" lists turned out to be incomplete
lists that you, entirely reasonably, treated as complete. I've tagged each
finding with where the fault actually sits. I'd still like all of them
addressed before merge — but read this as us finishing the review properly,
not as regressions on your side. None of it is speculative; every item is
reproduced at the cited site.

Precedence: the rule the helper now owns is still re-implemented elsewhere

1. Cross-provider Livewire merge is first-wins — module-vs-module inverted

(pre-existing; my previous review explicitly vouched for this layer — that
was wrong)

Item 5 last round said "cross-provider ordering is documented as
last-registered-wins" and only flagged the within-file case. You fixed
exactly what I flagged. But the layer I vouched for doesn't hold:
load_livewire_config (main.rs:9029) does
config.class_namespaces.entry(prefix).or_insert(reg) over provider files
ordered [app/Providers…, modules in ascending precedence], and the comment
above it still reads "First registration wins on prefix conflict" — the
other copy of the phrase item 5 corrected one layer down.

App-beats-module comes out right only because app is scanned first. Two
modules registering the same prefix resolve to the lowest-precedence
module — backwards from the documented rule, and backwards from the
translation-namespace merge in this same PR (.insert(), last-wins, pinned
by conflicting_namespace_registrations_resolve_last_registered_wins). No
test exercises the multi-provider merge; the completion fixtures construct
LivewireConfig directly and bypass it. Fix the merge, fix the comment, add
the two-module collision test the translation side already has.

2. View-namespace precedence contradicts the docs this PR adds

(merge code pre-existing; the module wiring and the docs claim are new; my
first review never looked here)

docs/configuration.md now promises "last-registered module provider wins;
an app/Providers registration overrides modules." The merge underneath
(salsa_impl.rs:10151) is lexicographic-path first-wins — literally commented
"Keep existing (first wins for now)" — and this PR newly feeds module
providers into that set. app/Legal/…/Providers/X.php sorts before
app/Providers/AppServiceProvider.php, so a module registration beats the
app's — the opposite of the documented rule. Priority-aware merging already
exists forty lines down (class_component_files, existing_prio >= prio);
use it, and pin app-vs-module and module-vs-module with tests.

3. Config-key completion carries its own parallel merge

(pre-existing in this PR's feature commits; item 7's "both consumers" list
was mine, and it was incomplete — you de-.rev()'d exactly what I named)

get_all_config_keys (main.rs:14524–14595) hand-builds the config-dir list
and re-implements last-wins via BTreeMap::insert, agreeing with
config_group_files only because the iteration order happens to match.
Enumeration (all keys) and per-group lookup are legitimately different
shapes — but the precedence rule itself must come from one place, or a
future change lands in one implementation and silently not the other. Route
the precedence decision through the helper (or bind the two with a test that
would fail on divergence).

Containment: the configured-vs-discovered split, finished

4. The PSR-4 provider branch is the gated fallback's unguarded twin

(pre-existing; item 2 named "the walkdir fallback" as "the other case" —
my enumeration implied completeness and missed this branch three lines up)

resolve_provider_class_file's PSR-4 arm (config.rs:1266) joins
module_dir.join(dir) where dir comes straight from composer.json's
autoload.psr-4 value. Path::join with an absolute value discards the
base entirely; .. segments walk out lexically and the OS resolves them at
is_file(). The result is read (read_to_string at four call sites). These
are manifest-derived paths — discovered, by the split's own definition. Gate
the candidate against module_dir and add an absolute/.. psr-4 entry to
the escape suite (the existing symlink test forces the fallback path and
never reaches this branch).

5. PSR-4 prefix matching accepts non-boundary textual matches

(pre-existing; plain first-review miss)

Same function, one line up: .strip_prefix(prefix_trimmed).and_then(|r| r.strip_prefix('\\').or(Some(r)).filter(…)). The .or(Some(r)) fallback
accepts a remainder that does not start with \ — so prefix
App\Legal\ContractManagement matches FQCN
App\Legal\ContractManagementSupport\Provider, and the longest-prefix
tie-break can prefer the bogus candidate over the true mapping. Composer
keeps the trailing \ in the comparison and rejects this. The fallback is
only legitimate for the empty PSR-4 prefix ("": "src/"); restrict it to
that case and pin overlapping prefixes with a test.

6. Livewire class_path is resolved, canonicalized, and never checked

(pre-existing; plain first-review miss — item 2 discussed containment only
for expand_module_dirs and the provider walk)

classify_add_namespace (livewire_namespaces.rs:142–148) receives root
and uses it only as a join base; the final class_path is canonicalized but
never containment-checked, and path_join's contract places that burden on
the caller. The value is then consumed twice without a gate: the completion
walk (main.rs:14936) runs WalkDir::new(&reg.class_path).follow_links(true)
with no max_depth and no walk-entry gate — a __DIR__.'/../../../..' in a
provider walks everything reachable on every completion request, a hang
hazard before it is anything else — and try_namespaced_class
(livewire_resolver.rs:690) joins + is_file()s into
LivewireComponent.paths, which blade_backing_class_sources reads. These
are provider-source-derived paths: discovered. Gate both consumers (or the
registration at extraction time), bound the walk, add the escape test.

Test debt

7. The rename surface has no test

(pre-existing gap; my first review checked the opt-in diff test and not
this)

collect_config_declaration_target (main.rs:21783) changed to a multi-file
rewrite (Vec<EditTarget> — "rewritten in every declaring file, or the
survivors would resurrect the old key") and has zero test references; the
test file's own header says "the seven surfaces, one test each" and delivers
six. One test: rename a key declared in two files, assert both edits.

8. modules.livewireRegistrars is never tested with a non-default name

(pre-existing gap; first-review miss)

Every registrar test uses the shipped default
(loadLivewireComponentsFrom). Nothing proves the setting is honored rather
than the extractor being hardcoded. One test with a custom wrapper name
closes it.

9. An unparseable positional argument shifts later positionals left

(this one is on me — it follows directly from item 1's instruction, which
specified "skip that argument" without saying what a skipped positional does
to the slots)

In collect_arguments (livewire_namespaces.rs:209), a positional argument
whose value doesn't parse continues without consuming its slot, so later
positionals land one slot early. With the three-parameter signature a
shifted call happens to end up dropped, so it's latent rather than live —
but the guarantee item 1 asked for ("skips that argument") should hold
precisely: a skipped positional should still consume its slot. Two-line
fix; test: leading $variable positional followed by literals extracts
nothing.

Riders, no blocking weight: the watcher-dedup comment claims a nested module
dir "costs nothing extra" but the dedup is exact-string only — fix the
comment; expand_module_dirs's documented symlink-following is never driven
through the glob expansion itself; the two named-argument Livewire tests are
labeled "declared order"/"any order" but neither permutation is the declared
order — rename them; the zero-match warn! is asserted by behavior, not
captured tracing output.

Settled, to prevent churn

The unconditional ns:: translation completion stays as disclosed (docs +
PR description — that adjudication holds). The config watcher's 18→19
assertion change was visible during the item-3 discussion and is accepted.
Non-leaf subtree behavior is pinned by design per item 8 of the last round.
None of these need touching again.

[marlonbasten]

All nine plus the riders, in 793ac93. Two places where I went past the letter of the finding, both because the narrow fix would not have held:

2. Priority-aware merging alone was not sufficient — module providers register at priority 2, the same tier as app providers, so app-over-module would still have depended on iteration order. Modules now have their own tier (0=framework, 1=package, 2=module, 3=app), so the documented rule holds by rank and last-wins only breaks ties within a tier. Both directions pinned; docs/configuration.md records the tier.

6. Took the extraction-time gate rather than gating both consumers — one gate, and a third consumer cannot reintroduce the hole. I bounded the completion walk anyway (max_depth(8)), since the hang hazard is independent of containment.

3. Enumeration still discovers which groups exist (that genuinely is a different shape), but per-group file order now comes from config_group_files, and a test asserts completion's winning source equals the helper's first file — so divergence fails rather than drifts.

Everything else followed the finding as written: the Livewire merge scans modules first and app last with insert(), the PSR-4 arm is lexically gated before probing and matches only at a namespace boundary (empty catch-all prefix excepted), a skipped positional consumes its slot, and the rename and custom-registrar surfaces have their tests. Riders done too — the watcher-dedup comment no longer overclaims, symlink following is driven through the glob expansion itself, and the two named-argument tests are renamed to the permutations they actually cover.

One follow-up commit, 78c5012: the new precedence-binding test compared path strings and failed on the windows leg — the fixture builds its module path from a forward-slash literal, so the two sides spell the same path with different separators. Compared by components now.

On the framing: no need to apportion fault. Items 1, 4 and 9 followed from lists and instructions that were reasonable to read as complete, and I read them that way — the sweep catching them is the review working, not either of us dropping something. Thanks for doing the full pass.

[marlonbasten]

@mikebronner I'm off for now, thanks for the reviews again and have a nice evening! 🙏

[mikebronner]

@marlonbasten Thanks so much! Go to bed already hehe :) I'll try to get this all sorted out and resubmitted to Zed tomorrow.