Skip to content

refactor: πŸ‘½οΈ Update diagnostic source from 'laravel-lsp' to 'laravel'. - #5

Merged
mikebronner merged 1 commit into
mainfrom
update-lsp-reference
May 12, 2026
Merged

mikebronner merged 1 commit into
mainfrom
update-lsp-reference

Conversation

@mikebronner

Copy link
Copy Markdown
Contributor

No description provided.

@mikebronner mikebronner self-assigned this May 12, 2026
@mikebronner
mikebronner merged commit 9a9eb09 into main May 12, 2026
@mikebronner
mikebronner deleted the update-lsp-reference branch May 12, 2026 15:31
mikebronner added a commit that referenced this pull request Jun 13, 2026
Resolve the four blockers from Holmes's review of the scope-aware Blade
variable / controller→view binding rename:

- Correctness: collect_variable_name_spans no longer descends into a
  non-capturing anonymous closure, so renaming a compact() binding stops
  clobbering an unrelated `function () { $name ... }` body variable. Arrow
  functions and `use ($name)` closures still descend β€” they share the outer
  variable.
- Security: view_binding_rename_edit validates the source view name with
  validate_view_name and constrains the resolved view path under the project
  root before any disk read, closing a path traversal where a hostile
  `view('/tmp/evil', ...)` could read an out-of-project file and leak its
  absolute path into the WorkspaceEdit.
- AC #5: prepare_blade_var_rename now gates on is_template_variable, rejecting
  variables that never surface in template markup (undefined names, @php-block
  / function-locals) instead of accepting any $var.
- AC #6: extract a pure binding_rename_spans cross-file core and drive
  view_binding_rename_edit through it, with tests exercising the real
  controller<->view orchestration (array-key + compact) and the
  multi-controller non-contamination case.

Also tightens string_literal_text (matching quotes) and string_content_span_at
(cursor strictly before the closing quote), and makes for_loop_scopes_rename
assert the exact span set, per the review's non-blocking notes.

Tests: 1631 lib tests pass (+8 new); cargo fmt + clippy -D warnings clean.
mikebronner added a commit that referenced this pull request Jun 14, 2026
…ontroller) (#89)

* chore: start work on #55

* feat: ✨ scope-aware Blade variable rename + controllerβ†’view binding rename

Add Blade template variable rename to the rename engine (issue #55):

- Scope-aware in-template rename: a variable introduced by @foreach /
  @forelse / @for is block-scoped; a file-level variable (controller-passed
  or @php-assigned) is file-scoped but skips any nested loop that re-binds
  the same name. Blade comments and @verbatim regions are masked out.
- Controller→view binding rename: F2 on a view-data key in
  view('v', ['key' => …]) or compact('key') rewrites the controller key,
  the in-view file-scoped $key usages, and (for compact) the enclosing
  method's local $key so the controller stays valid.

New pure module src/blade_var_rename.rs (fully unit-tested), wired into the
prepare_rename / rename handlers ahead of the literal-symbol classifier.
Docs and README updated to move the feature from Planned to shipped.

Refs #55

* fix: πŸ› address PR #89 review on Blade variable rename (#55)

Resolve the four blockers from Holmes's review of the scope-aware Blade
variable / controller→view binding rename:

- Correctness: collect_variable_name_spans no longer descends into a
  non-capturing anonymous closure, so renaming a compact() binding stops
  clobbering an unrelated `function () { $name ... }` body variable. Arrow
  functions and `use ($name)` closures still descend β€” they share the outer
  variable.
- Security: view_binding_rename_edit validates the source view name with
  validate_view_name and constrains the resolved view path under the project
  root before any disk read, closing a path traversal where a hostile
  `view('/tmp/evil', ...)` could read an out-of-project file and leak its
  absolute path into the WorkspaceEdit.
- AC #5: prepare_blade_var_rename now gates on is_template_variable, rejecting
  variables that never surface in template markup (undefined names, @php-block
  / function-locals) instead of accepting any $var.
- AC #6: extract a pure binding_rename_spans cross-file core and drive
  view_binding_rename_edit through it, with tests exercising the real
  controller<->view orchestration (array-key + compact) and the
  multi-controller non-contamination case.

Also tightens string_literal_text (matching quotes) and string_content_span_at
(cursor strictly before the closing quote), and makes for_loop_scopes_rename
assert the exact span set, per the review's non-blocking notes.

Tests: 1631 lib tests pass (+8 new); cargo fmt + clippy -D warnings clean.

* fix: πŸ› Scope compact() rename to its enclosing closure.

`enclosing_function_local_spans` matched only the legacy
`anonymous_function_creation_expression` node kind, which does not exist in
tree-sitter-php 0.24 (the closure kind is `anonymous_function`). A
`compact('name')` inside a route closure therefore never elected the closure
as its scope, fell back to the whole-file root, and renamed unrelated `$name`
occurrences in sibling closures. Match both names, per repo convention.

Also addresses two review notes flagged alongside: anchor `mask_php_blocks`
on a word boundary so `@phpunit` / `@phpdoc` no longer trips a spurious `@php`
mask region, and correct the `view_binding_key_at` docstring to drop the
unimplemented `->with` chained forms.

Refs: #55

* fix: πŸ› balance parens in Blade loop scope detection (#55)

@foreach/@forelse loop arguments were captured with a `\([^)]*\)` regex that
stops at the first `)`, so an iterable containing a call β€”
`@foreach($users->where('active', true) as $user)` β€” truncated to
`($users->where('active', true)`, dropping the ` as $user` tail. The loop
then registered no variables, its scope went invisible, and the file-scope
arm admitted every `$var` β€” the cross-scope clobber AC #1 forbids.

Capture the loop argument list by balancing parens (ignoring parens inside
string literals) and recover the binding by splitting on the last ` as `
instead of a `[^)]` run, so parenthesized iterables keep their variable.

Also addresses PR #89 round-3 non-blocking notes:
- canonicalize both sides of the view-path root-containment guard before the
  textual `starts_with` check, so a symlink can't leak an out-of-project path
  into the WorkspaceEdit.
- document the namespaced-view (`package::view`) limitation on
  `validate_view_name`.

Regression tests: method-call and nested-paren iterables in blade_loops, plus
in_scope_spans loop-scoping for a parenthesized iterable.

* fix: πŸ› Balance Blade loop directive parens across wrapped lines

A @foreach/@forelse/@for/@while header whose argument list wraps across
physical lines was never registered as a loop block β€” the per-line paren
scan gave up at the first line end β€” so a rename inside it silently
clobbered the whole file. Balance the directive parens across continuation
lines (one code path; single-line fast path unchanged). Refs #55

* fix: πŸ› Harden Blade variable rename against fail-open clobbers

The scope engine renamed file-wide whenever it couldn't resolve a loop's
binding β€” a silent clobber on valid templates. Root-cause fixes:

- Resolve loop blocks over the comment/@verbatim-masked source (as
  variable_spans already does), so a {{-- --}} comment in or around a loop
  header no longer desyncs the parse or forms a phantom binding block.
- Recover every binding shape β€” array/list() destructuring, by-reference
  (&$item), key + destructured value β€” by extracting every $ident from the
  foreach target instead of matching one fixed pattern.
- Refuse $loop (reserved, never header-bound) at the prepare gate.
- Fail-closed backstop: when loop scope can't be resolved β€” an opaque
  @foreach/@forelse (binding unparseable) or a broken (unbalanced-paren)
  header β€” refuse the rename and never clobber into the region, rather
  than admitting file-wide spans.

15 regression tests cover each shape. Refs #55
mikebronner added a commit that referenced this pull request Jun 18, 2026
…root targets

Add a containment backstop at the file-create seam (FileAction::build_code_action,
issue #199 AC #5/#6): refuse to offer any create quick-fix (View, BladeComponent,
Livewire, Inertia, …) whose target_path escapes the project root, returning None
instead of constructing an out-of-root ResourceOp::Create. Completes the
"fail-closed containment on every FS-touching path" sweep (#130 β†’ #143 β†’ #148 β†’
#194) across the third surface β€” the write seam, alongside the read/resolve paths.

The guard uses path_within_root_lexical, NOT the fail-closed path_within_root the
sibling read paths use: a create target never exists yet, so path.canonicalize()
always fails for it and the fail-closed guard would refuse *every* create,
including legitimate in-root ones. The lexical guard refuses out-of-root and
interior-`..` escapes while admitting a not-yet-created in-root target, and still
canonicalizes to catch symlink escapes when the target exists. (AC #5 named
path_within_root; flagged on the PR β€” same class of AC defect as the #3/#4
dispute resolved via Option 1.)

Add code_action_create_containment.rs: out-of-root, interior-`..`, in-root
positive control, and under-root symlink-escape cases.

Reframe the resolve-seam negative tests per the #199 escalation (Option 1): they
assert the *invariant* (an out-of-root / symlink-escaping component never
resolves), with the resolve_component_file guard documented as the innermost
backstop whose reject branch is unreachable via the public API today β€” the
upstream path_within_root_lexical filter catches both negatives first. Drop the
false "only path_within_root catches" claim and correct the misleading assert
messages.

Fixes: #199

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
mikebronner added a commit that referenced this pull request Jun 18, 2026
…for containment-invariant uniformity (#203)

* chore: start work on #199

* fix: πŸ”’οΈ Add fail-closed path_within_root guard to resolve_component_file.

Mirror the sibling resolve_component_existing_file: after the
file_exists_cached check, refuse any candidate whose real path escapes
config.root before returning it. resolve_component_path already drops
out-of-root candidates with the lexical path_within_root_lexical filter,
so this is defense-in-depth β€” it makes the fail-closed containment
invariant hold uniformly across every FS-touching component resolver
(the #130 β†’ #143 β†’ #148 β†’ #194 chain).

Add component_file_navigation_containment.rs pinning the invariant at the
resolve_component_file boundary: out-of-root and under-root-symlink
escapes return None, in-root files still resolve.

Fixes: #199

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

* fix: πŸ”’οΈ Guard the build_code_action write/create seam against out-of-root targets

Add a containment backstop at the file-create seam (FileAction::build_code_action,
issue #199 AC #5/#6): refuse to offer any create quick-fix (View, BladeComponent,
Livewire, Inertia, …) whose target_path escapes the project root, returning None
instead of constructing an out-of-root ResourceOp::Create. Completes the
"fail-closed containment on every FS-touching path" sweep (#130 β†’ #143 β†’ #148 β†’
#194) across the third surface β€” the write seam, alongside the read/resolve paths.

The guard uses path_within_root_lexical, NOT the fail-closed path_within_root the
sibling read paths use: a create target never exists yet, so path.canonicalize()
always fails for it and the fail-closed guard would refuse *every* create,
including legitimate in-root ones. The lexical guard refuses out-of-root and
interior-`..` escapes while admitting a not-yet-created in-root target, and still
canonicalizes to catch symlink escapes when the target exists. (AC #5 named
path_within_root; flagged on the PR β€” same class of AC defect as the #3/#4
dispute resolved via Option 1.)

Add code_action_create_containment.rs: out-of-root, interior-`..`, in-root
positive control, and under-root symlink-escape cases.

Reframe the resolve-seam negative tests per the #199 escalation (Option 1): they
assert the *invariant* (an out-of-root / symlink-escaping component never
resolves), with the resolve_component_file guard documented as the innermost
backstop whose reject branch is unreachable via the public API today β€” the
upstream path_within_root_lexical filter catches both negatives first. Drop the
false "only path_within_root catches" claim and correct the misleading assert
messages.

Fixes: #199

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

* fix: πŸ”’οΈ Guard sibling create paths in build_code_action multi-file actions

The write/create-seam backstop (issue #199) only validated `self.target_path`,
but the multi-file action types each emit a SECOND `ResourceOp::Create`:

- `Livewire` β†’ a Blade view at `get_livewire_view_path`
- `BladeComponentWithClass` β†’ a PHP class at `get_component_class_path`

Both paths are derived from `self.name` β€” an independent diagnostic field, not
coupled to `target_path`. Because `PathBuf::join`/`push` of an absolute-looking
segment *replaces* the base, a forged diagnostic with an in-root `target_path`
and `name = "/etc/passwd"` slipped past the guard and materialised a file outside
the project root via the second create β€” the exact containment escape this chain
exists to close.

Guard each sibling path with `path_within_root_lexical(&path, root)` and return
`None` on escape, and correct the guard comment that wrongly asserted every
create materialises only `target_path`.

Adds non-vacuous multi-file coverage in `code_action_create_containment.rs`: a
`Livewire` / `BladeComponentWithClass` action with an in-root `target_path` but an
escaping `name`-derived view/class path returns `None` (each would be `Some`
without the guard), plus in-root positive controls for both types.

Refs #199

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
mikebronner added a commit that referenced this pull request Jun 18, 2026
… the emit-safe guard.

The component-diagnostic loop (main.rs:16683) built its "Expected at:" hint
from an unfiltered `possible_paths.first()` β€” the fourth and last surface in
the `from_diagnostic` β†’ `CreateFile` family, still echoing a raw candidate
after the view()/@extends/@include loops were closed. A dangling under-root
symlink (or out-of-root namespace path) planted at the conventional component
path was echoed into the message, parsed back into a create target, passed the
lexical-only #199 backstop, and a client `CreateFile` could follow it out of
the project tree. Route it through `in_root_expected_path_hint` so all four
"Expected at:" surfaces share the emit-safe guard.

Also folds in the test hardenings from the same review round:
- New `emit_safe_refuses_unverifiable_candidate_behind_a_symlink_loop` pins the
  `ELOOP` fail-closed case of the emit-safe `None` arm (deterministic on every
  unix uid), completing the EACCES/ENOTDIR/ELOOP trio named in its contract.
- `emit_safe_refuses_out_of_root_candidate` now writes the escapee to disk and
  asserts the precondition, so it unambiguously proves an *existing* out-of-root
  file is refused, not merely an absent one.
- `expected_path_hint_picks_first_in_root_candidate` gains the
  `canonicalize().is_err()` precondition matching its sibling tests.
- New `component_expected_path_hint_is_unknown_when_all_candidates_out_of_root`
  regression test mirrors AC #5 for the component surface.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
mikebronner added a commit that referenced this pull request Jun 18, 2026
…stic surfaces (Livewire diagnostic fallback + "Expected at:" message hint) (#202)

* chore: start work on #201

* fix: πŸ”’οΈ Extend path containment guard to remaining diagnostic surfaces.

Route the Livewire "component not found" diagnostic fallback through
any_in_root_candidate_exists so an out-of-root loadViewsFrom/
component_namespaces registration can no longer make diagnostics
stat-probe files outside the project tree, nor let an out-of-root view
that exists on disk silently satisfy the check.

Source the "Expected at:" message hint from the first in-root candidate
(new in_root_expected_path_hint helper, shared by the view() and
@extends/@include loops) so a maliciously-registered out-of-root
namespace path is never echoed back to the client in the message text.

Add Livewire-fallback and message-hint cases to view_diagnostic_containment.

Fixes: #201

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

* fix: πŸ”’οΈ Refuse dangling under-root symlinks in the "Expected at:" diagnostic hint

The "Expected at:" create-this-file hint baked into a "view not found"
diagnostic is parsed back into a CreateFile target by `from_diagnostic`, so
it is an *emitted* path. The branch sourced it via `path_within_root_lexical`,
whose docstring explicitly disclaims emitted paths β€” its `unwrap_or(true)`
admits a dangling under-root symlink whose target is missing, which a client
`CreateFile` could follow out of the project tree (issues #134/#155).

Add a third containment entry point, `path_within_root_emit_safe`: it shares
the lexical out-of-root gate and still admits a genuinely-absent in-root path
(the hint is for a missing view, so fail-closing would break "create view"),
but refuses a leaf dangling symlink via `symlink_metadata` β€” the residual the
lexical guard leaves open. Point `in_root_expected_path_hint` at it (both
"Expected at:" call sites share the helper, AC #3/#4). Extract the shared
lexical gate into `lexically_in_root` to keep the module's single-source-of-
truth invariant.

Closes Holmes's escalated AC-vs-documented-contract dispute on PR #202, per
the decision to close the residual in this PR rather than defer it.

Refs #201
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

* fix: πŸ”’οΈ Refuse non-NotFound lstat errors in emit-safe containment guard

`path_within_root_emit_safe`'s `None` arm tested `symlink_metadata().is_err()`,
which is true for EVERY lstat error β€” not just `NotFound`. A lexically-in-root
candidate that fails to canonicalize and whose lstat returns `EACCES` (a
no-search-permission parent hiding a dangling under-root symlink), `ENOTDIR`,
or `ELOOP` was therefore ADMITTED and echoed into the "Expected at:" diagnostic
hint, where a client `CreateFile` could follow it out of the project tree β€”
the exact escape this guard exists to refuse. The doc comment already specified
the discriminating condition as `NotFound`; the code failed open against it.

Discriminate on `NotFound` so the guard fails closed on anything it cannot
prove absent, admitting only a genuinely-absent speculative create target.

- `is_err()` β†’ `is_err_and(|e| e.kind() == ErrorKind::NotFound)`
- Tighten the doc comment to state non-`NotFound` errors fail closed
- Add two `#[cfg(unix)]` tests pinning the corrected contract: an unverifiable
  candidate behind a file path component (`ENOTDIR`, deterministic) and a
  dangling under-root symlink behind a no-search-permission parent (`EACCES`,
  the case Holmes named) must both be refused

Addresses Holmes's review blocker on PR #202.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

* fix: πŸ”’οΈ Route the "Blade component not found" diagnostic hint through the emit-safe guard.

The component-diagnostic loop (main.rs:16683) built its "Expected at:" hint
from an unfiltered `possible_paths.first()` β€” the fourth and last surface in
the `from_diagnostic` β†’ `CreateFile` family, still echoing a raw candidate
after the view()/@extends/@include loops were closed. A dangling under-root
symlink (or out-of-root namespace path) planted at the conventional component
path was echoed into the message, parsed back into a create target, passed the
lexical-only #199 backstop, and a client `CreateFile` could follow it out of
the project tree. Route it through `in_root_expected_path_hint` so all four
"Expected at:" surfaces share the emit-safe guard.

Also folds in the test hardenings from the same review round:
- New `emit_safe_refuses_unverifiable_candidate_behind_a_symlink_loop` pins the
  `ELOOP` fail-closed case of the emit-safe `None` arm (deterministic on every
  unix uid), completing the EACCES/ENOTDIR/ELOOP trio named in its contract.
- `emit_safe_refuses_out_of_root_candidate` now writes the escapee to disk and
  asserts the precondition, so it unambiguously proves an *existing* out-of-root
  file is refused, not merely an absent one.
- `expected_path_hint_picks_first_in_root_candidate` gains the
  `canonicalize().is_err()` precondition matching its sibling tests.
- New `component_expected_path_hint_is_unknown_when_all_candidates_out_of_root`
  regression test mirrors AC #5 for the component surface.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
mikebronner added a commit that referenced this pull request Jul 15, 2026
The #246 strict clear fired for ANY unresolvable pending hop once the
chain reached EloquentCollection mode β€” but heuristic guesses (local
scopes like User::forCurrentTenant(), unmodeled builder methods) share
the pending-hops queue with genuine relation claims. A scope followed
by ->get() flipped the mode, the scope name failed to resolve as a
relation, and effective_model was cleared β€” silencing column-typo
diagnostics that main correctly flagged on the root table.

- chain: type the queue β€” RelationHop { name, kind } with
  RelationHopKind::{Claim, Heuristic} replacing the untyped Vec<String>
- cursor: the RelationProperty receiver seed (property access or
  executed relation assignment) is a Claim; mid-chain unknowns are
  Heuristic
- finalize: a miss clears effective_model only for Claim hops;
  Heuristic misses keep the skip-and-continue fallback in every mode
- tests: regression coverage for the scope-then-terminator shape
  (diagnostics + unit), plus a dedicated end-to-end completion test for
  the executed-relation collection shape (review follow-up, AC #5)
mikebronner added a commit that referenced this pull request Jul 15, 2026
* chore: start work on #246

* fix: πŸ› type executed relation-query collections against the related model

$var = $user->competitions()->select(…)->get() previously typed $var as an
EloquentBuilder on the ROOT model (User), so collection chains on it
false-positived "Column not found" against the users table.

- flow: detect the executed-relation assignment shape (relation call at the
  chain root, recognised builder methods between, COLLECTION_TERMINATOR on
  top; instance or static Eloquent root) via resolve_collection_relation
- extractor: emit the RelationProperty receiver for it β€” collection mode at
  the base model with the relation queued as a pending hop, resolved by the
  existing async finalize step
- finalize: a collection-mode hop that fails to resolve now clears
  effective_model (stay quiet) instead of falling back to the root model;
  builder-mode heuristic hops keep the skip fallback

Fixes #246

* fix: πŸ› gate strict relation-hop clear on claim hops, not collection mode

The #246 strict clear fired for ANY unresolvable pending hop once the
chain reached EloquentCollection mode β€” but heuristic guesses (local
scopes like User::forCurrentTenant(), unmodeled builder methods) share
the pending-hops queue with genuine relation claims. A scope followed
by ->get() flipped the mode, the scope name failed to resolve as a
relation, and effective_model was cleared β€” silencing column-typo
diagnostics that main correctly flagged on the root table.

- chain: type the queue β€” RelationHop { name, kind } with
  RelationHopKind::{Claim, Heuristic} replacing the untyped Vec<String>
- cursor: the RelationProperty receiver seed (property access or
  executed relation assignment) is a Claim; mid-chain unknowns are
  Heuristic
- finalize: a miss clears effective_model only for Claim hops;
  Heuristic misses keep the skip-and-continue fallback in every mode
- tests: regression coverage for the scope-then-terminator shape
  (diagnostics + unit), plus a dedicated end-to-end completion test for
  the executed-relation collection shape (review follow-up, AC #5)

* fix: πŸ› discriminate scope calls and mode-flips in executed-relation collections

* fix: πŸ› collect unrecognised middle calls as heuristic hops in executed-relation collections

The resolve_collection_relation shape gate rejected any assignment chain
with an unrecognised middle call, so idiomatic Laravel chains kept the
original #246 false positive: pivot builder methods (->wherePivot(...),
absent from the recognised-builder catalog), scopes on the related model
(->competitions()->approved()->get()), and root scopes before the
relation (->forCurrentTenant()->competitions()->get()).

Mirror the inline cursor collector: unrecognised middles no longer
reject β€” each becomes a RelationHopKind::Heuristic hop carried on the
receiver (call_hops) and resolved in source order after the relation
claim by the shared finalize step; a miss keeps the running model. The
round-3 mode-flip gate is unchanged: a recognised middle whose
ChainEffect isn't None (mid-chain get()/toBase()/first()) still rejects.

Proactive round-4 hardening per human review direction on PR #266.
mikebronner added a commit that referenced this pull request Aug 28, 2026
`get_phpunit_env_context` parses two PHPUnit spellings, `<env name="` and
`<server name="`, through separate arms of one match β€” different literals,
different offsets (`e + 11` against `s + 14`). The suite drove the first
spelling only, so the second had never reached `completion()` anywhere in the
crate, and the pull request re-quoted AC #5 without its `<server name="…">`
clause and without noting the narrowing.

Two tests close it: the typed prefix and the empty-prefix repro, mirroring the
`<env name="` pair. Both pin the offset arithmetic β€” rewriting `s + 14` to
`s + 11` fails these two tests and nothing else in the crate.

The module doc comment claimed that restoring the deleted `std::env::vars()`
loop turns every test in the file red. It does not. The shadowing test's fixture
`.env` declares the process variable's own name, so the loop's own
`!seen_names.contains(&name)` guard skipped that variable before this fix as
well. The comment now says nine of ten, names the test that stays green, and
names the second mutation that discriminates it. Two neighbouring claims in the
same block are corrected with it: the context list now carries both PHPUnit
spellings, and the "each test asserts a positive control" claim now records its
one deliberate exception.

Verified by running the mutations, not by inspection: restoring the loop fails 9
of 10, the `.env` echo-format mutation fails the tenth, the offset mutation fails
exactly the two new tests.

Watson-Branch: #342
mikebronner added a commit that referenced this pull request Aug 28, 2026
…s inline (#343)

* chore: start work on #342

Watson-Branch: #342

* fix: πŸ› Stop offering the LSP process environment in env completion.

The env-completion handler merged `std::env::vars()` into its item list and
rendered each value inline in `detail` and `documentation`. The language server
inherits Zed's environment, which inherits the login shell's, so any credential
whose name matched the typed prefix was offered with its value on screen β€” worst
during screen-sharing and recording, when a completion popup is most visible.

It was also wrong independent of secrets: `${...}` interpolation inside `.env`
and `env()` at runtime both resolve against the dotenv file set, never against
the editor's process environment, so those names would not exist wherever the
application actually runs.

Completion now offers only what the project declares in its `.env` files. The
`seen_names` set went with the loop β€” its only read site was the merge's
duplicate check. The `.env` echo path (value plus source file) is untouched.

Eight tests drive the real `textDocument/completion` entry point across all
three env contexts with a real process variable set, including the issue's
empty-prefix repro and the `Ok(None)` response shape.

Fixes: #342

* test: βœ… Cover <server name="…"> in the env-completion leak suite.

`get_phpunit_env_context` parses two PHPUnit spellings, `<env name="` and
`<server name="`, through separate arms of one match β€” different literals,
different offsets (`e + 11` against `s + 14`). The suite drove the first
spelling only, so the second had never reached `completion()` anywhere in the
crate, and the pull request re-quoted AC #5 without its `<server name="…">`
clause and without noting the narrowing.

Two tests close it: the typed prefix and the empty-prefix repro, mirroring the
`<env name="` pair. Both pin the offset arithmetic β€” rewriting `s + 14` to
`s + 11` fails these two tests and nothing else in the crate.

The module doc comment claimed that restoring the deleted `std::env::vars()`
loop turns every test in the file red. It does not. The shadowing test's fixture
`.env` declares the process variable's own name, so the loop's own
`!seen_names.contains(&name)` guard skipped that variable before this fix as
well. The comment now says nine of ten, names the test that stays green, and
names the second mutation that discriminates it. Two neighbouring claims in the
same block are corrected with it: the context list now carries both PHPUnit
spellings, and the "each test asserts a positive control" claim now records its
one deliberate exception.

Verified by running the mutations, not by inspection: restoring the loop fails 9
of 10, the `.env` echo-format mutation fails the tenth, the offset mutation fails
exactly the two new tests.

Watson-Branch: #342

* test: βœ… Assert the .env documentation panel in the env-leak suite.

AC #8 requires the shadowing regression to confirm the .env-declared
variable still completes with its value AND `Source: <file>`. AC #4 fixes
what that names: `detail` is `"<value> (from <file>)"`, `documentation` is
`Source: <file>`. The suite asserted `label` and `detail` and never read
`documentation`, so deleting `.section(format!("Source: {}", source_file))`
from `completion()` left all 3185 tests green.

Sweeps the field enumeration rather than the flagged member: the panel is
asserted whole, so dropping any one of the three `CompletionDoc` builder
calls β€” `.header`, `.summary`, `.section` β€” fails 9 of the module's 10
tests, verified live for each. Test-only change; `main.rs` is untouched.

Watson-Branch: #342

* chore: πŸ” Re-trigger CI on an unchanged tree.

GitHub accepted the push of dbd4792 β€” CodeQL ran on that SHA β€” but never
dispatched the `pull_request`-triggered CI workflow for it. ci.yml has no
paths filter and no concurrency block, and the workflow is active, so the
event was dropped on GitHub's side. Closing and reopening the pull request
did not dispatch it either.

This commit is empty: the tree is byte-identical to dbd4792. It exists only
to raise a fresh synchronize event so the required checks run.

Watson-Branch: #342
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.

1 participant