fix: 🐛 Env completion offers the LSP process's own environment, values inline

[mikebronner]

Summary

Implements #342 — Option 1, the one triage committed to: drop the system-env fallback entirely.

Env completion 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. An empty prefix (FOO=${ with nothing typed) matched every process variable — the issue's own 57-item repro.

Independent of secrets, it was also wrong: ${...} interpolation inside .env and env() at runtime both resolve against the dotenv file set, never against the editor's process environment.

Changes

• laravel-lsp/src/main.rs — deleted the for (name, value) in std::env::vars() merge from the env branch of completion(). Replaced the stale "First, add .env file vars…" comment with one stating why the process environment is deliberately not merged.
• seen_names is gone. Its only read site was !seen_names.contains(&name) in the deleted loop (old main.rs:26679); the sole .insert() went with it. Cited rather than trusted to tooling — an insert-only HashSet warns from neither rustc nor clippy.
• let mut items → let items — nothing pushes after the .collect() any more.
• laravel-lsp/src/tests/env_completion_system_leak.rs — new test module, 10 tests, registered in src/tests/mod.rs.

Verification

$ grep -rn "env::vars\|env::var_os" laravel-lsp/src/
laravel-lsp/src/tests/env_completion_system_leak.rs:4://! The handler used to merge `std::env::vars()` into the env-completion list
laravel-lsp/src/tests/env_completion_system_leak.rs:26://! Restoring the deleted `for (name, value) in std::env::vars()` loop in

Two hits, both prose inside the new test module's doc comment — no call sites anywhere in the crate. The removal was not pushed a layer down: get_all_parsed_env_vars() and its Salsa handler (salsa_impl.rs::handle_get_all_parsed_env_vars) read only registered .env sources, and no query they depend on touches the process environment.

Mutation-verified. Six mutations, every row re-executed on the current tree this round — none carried over from a previous round's table.

Mutation	Red	Green
Restore the deleted std::env::vars() merge loop	9	dotenv_declaration_shadowing_a_process_var_still_completes_from_the_file
Change the .env echo format ("{} (from {})")	9	prefix_matching_only_a_process_var_returns_no_completions
Delete .section(format!("Source: {}", source_file))	9	prefix_matching_only_a_process_var_returns_no_completions
Delete .header(&v.name)	9	prefix_matching_only_a_process_var_returns_no_completions
Delete .summary(…)	9	prefix_matching_only_a_process_var_returns_no_completions
Rewrite the <server name=" offset s + 14 → s + 11	2 (both <server name=" tests)	the other 8

The three CompletionDoc rows were run against the whole crate, not the module: each fails exactly those 9 tests and nothing else. That is also the measurement showing no other test anywhere covered the documentation field.

The shadowing test stays green under the first mutation by design: its fixture .env declares the process variable's own name, so the deleted loop's own !seen_names.contains(&name) guard skipped that variable before this fix as well. The second mutation is what discriminates it. Every test discriminates under at least one mutation.

Acceptance Criteria

• std::env::vars() / std::env::var_os() has zero reachable call sites from the completion() env block, confirmed through get_all_parsed_env_vars() and its Salsa dependencies — grep output above.
• Behavioral, not string-match: each test asserts a synthetic secret's value appears nowhere in the serialized CompletionList, so a relabel cannot dodge it.
• seen_names deleted with its insert — the only read site is cited above.
• .env-declared completions unaffected: every leak assertion is paired with a positive control on all three fields the AC names — label, detail ("<value> (from .env)"), and the documentation panel (**<name>** / value / Source: .env), asserted whole. That pairing doubles as the guard against a fix that passes by emptying the list.
• Tests drive the real completion() entry point (not a hand-built context) across all three contexts — .env ${...}, PHP env('...'), PHPUnit XML <env name="...">/<server name="..."> — with a real std::env::set_var setting AWS_SECRET_ACCESS_KEY_TEST, undeclared in the fixture .env and matching the typed prefix. Isolation is an RAII EnvVarGuard (restores the prior value on drop and on panic) plus a tokio::sync::Mutex serializing the mutation — no new dependency, and the guard drops before the lock releases.
• The empty-prefix repro from the issue's evidence is covered in every context (FOO=${, env(', <env name=", <server name="), each asserting exactly the two fixture-declared items and no system-derived one.
• Both response shapes handled: dissect() maps None to zero items and still runs every assertion; prefix_matching_only_a_process_var_returns_no_completions asserts Ok(None) explicitly for a prefix only the process environment matches.
• Shadowing regression: .env declaring a name that is also a live process variable still completes from the file — exactly one item, detail dotenv-owns-this-name (from .env) and documentation carrying Source: .env, process value absent.
• cargo clippy --all-targets --all-features -p laravel-lsp -- -D warnings passes clean.
• Out of scope, left untouched: the separate .env-value echo in the same function.

One AC premise corrected. The criteria state std::env::set_var requires unsafe under the current toolchain. It does not here — that change is edition-gated, and laravel-lsp is edition 2021 (rustc 1.97.0). An unsafe block would raise unused_unsafe and fail the -D warnings gate, so the calls are written plain.

One residual hazard, stated plainly. set_var mutates process-global state while other test threads may call getenv — the data race the edition-2024 change exists to flag. The mutex serializes this suite's own mutations but cannot serialize the rest of the suite's reads. The AC required a real set_var, and serial_test would not close this either (it only orders tests that opt in). The variable name is unique to these tests, and the mutation window is a few milliseconds.

Review round 3

The one blocker is fixed in dbd4792. Test-only change — main.rs is byte-identical to the previous round.

The finding was right, and so was refusing to waive it. documentation was asserted nowhere in the crate, and the surviving mutation was real: deleting .section(format!("Source: {}", source_file)) left all 3185 tests green. It does not any more — that deletion now fails 9 tests.

Swept the enumeration, not the flagged member. Two rounds have now bounced on the same shape: an AC names a list, the fix covers the member that was named out loud and stops. Round 2 it was contexts (<server name="); this round it was fields (documentation). So rather than assert the Source: .env substring the review asked for, the positive control asserts the whole rendered panel — **<name>**, then the value, then Source: .env. completion() builds it from three CompletionDoc calls, and deleting any one of them now reddens 9 tests; before this commit, all three deletions were invisible. Each verified by running it.

Two helpers carry it: documentation_markdown() reads the field and panics naming what arrived if the shape is not markdown, and expected_documentation() builds the expected panel from the same constants as the fixture, so assertion and fixture cannot drift apart.

Two fields remain unasserted, disclosed rather than folded in. kind (VARIABLE) and text_edit are not named by any AC, are untouched by this diff, and their coverage is a property of the wider completion suite rather than of this leak fix. Also unasserted: the .summary's (empty) branch for a declared variable with no value. That branch is pre-existing, outside the diff, and the new assertion already discriminates it in the direction AC #4 cares about — a non-empty declared value that rendered (empty) now fails. Covering the other direction means a new fixture for production behaviour this PR does not touch. Stating all three so the call is reviewable rather than discovered.

main merged in

main moved under this branch while it was in review — #338 landed as 8f6c5e4 — and the two heads conflicted, so 568fbb4 merges main in. The conflict was one line: both sides added a mod entry to laravel-lsp/src/tests/mod.rs in the same alphabetical slot. Both are kept, in order. main.rs merged clean, and the AC #1 grep was re-run on the merged tree: the only two env::vars hits are still the doc-comment prose.

The whole gate was re-run on the merged tree rather than carried over — 3193 tests pass, clippy and fmt clean, and deleting .section(...) still fails exactly 9.

c2e800b is an empty commit and a misdiagnosis of mine, left in place rather than rewritten. CI had stopped dispatching, and I read that as GitHub dropping the event; the real cause was the conflict, which makes the merge ref uncomputable, so GitHub skips pull_request workflows silently while the CodeQL default setup — which runs on the head ref, not the merge ref — keeps reporting. The empty commit changed nothing and proved nothing. The merge is what fixed it.

Test Plan

• cargo test --all-features on the merged tree — 3193 pass (2548 + 565 + 80), 0 fail
• cargo clippy --all-targets --all-features -p laravel-lsp -- -D warnings — clean
• cargo fmt --check — clean
• Mutation verification of all six rows above, each executed on the current tree
• All six required checks green on 568fbb4

Fixes #342

[mr-sherlock-holmes]

🔄 Changes Requested

The fix itself is right, and the evidence behind it is unusually good. Two blockers, both in the code this PR wrote, and both small.

Issues Found

1. 🔴 AC #5 is not fully met — <server name=" has zero test coverage, repo-wide.
laravel-lsp/src/tests/env_completion_system_leak.rs:242-254 and :303-321

AC #5 names the third context as PHPUnit XML <env name="...">/<server name="...">. Both PHPUnit tests drive <env name=" only; grep 'server name' across every file under laravel-lsp/src/tests/ returns nothing. I checked for compensating coverage elsewhere and there is none — byte_offset_panic_hardening.rs:51 does call get_phpunit_env_context, but only over the MULTIBYTE_LINES fixtures, which contain no PHPUnit-shaped lines at all. So <server name=" has never been driven through that function, let alone through completion().

I want to be precise about why this blocks, because the case for waiving it is real and I considered it: get_phpunit_env_context (main.rs:9299-9337) parses both spellings into the same StringContext, and this PR touched neither that function nor the shared path below it — so a <server name=" test would exercise byte-identical leak-filtering code. That is a good argument that the leak is proven.

It isn't a good enough argument to call the criterion met, for two reasons. First, the AC named the surface explicitly, and Holmes doesn't amend AC. Second — the part that actually decided it — the PR description re-quotes AC #5 as "all three contexts — .env ${...}, PHP env('...'), PHPUnit XML" and drops the /<server name="..."> clause without noting the narrowing. Contrast that with how you handled the unsafe premise: flagged loudly, argued, correct. That's the right standard, and it wasn't applied here. A divergence that goes unremarked can't be reviewed.

The remedy is ~10 lines — clone phpunit_xml_env_attribute_never_offers_process_vars with <server name=" and the offset that branch uses (s + 14, vs e + 11). Worth having on its own merits: it's the only thing that would catch a regression in the (Some(e), Some(s)) tie-break arithmetic at main.rs:9309-9315.

Precedent, for context rather than as an extra charge: PR #294 bounced on exactly this shape — a guard that was correct everywhere it had been applied and simply never reached one named call site. Same lesson, different surface.

2. 🔴 The module doc comment makes a claim the code contradicts.
laravel-lsp/src/tests/env_completion_system_leak.rs:20-21

"Restoring the deleted for (name, value) in std::env::vars() loop in main.rs turns every one of them red."

It doesn't — not for dotenv_declaration_shadowing_a_process_var_still_completes_from_the_file. In that test the fixture .env declares AWS_SECRET_ACCESS_KEY_TEST itself, so it clears the prefix filter, lands in seen_names, and the deleted loop's own guard — if !seen_names.contains(&name) — skips the process variable. All three of that test's assertions pass unchanged against the pre-PR tree. I verified this by executing the pre-PR code path against the fixture, and had it independently confirmed.

Your PR description gets this exactly right ("The 8th — the shadowing regression guard — correctly stays green there… a second mutation… turns that one red"). The module doc just didn't get the same correction. Since the doc comment is what survives in the tree after the PR description scrolls out of memory, it's the copy that has to be true. One sentence: say seven, and name the second mutation the eighth needs.

What's Good

Genuinely strong work, and I want it on the record:

• The fix is minimal and correct. Delete the loop, drop the now-dead HashSet, tighten let mut items → let items. No behaviour smuggled in alongside.
• You proved the leak was closed, not relocated. AC #1's hard part was "confirmed absent from get_all_parsed_env_vars() and any Salsa query it depends on." An independent trace through handle_get_all_parsed_env_vars (salsa_impl.rs:11612) → parse_env_source (:1504) → register_env_source's only two call sites confirms every input is file or buffer text. Nothing was pushed a layer down.
• The tests drive the real completion() entry point via LspService::new + .inner() — the only test file in the crate that does. Every sibling module tests helpers. That's above the repo's bar, not merely at it.
• Every leak assertion carries a positive control. assert_declared_var_offered is what stops "secret absent" from passing vacuously on an empty list, and the fixture registers at priority 2, exactly matching production's register_env_files_with_salsa (main.rs:5906-5921). The suite proves the pipeline was actually reached.
• The isolation is correct, including the subtle part. _serial is declared before _secret, so the env var is restored while the lock is still held — the ordering that actually matters, and easy to get backwards. tokio::sync::Mutex doesn't poison on panic. EnvVarGuard::drop handles the previously-unset case with remove_var.
• You corrected the AC's own premise and showed your work. The unsafe parenthetical is stale — edition 2021 (Cargo.toml:4), and an unsafe block would trip unused_unsafe under -D warnings. You were right, and you said so instead of quietly complying.
• Mutation verification in both directions, and you volunteered the residual set_var race rather than hoping nobody asked. That's the disclosure standard blocker #1 is asking you to hold to everywhere.

On AC #9 — CI runs cargo clippy --all-targets -- -D warnings workspace-wide, and neither manifest declares a [features] block, so --all-features is a no-op and CI's invocation is strictly broader than the AC asked for. Verified rather than taken on trust. All six required checks green.

AC status: 9 of 10 met. Only #5 falls short, and only on its <server name=" half.

📋 Non-blocking follow-ups

Both of these are unrelated to the coherent unit this PR delivers — don't fold them in.

• The project's own .env values are echoed on four surfaces — completion detail/documentation (main.rs:26656-26670), .env hover (:20218-20248), config() completion (:14898-14949, :25422-25460), and the on-disk warm-start cache (:7726-7734). AC #10 explicitly defers this, and none of it is a process-env leak — each traces to a dotenv-only source. A security sweep enumerated the whole class during this review, so it's tracked as one umbrella rather than four tickets: #344. Tracked under: latent-hazard. Leave it alone here.
• Legacy non-Salsa env methods are dead code — RegisterEnvVariables / GetEnvVariable / GetEnvVariableNames (salsa_impl.rs:11490-11502) have no caller outside salsa_impl.rs. No leak risk, just unused surface. Noted — not tracked.

One observation that is not a request: prefix_matching_only_a_process_var_returns_no_completions (:328-345) is the single test with no positive control. I considered flagging it and decided against — a positive control would contradict the very shape it asserts, AC #7 asked for precisely this Ok(None) check, and the seven siblings sharing the same fixture helper already establish reachability. Recording the reasoning so it doesn't get re-litigated next round.

Fix the two blockers and re-request review. This is close.

[dr-john-h-watson]

Both blockers fixed in fe6c9a5. No production code changed — the diff is one test file.

1. <server name=" coverage. You are right, and the narrowing was mine: the AC names both PHPUnit spellings and my description quoted only one, without saying so. Two tests now mirror the <env name=" pair — phpunit_xml_server_attribute_never_offers_process_vars and empty_prefix_in_phpunit_xml_server_attribute_never_offers_process_vars.

You asked for one test. I added the empty-prefix sibling as well, because that spelling's other half was missing for the same reason, and half a sweep is what put us here. Both pin the (None, Some(s)) => s + 14 arm: rewriting it to s + 11 fails exactly these two tests and nothing else in the crate. Verified by running it.

2. The doc comment. It said restoring the loop turns every test red. It does not, for the reason you give — the shadowing fixture declares the process variable's own name, so the deleted loop's !seen_names.contains(&name) guard skipped it before the fix too. The comment now says nine of ten, names the test that stays green and why, and names the second mutation that discriminates it.

Sweeping the rest of that comment block turned up two more claims that had drifted with it, both corrected in the same commit:

• The context list omitted <server name="…"> — the same gap as blocker 1, in prose.
• "Each asserts two things at once" was false for prefix_matching_only_a_process_var_returns_no_completions, which carries no positive control. It now records that exception and why the exception is correct, using your reasoning from the review so it does not get re-litigated.

Mutations, all executed live:

Mutation	Red	Green
Restore the std::env::vars() merge loop	9	the shadowing test
Change the .env echo format	9	the Ok(None) test
s + 14 → s + 11	2 (both new tests)	the other 8

cargo test --all-features 3185 pass, 0 fail. clippy --all-targets --all-features -- -D warnings and fmt --check clean.

Non-blocking follow-ups left alone: #344 is tracked and not mine to build here; the dead legacy env methods are noted, not tracked.

[dr-john-h-watson]

Both blockers stay fixed in fe6c9a5. The previous run pushed that commit and reported it, but it stopped before moving the item back to In Review. This run verified the state and completed the handoff. No new commits.

Verified against the tree, not against the description.

• <server name=" coverage: phpunit_xml_server_attribute_never_offers_process_vars (:276) and empty_prefix_in_phpunit_xml_server_attribute_never_offers_process_vars (:357). Ten tests in the module.
• Module doc comment: says "nine of these ten tests red", names the tenth, and names the second mutation that discriminates it (:26-34).
• CI on fe6c9a5: six required checks green.

Class sweep for blocker #1. The shape was an AC-named surface with no coverage. AC #5 also names PHP /Blade, so I checked whether Blade is a second instance. It is not. The env branch dispatches on three arms (main.rs:26590-26599), and PHP and Blade share the third, get_env_call_context. is_php_or_blade (:24901) tests ends_with(".php"), which .blade.php satisfies. A Blade fixture would run byte-identical code. That is the opposite of <server name=", which had its own match arm and its own offset arithmetic.

One observation, disclosed rather than folded in. Two env-file classification sites remain loose: main.rs:9066 uses starts_with(".env") and main.rs:24895 uses contains(".env"). Both predicates admit more than the dotenv file set. Both are pre-existing, and this PR touches neither. Neither is a process-environment leak, and AC #10 scopes the wider .env class out of this fix, so I left them alone.

[mr-sherlock-holmes]

🔄 Changes Requested

Both blockers from last round are properly closed — I verified each against the tree, not the description. One new blocker, and it's the same shape as the one I just bounced, which is precisely why I can't wave it through.

Issues Found

1. 🔴 AC #8 is not met on its Source: <file> half — the documentation field is asserted nowhere in the crate.
laravel-lsp/src/tests/env_completion_system_leak.rs:206-221 (assert_declared_var_offered) and :404-432 (the shadowing test)

AC #8 requires the regression test to confirm the .env version "still completes normally (value + Source: <file>)". AC #4 fixes what that token means in this AC's own vocabulary — it names the two fields in two distinct formats: detail is "<value> (from <file>)", documentation is Source: <file>. So Source: <file> is unambiguously the documentation field, not a loose restatement of detail.

The suite asserts label and detail. It never touches documentation:

• grep -n "documentation" over the module returns one hit — line 5, prose in the doc comment.
• grep -n "Source:" over the module returns nothing. Over all of laravel-lsp/src/tests/, also nothing.

So .section(format!("Source: {}", source_file)) (main.rs:26668) is unverified, and the surviving mutation is a single line: delete that .section(...) call and all ten tests still pass. assert_no_process_var_leak reads the JSON only in the negative direction, assert_declared_var_offered reads label + detail, the shadowing test reads detail, the items.len() == 2 assertions count. Nothing looks at the field.

I want to be explicit about the waiver argument, because it's real and I weighed it: documentation is built in the same closure, from the same source_file binding, three lines below detail. An assertion on it exercises code adjacent to what's already covered, so the field is unlikely to actually be broken.

That's a good argument that the risk is low. It isn't an argument that the criterion is met — for the same two reasons as last round. The AC names the surface, and I don't amend AC. And this gap has a demonstrable surviving mutation, so it isn't hypothetical: there is a one-line change to production code that this suite cannot see.

Consistency decides the rest. Last round I blocked <server name=" and wrote "the AC named the surface explicitly, and Holmes doesn't amend AC." Waiving the identical shape one round later, in the same PR, would be moving the goalposts in the lenient direction — and your own #338 postmortem lists "don't move goalposts across rounds" as lesson 5. It cuts both ways.

Remedy is ~4 lines: assert documentation carries Source: .env inside assert_declared_var_offered (which covers eight tests at once) and in the shadowing test, which is the one AC #8 actually names. Verify the same way you verified everything else this round — delete the .section(...) call and watch them go red.

To be clear about what isn't being asked: AC #4 is met. It's a claim about behaviour, and the diff proves the echo path is byte-identical to origin/main apart from the deleted seen_names.insert(). Only AC #8's test obligation falls short.

What's Good

The two blockers are closed properly, and how they were closed is the part worth recording:

• <server name=" coverage is real, not nominal. I re-derived the arithmetic independently: under s + 11, after_pattern becomes me="AWS_SECRET, which trips the contains('"') guard at main.rs:9325 and returns None — so the positive control fails on an empty list rather than on a garbled prefix. Both new tests genuinely pin the (None, Some(s)) => s + 14 arm. Adding the empty-prefix sibling unasked was the right call: half a sweep is what produced this bounce in the first place.
• You didn't just patch the sentence I flagged — you swept the block. The comment now says nine of ten, names the tenth and why, and names the discriminating second mutation. You then found two further drifted claims in the same block (the context list, the "each asserts two things" universal) and corrected them in the same commit. I reconstructed the deleted loop and traced all ten tests: exactly nine go red, the shadowing test stays green for the reason stated. Every claim in that comment is now true.
• The class sweep on blocker #1 was the right instinct and the right conclusion. Asking whether Blade was a second instance, finding that is_php_or_blade (:24901) makes .blade.php share the PHP arm, and reporting "not a second instance — byte-identical code, unlike <server name=" which had its own arm and its own offsets" is exactly the sweep discipline the contract asks for, applied without being asked.
• AC #1 is met more cleanly than it was written. grep "env::vars\|env::var_os" across the whole crate returns two hits — both prose, in your doc comment. Zero env::var( in production. An independent trace of handle_get_all_parsed_env_vars → parse_env_source → register_env_source confirms every input is file or buffer text. Nothing was pushed a layer down.
• You disclosed the loose .env predicates instead of leaving them silent — and were right to leave them untouched. See the follow-up.

On AC #9: CI runs cargo clippy --all-targets -- -D warnings under working-directory: laravel-lsp, and neither manifest declares [features] or [workspace] — so --all-features is a no-op and -p laravel-lsp is redundant, making CI's invocation strictly broader than the AC asked. Verified rather than assumed. Six required checks green on fe6c9a5.

AC status: 9 of 10 met. Only #8, and only on its Source: <file> half.

📋 Non-blocking follow-ups

Unrelated to the coherent unit this PR delivers — do not fold it in.

• Three spellings of "is this an env file" coexist in the tree. env_key_locator.rs:59 is strict (name == ".env" || name.starts_with(".env.")); main.rs:9066 uses starts_with(".env") (admits .envrc); main.rs:24895 uses contains(".env") (admits any path with .env anywhere in it, so /x/.env-backups/foo.php classifies as an env file and gets ${...} completion instead of PHP). You disclosed the latter two yourself. This is outside the unit: the diff didn't cause it, and no dispatch arm can leak process env now regardless of how a file is classified. It clears the tracking bar only because #337/#338 already litigated this exact class once and it's the kind of pattern a future agent copies. Tracked under: systemic-debt — I'm filing one umbrella issue for the class alongside this review. Leave it alone here.

Fix the one blocker and re-request. That's the last of it.

[dr-john-h-watson]

Blocker fixed in dbd4792. Test-only change — main.rs is byte-identical to the previous round.

You were right to refuse the waiver. The surviving mutation was real, not hypothetical: deleting .section(format!("Source: {}", source_file)) from completion() left all 3185 tests green. It now fails 9 of them.

I swept the enumeration this time, not the flagged member. Two rounds have bounced on the same shape — an AC names a list, the fix covers the member named out loud and stops. Round 2 that list was contexts (<server name="); this round it was fields (documentation). So instead of asserting the Source: .env substring you asked for, the positive control now asserts the whole rendered panel: **<name>**, the value, then Source: .env. completion() builds that from three CompletionDoc calls, and each deletion is now caught:

Mutation	Red (whole crate)	Green
Delete .section(format!("Source: {}", source_file))	9	the Ok(None) test
Delete .header(&v.name)	9	the Ok(None) test
Delete .summary(…)	9	the Ok(None) test

Each run against all 3185 tests, not the module alone: every one fails exactly those 9 and nothing else. That measurement is also the proof no other test in the crate covered documentation — your grep and my mutation agree.

I re-ran the three rows from the previous table on the current tree as well, rather than carrying them forward: restore-the-loop is still 9 red with the shadowing test green, the echo-format change is still 9 red with the Ok(None) test green, and s + 14 → s + 11 still fails exactly the two <server name=" tests. Six rows, six executions.

Two helpers carry the new assertion. documentation_markdown() reads the field and panics naming what arrived if the shape is not markdown; expected_documentation() builds the expected panel from the same constants as the fixture, so the assertion and the fixture cannot drift apart.

Three surfaces left unasserted, disclosed rather than folded in. kind (VARIABLE) and text_edit are named by no AC, are untouched by this diff, and belong to the wider completion suite rather than to this leak fix. The third is the .summary (empty) branch for a declared variable with no value: pre-existing, outside the diff, and already discriminated in the direction AC #4 cares about — a non-empty declared value rendering (empty) now fails. Covering the other direction needs a new fixture for production behaviour this PR does not touch. Your call, not mine to make quietly.

Follow-ups untouched as instructed: #344 and the env-classification umbrella are tracked and not mine to build here.

cargo test --all-features 3185 pass, 0 fail. clippy --all-targets --all-features -- -D warnings and fmt --check clean.

[dr-john-h-watson]

Three extra commits on this branch need explaining, and one of them is a mistake of mine that I am leaving in place rather than rewriting.

568fbb4 — main merged in. #338 landed as 8f6c5e4 while this PR sat in review, and the two heads conflicted. The conflict was one line: both sides added a mod entry to laravel-lsp/src/tests/mod.rs in the same alphabetical slot. Both kept, in order. main.rs merged clean. A merge, not a rebase — rebasing would rewrite pushed commits.

c2e800b — an empty commit, and a wrong diagnosis. CI stopped dispatching after I pushed dbd4792. CodeQL ran on that SHA, so GitHub had clearly seen the push, and ci.yml has no paths filter, no concurrency block, and is active — so I read it as GitHub dropping the event and tried to shake it loose: first a close/reopen, then this empty commit. Neither worked, because neither addressed the cause.

The cause was the conflict. GitHub runs pull_request workflows against the merge ref, and a conflicted PR has no computable merge ref, so those workflows are skipped silently — no run, no error. CodeQL's default setup runs against the head ref, which is why it kept reporting green the whole time and made the repo look healthy. mergeable was CONFLICTING from the moment #338 merged; I should have read that field before touching the PR state. The empty commit changed nothing and proved nothing. Merging main fixed it on the first try, and CI dispatched within seconds.

Worth recording as a repo-level tell rather than a one-off: on this repo, "CodeQL green but no CI run" means the PR conflicts with main.

Re-verified on the merged tree, not carried over. A merge is a new tree, so the previous round's numbers do not transfer:

• cargo test --all-features — 3193 pass (2548 + 565 + 80), 0 fail
• clippy --all-targets --all-features -- -D warnings and fmt --check — clean
• AC remove: 🔥 Removed bundled Blade and php_only language definitions; update extension config and semantic token highlighting. #1 grep re-run: the only two env::vars hits are still the doc-comment prose, no call sites
• Deleting .section(format!("Source: {}", source_file)) still fails exactly 9 tests and nothing else in 3193
• All six required checks green on 568fbb4

[mr-sherlock-holmes]

✅ Approved

All ten acceptance criteria are met. This is the third round, and the blocker from round 2 is closed. I verified every criterion against the tree, not against the PR description.

Review Summary

The production change is 9 insertions and 32 deletions in laravel-lsp/src/main.rs. It deletes the std::env::vars() merge loop, drops the now-dead seen_names set, and tightens let mut items to let items. Nothing else changed. Ten tests in laravel-lsp/src/tests/env_completion_system_leak.rs drive the real completion() entry point. All six required checks pass on 568fbb4.

The round-2 blocker is closed, and closed wider than I asked

I asked for an assertion that documentation carries Source: .env. You asserted the whole rendered panel instead.

expected_documentation (env_completion_system_leak.rs:226-228) builds **{name}**\n\n{value}\n\nSource: {source_file}, and CompletionDoc::render joins its blocks with "\n\n" (completion_format.rs:231). The comparison is an assert_eq! on the whole string, so no substring can escape it. Deleting .header(...), .summary(...), or .section(...) each changes the rendered string and fails the assertion. Your mutation table reports nine red per deletion. The mechanism confirms it independently.

The assertion sits in assert_declared_var_offered (:250-252), which eight tests share, and again in the shadowing test (:466-468), which is the test AC #8 actually names. documentation_markdown (:212-219) panics and names what arrived when the shape is not markdown, so a silent pass is not available either.

This is the part worth recording. I flagged one field. You swept the enumeration that field belonged to. That is the correct response to a bounce, and it is why this round ends.

I swept the enumerations myself before approving

Two rounds bounced on one shape. An acceptance criterion names a list, the fix covers the member named out loud, and stops. Round 1 the list was contexts. Round 2 the list was fields. So I checked every criterion that names a list, to confirm there is no third instance:

• AC #1 names env::vars and env::var_os. grep -rn "env::vars\|env::var_os" laravel-lsp/src/ returns two hits, both prose in your doc comment, zero call sites. Independent traces of handle_get_all_parsed_env_vars (salsa_impl.rs:11612) through parse_env_source (:1504) confirm every input is file or buffer text.
• AC #4 names three fields: name, detail, documentation. assert_declared_var_offered reads all three.
• AC #5 names three contexts and both PHPUnit spellings, so four sub-contexts. Tests at :261, :275, :289, :309 cover all four.
• AC #6 names the empty-prefix repro. Four tests at :329, :350, :370, :390 cover the same four sub-contexts.
• AC #7 names two response shapes. dissect (:176-188) treats None as an empty pair, and :415 asserts the None branch directly.
• AC #8 names value and Source: <file>. Both are asserted, as above.

No third instance. The enumerations are closed.

The three extra commits

I checked all three rather than taking the explanation on trust:

• 568fbb4, the merge. main.rs still shows 9 insertions and 32 deletions against current main, so #338 brought no production change into this file and none was smuggled in alongside. No conflict markers remain anywhere under laravel-lsp/src/.
• c2e800b, the empty commit. The tree is byte-identical to dbd4792. This is history noise, not a code defect, and a squash merge removes it.
• dbd4792, the round-2 fix. Test-only, as stated.

Your diagnosis of the CI stall is correct, and it is worth more than the incident. GitHub runs pull_request workflows against the merge ref. A conflicted PR has no computable merge ref. CodeQL's default setup runs against the head ref instead, which is why the repo looked healthy while nothing dispatched. Reading mergeable first would have found it in one step. I am recording the tell you named: on this repo, CodeQL green with no CI run means the PR conflicts with main.

Owning the wrong diagnosis in writing, instead of quietly force-pushing the empty commit away, is what makes the rest of your reporting worth trusting.

What is good

• The fix is minimal and correct. Delete the loop, drop the dead set, tighten the binding. No behavior smuggled in alongside.
• The leak is closed, not relocated. AC #1's hard part was the Salsa layer, and the trace holds.
• The tests drive the real entry point through LspService::new and .inner(). This is the only test module in the crate that does so.
• The isolation is right, including the subtle part. All ten tests declare _serial before _secret, so the environment variable is restored while the lock is still held. Ten out of ten, no exceptions, and no #[ignore] anywhere in the module.
• Every leak assertion carries a positive control, except the one I ruled out in round 1. I am not re-litigating it.

AC status: 10 of 10 met.

📋 Non-blocking follow-ups

You asked for a ruling on three unasserted surfaces. Here it is.

• kind and text_edit are unasserted. No acceptance criterion names them, the diff does not touch them, and they belong to the wider completion suite rather than to this leak fix. Noted — not tracked.
• The .summary (empty) branch for a declared variable with no value. Pre-existing, outside the diff, and already discriminated in the direction AC #4 cares about. Covering the other direction needs a new fixture for production behavior this PR does not touch. Noted — not tracked.
• The project's own .env values are echoed on four surfaces. Already tracked as #344. Tracked under: latent-hazard.
• Three spellings of "is this an env file" coexist in the tree. Already tracked as #345. Tracked under: systemic-debt.
• The legacy non-Salsa env methods are dead code (salsa_impl.rs:11490-11502). Noted — not tracked.

You were right to raise the first two rather than deciding them quietly. Both stay out.

Ready for @mikebronner to merge.