fix: mask sensitive .env values in LSP output

[mikebronner]

Summary

Implements #344 — one policy, one home for it, every surface that echoes a .env value. The policy has two gates: the AC's name predicate, and a value-shape gate added in review round 2 for the credential a name cannot see.

Changes

• Gate 1 — the name. completion_display::is_sensitive_env_name: case-insensitive, splits on _, matches a whole segment against KEY, SECRET, PASSWORD, TOKEN, CREDENTIAL, PRIVATE, AUTH, PWD. Segment matching, never substring, so AUTHOR_NAME and TOKENIZE_INPUT keep their values. REDACTED_ENV_VALUE is the one string every client-rendered surface prints.
• Gate 2 — the value's shape (round 3). DATABASE_URL splits to DATABASE / URL and matches no segment, yet stock Laravel's config/database.php reads 'url' => env('DATABASE_URL') and fills it with mysql://user:hunter2@host/db. completion_display::mask_url_credentials masks the password and keeps the host, port and database — the whole reason anyone reads that value. It is database.rs's own mask_url_password, moved next to the name gate so the two halves of one policy live in one file and cannot become two spellings. Returns Cow, borrowing on every fail-open path.
• All five consumers apply both gates: env completion, .env hover, config('…') completion, the warm-start disk cache, and mask_env_value_for_log — whose unmatched arm is now the masker rather than the raw value.
• Surface 1 — env completion: a matched name drops detail to (from .env) and the panel to the redaction string, before the existing empty-value branch, so DB_PASSWORD= renders redacted and never (empty). An unmatched name renders its value through gate 2.
• Surface 2 — .env hover: a matched name replaces the code block with the redaction note; an unmatched name keeps its code block, masked. The source link survives; the commented-out and not-found paths are untouched.
• Surface 3 — config('…') completion: resolve_env_value applies both gates before the value reaches ConfigKeyCompletion (the AC's "or redact before it's stored there" option — no signature churn, and the two render sites cannot diverge). Only the dotenv-sourced path is touched: a literal default is already visible in the PHP file, ${VAR} has no value, and a plain config literal is never checked — even when the config key would match.
• Surface 4 — warm-start cache: a matched name is stored with an empty value, key retained, with no is_commented filter so # MAIL_PASSWORD=… is redacted too; every other value is written through gate 2, so no credential reaches the file under either gate. CACHE_VERSION 5 → 6, with a compile-time floor (const _: () = assert!(CACHE_VERSION >= 6)) so the bump cannot be walked back and re-admit a plaintext cache. No further bump for gate 2: v6 has never been released.
• Surface 5 — the server log: database.rs logged the value resolved from .env at info!, which fires under the default EnvFilter("info,salsa=warn") into the stderr panel Zed shows and docs/troubleshooting.md asks users to paste into bug reports; resolve_env logged it again at debug!. Both route through mask_env_value_for_log — (set) for a matched name, the masked URL for a matched shape, the value in full for DB_HOST.
• The unreachable (bool) env(...) branch is deleted. env_pattern is unanchored, so it matches the env('X') substring of a (bool) env('X') cast and returns before bool_env_pattern is consulted — the second arm could never run. Two arms that cannot both execute are not redundancy; they are a place for a fix to land in one and not the other, plus a doc comment and a test narrative claiming a safety property the code could not have. The cast spelling still resolves and still redacts, through the one arm, and keeps its fixture.
• fix: 🐛 Env completion offers the LSP process's own environment, values inline #342 tests moved with the policy: their declared control is renamed AWS_SECRET_DECLARED → AWS_SECRETARIAT_REGION (still shares the typed prefix, no longer matches the pattern), and the shadowing test asserts the redacted rendering its secret-shaped name must produce.

The class sweep behind gate 2

The class is "a .env value reaching a display surface with a credential still in it." Every consumer of the parsed-dotenv map was enumerated, not just the AC's four:

Site	Reads	Disposition
completion() env branch, hover_for_env, resolve_env_value, the cache writer	the value	both gates
mask_env_value_for_log (info! + debug! in database.rs)	the value	both gates
create_env_location_from_salsa (main.rs:16173)	name and position only	nothing to leak
the env() diagnostic (main.rs:18010)	existence only	nothing to leak

The connection summary's neighbouring lines are still deliberately untouched: password: masks unconditionally by field, url: routes through the same masker, and driver: / host: / database: / username: are the connection metadata Laravel's own config:show prints. using default: … echoes the literal in the PHP file being edited — surface 3's exemption.

One finding worth your attention

The warm-start cache has no display consumer. register_cached_env_vars writes the Salsa actor's env_variables map; get_env_variable/get_env_variable_names have no caller outside salsa_impl, and all four client surfaces read get_all_parsed_env_vars/get_parsed_env_var, which walk registered .env sources only. So surface 4's redaction is about the plaintext on disk, not about what a warm start renders — the AC's "shows the name plus the redaction string immediately on warm start" describes a capability the code does not have today. Rather than test code that never runs, the cache-parity claim is pinned where it is observable: a matched name with an empty value (exactly what a cache read yields) renders redacted, not (empty).

Acceptance Criteria

• Policy decided: Option 2, one shared predicate every surface calls; a cross-surface test drives two matched categories (DB_PASSWORD, API_TOKEN) through all four
• The predicate exactly as specified, with every positive and substring-but-not-segment negative fixture pinned. Unchanged in round 3 — gate 2 sits beside it and does not amend it
• Surface 1 — env completion, redaction before the empty-value check, non-matching variables byte-for-byte unchanged
• Surface 2 — hover, no part of the rendered markdown carries the value
• Surface 3 — the literal-default, placeholder and plain-literal exemptions each tested. On "both regex branches": the second branch was unreachable and is now deleted, so the criterion's concern — a fix landing in one branch and not the other — is closed structurally rather than by testing a path that cannot execute. The (bool) spelling is still driven by a fixture through the surviving branch
• Surface 4 — name kept with an empty value, is_commented irrelevant, CACHE_VERSION bumped
• Regression guard — an unmatched variable is unaffected on all four surfaces, across a cache round trip and across a CACHE_VERSION-forced rescan (now one test, planting both variables)
• Tests drive the real entry points (completion(), hover_for_env, get_all_config_keys + the config('…') completion response, a real CacheManager save/load), assert over the whole serialized response, and assert the deserialized cache struct rather than its bytes
• cargo clippy --all-targets --all-features -p laravel-lsp -- -D warnings and cargo test --all-features clean

Test Plan

• Full suite green: 2556 lib + 577 bin + 80 integration
• Clippy clean with -D warnings, cargo fmt clean
• Gate 2 is mutation-verified at every site, nine mutations, every one red with the named test actually running: revert the gate in env completion, in hover, in resolve_env_value, in the cache writer, and in mask_env_value_for_log (twice — the info! test and the debug! test); delete the CACHE_VERSION check; make the masker echo the password instead of ***; make its fail-open path reallocate instead of borrowing. A tenth confirms the surviving env() arm is load-bearing for both spellings: dropping env_display_value from it reddens the config test
• The DATABASE_URL fixture crosses all five surfaces. The completion leak sweep now includes its password, so every existing surface test asserts against it over the whole serialized response, and each surface additionally asserts the masked form is present — a test that merely dropped the value would otherwise pass
• Two weak cache assertions replaced. !has_cached_data() inspected only the vendor/app/config sections and would have passed with the version check deleted — dropped; get_env_vars().is_none() is the assertion that discriminates. The pre-bump fixture now plants an ordinary variable beside the secret, asserts the whole file is dropped, then drives the real rescan and asserts the ordinary value returns unchanged while the secret comes back empty
• Log tests drive the real entry points — parse_database_config, parse_env_setting, resolve_env — and assert the plaintext is absent, the masked line is present (pinned to the line that owns the value, not a bare (set) search the pre-existing password: (set) summary would satisfy), and an ordinary DB_HOST value still logs in full
• Log capture is deliberate about tracing: the subscriber is installed once, globally, because a scoped with_default lets an ordinary test reach resolve_env first with no subscriber, cache Interest::never() for that callsite, and silently empty every later capture. That is what turned the first push red on macOS only (e18ce7a); isolation lives in a thread-local buffer, so a non-capturing test drops its output exactly as before
• Doc sweep on every claim this round falsified: the bool_env_pattern comment and the test narrative that named "two branches", mask_url_password's three references, the CACHE_VERSION v6 note and CachedEnvVars's doc (both said "by name" only), and the module docs in completion_display.rs and tests/env_value_redaction.rs. Two invariants asserted in new prose were grepped before being written: env_display_value has exactly one caller, and resolve_env_value is the only route from the dotenv map into ConfigKeyCompletion

Fixes #344

[mr-sherlock-holmes]

🔄 Changes Requested

All nine acceptance criteria are met — I verified each one, and the work behind them is genuinely good. But the review turned up a fifth surface that echoes .env values in plaintext, on the default configuration, and it is the same disclosure class this issue exists to close. A hard security defect blocks regardless of where it lives (§4e), so it has to be shut before this merges.

Issues Found

🔴 A .env-sourced DB password is written to stderr at the default log level

laravel-lsp/src/database.rs:1258

if let Some(env_value) = self.resolve_env(&env_var) {
    info!("🗄️      → resolved from .env: {}", env_value);

Reached from parse_env_setting(&block, "password", "") at database.rs:1095, on the mainline parse_database_config() path — routine for any Laravel project with a config/database.php.

Why this is live, not theoretical: main.rs:26839 sets EnvFilter::new("info,salsa=warn") when RUST_LOG is unset, writing to std::io::stderr (main.rs:26843). info! therefore fires out of the box, into the stream Zed surfaces in a visible log panel. That is precisely the screen-share/recording exposure this issue's own text names as the threat model.

What makes it unambiguous: fifteen lines later, the same function already refuses to do this —

info!("🗄️    password: {}", if password.is_empty() { "(empty)" } else { "(set)" });   // :1110-1117
info!("🗄️    url: {}", mask_url_password(u));                                          // :1121

The masking convention for this exact value already exists in this exact function. Line 1258 simply bypasses it and prints the secret in the clear first.

Precedent — this is the second time in this repo. PR #294 was the identical shape: translation_lookup.rs had a path_within_root guard on one sibling call site that never reached the others. Same lesson landing twice: a guard that exists somewhere in the file is not a guard on the file.

🔴 Same class, second site — close both together

laravel-lsp/src/database.rs:1423

debug!("🗄️  resolve_env({}): {:?}", key, result);

Gated behind debug! rather than info!, so it is dormant under the default filter — but it is the same helper (resolve_env, called for password at :1095) and the same plaintext disclosure, one RUST_LOG=debug away. RUST_LOG=debug is an ordinary troubleshooting step, and troubleshooting output is exactly what gets pasted into a bug report.

I swept the class rather than handing you one site at a time. grep for every resolve_env / read_env_value caller and every log statement carrying an env value across database.rs and config.rs returns exactly these two sites — the class is bounded and closeable in one pass. Please fix both, and mirror the convention already at :1110-1117 (log (set)/(empty), or route through the existing mask_url_password-style masking) rather than introducing a third spelling.

A regression test that asserts the captured log output for a .env-sourced password contains no plaintext would pin this the way the four LSP surfaces are already pinned.

What's Good

This is strong work, and I want to be specific about why:

• One predicate, genuinely shared. is_sensitive_env_name at completion_display.rs:63-68 matches the AC's spec verbatim, and all four surfaces route through it — no per-site reimplementation. The env_display_value helper collapsing both env() regex branches means they structurally cannot diverge, which is better than testing that they currently don't.
• The predicate's fixtures are fully pinned — all five positives and all seven substring-but-not-segment negatives (PASSKEYBOARD_LAYOUT, AUTHOR_NAME, SECRETARY_ID, …) at completion_display/tests.rs:242-297.
• The tests are honest. They drive real entry points (completion(), hover_for_env, get_all_config_keys, a genuine CacheManager save/load), the leak assertion serializes the whole CompletionResponse — closing the insertText/label/sortText escape hatch — and the cache assertion reads the deserialized struct, not the bytes, closing the reversible-encoding hatch. a_pre_bump_cache_holding_plaintext_is_rejected even derives the version field from the real written JSON instead of hardcoding it.
• The modified #342 tests were not weakened. I scrutinised the -16 lines specifically. Renaming AWS_SECRET_DECLARED → AWS_SECRETARIAT_REGION was necessary — the old name's SECRET segment would now be legitimately redacted, destroying that test's original purpose — and the rename preserves the typed-prefix property the sibling tests depend on. Correct call, well commented.
• CACHE_VERSION 5 → 6 with a compile-time floor (const _: () = assert!(CACHE_VERSION >= 6)) so the bump cannot be silently walked back. That is a nice touch the AC did not ask for.
• You disclosed the two awkward findings yourself rather than letting them pass quietly. Both check out exactly as you described them — I verified each independently. That candour is worth more to this review than a clean-looking PR body.

📋 Non-blocking follow-ups

Both are unrelated to the coherent unit; neither clears the tracking gate, so both are noted, not tracked.

• The predicate misses keyword-concatenated names — APIKEY, SECRETKEY, APITOKEN all return false, since split('_') never yields a segment equal to KEY/SECRET/TOKEN. completion_display.rs:64. Disposition: Noted — not tracked. This is not a defect against the contract: the AC pins the predicate exactly, segment-matching was chosen deliberately to avoid over-redacting AUTHOR_NAME/TOKENIZE_INPUT, and I do not amend acceptance criteria. Flagging it so @mikebronner can decide whether the policy should widen later — a decision, not a bug.
• Surface 4's stated rationale is moot in the current tree. register_cached_env_vars writes the Salsa env_variables map (salsa_impl.rs:9103-9117), but all four display surfaces read get_all_parsed_env_vars/get_parsed_env_var, which walk live-parsed salsa_env_files only (salsa_impl.rs:11581-11640); the legacy getters have no caller outside salsa_impl.rs. So the AC's "surfaces 1-3 can still show the name plus the redaction string immediately on warm start" describes a capability that does not exist. Disposition: Noted — not tracked. Pre-existing, untouched by this diff, and the requirement — never write plaintext to disk — is correctly implemented regardless. Your decision to pin the claim where it is observable, instead of writing a test asserting about code that never runs, was the right one.

Please close the two database.rs sites and re-request review. Everything else here is ready.

[dr-john-h-watson]

Both blockers are fixed in c471711. No unrelated cosmetics were touched; the two noted follow-ups are yours to track or drop.

The two database.rs sites. parse_env_setting's info! and resolve_env's debug! now route through one helper, mask_env_value_for_log, which consults completion_display::is_sensitive_env_name — the same predicate the other four surfaces use. A matched name prints (set), the spelling this file already used fifteen lines below the leak, so there is no third spelling. An unmatched name logs its value unchanged: DB_HOST and DB_DATABASE are why those lines exist.

The class, swept independently. I greped every read_env_value / resolve_env caller and every log macro in the crate (single-line and multi-line, src/ and the test tree) for a .env-sourced value. Your two sites are the class. Three neighbours look similar and are deliberately untouched, with the reasoning now in the PR body: the connection summary masks by field (password unconditionally, URL through mask_url_password), which is stronger than the name heuristic for the one field that is a secret by construction; the using default: line echoes a literal from the PHP file being edited; and the None branch carries no value.

Tests. Three, driving parse_database_config, parse_env_setting and resolve_env, asserting the plaintext is absent, the masked line is present, and an ordinary value still logs in full. The (set) assertions are pinned to the line that owns the value — a bare (set) search passes on the pre-existing password: (set) summary and proves nothing. Two matched keyword categories cross the gate, and one test feeds a secret-named variable through an innocuous config key to prove the predicate reads the env var's name. Five mutations, every one red.

One thing worth your attention. The first push was green locally, on ubuntu and on windows, and red on macOS. The cause was the test capture, not the fix: tracing caches a callsite's Interest on first use, so an ordinary test reaching resolve_env with no subscriber installed cached "never" for that macro and emptied my capture. e18ce7a installs one global subscriber and moves isolation into a thread-local buffer. Worth knowing before anyone writes the next log-assertion test in this repo.

Doc sweep. The "four surfaces" enumerations in completion_display.rs and tests/env_value_redaction.rs now name the fifth and say why the log prints (set) rather than REDACTED_ENV_VALUE. CI green on all three runners.

[mr-sherlock-holmes]

🔄 Changes Requested

Both blockers from the last round are genuinely closed — c471711 routes both log sites through mask_env_value_for_log, which delegates to the same is_sensitive_env_name rather than inventing a third spelling, and its doc comment's "no (empty) arm" claim checks out (config.rs:204 filters empties to None before they can reach it). All nine acceptance criteria are met. CI is green on all three runners.

Two things still block, and the first is the same lesson landing a third time.

Issues Found

🔴 A URL-shaped .env value bypasses the name predicate — plaintext DB credential at the default log level

laravel-lsp/src/database.rs:1283 (the line this PR modified)

The predicate is name-based, exactly as AC #2 specifies. But Laravel's stock config/database.php ships 'url' => env('DATABASE_URL') (and env('REDIS_URL') for redis), and those values are mysql://user:hunter2@host/db — the password is inside the value, and DATABASE_URL splits to DATABASE / URL, matching none of the eight segments.

I traced the path rather than assuming it: parse_optional_setting(&block, "url") at :1137 → parse_env_setting(block, key, "") at :1260 → your new info! at :1283. mask_env_value_for_log("DATABASE_URL", …) returns the value untouched, and main.rs:26839 sets EnvFilter::new("info,salsa=warn") when RUST_LOG is unset — so this fires out of the box, into Zed's visible log panel. Same screen-share exposure #344 exists to close.

What makes this unambiguous: twenty-six lines later, at database.rs:1147, the same value is printed correctly —

info!("🗄️    url: {}", mask_url_password(u));   // :1147

mask_url_password already exists at :482, already handles exactly this shape, and is already applied to this exact setting. Line :1283 prints it in the clear first.

This is the third round of one pattern. PR #294: path_within_root guarded one sibling call site and not the others. Last round on this PR: masking existed at :1110-1117 and not at :1258. Now: mask_url_password exists at :1147 and not at :1283. A guard that exists somewhere in the file is not a guard on the file.

Scope — please fix the class, not the line. The same gap is on all four AC surfaces, because they share the predicate: env completion (main.rs:26719), hover (:20283), config('…') (:14986), and the disk cache (:7741) each render or persist a DATABASE_URL in full. The cache one is worst — plaintext credentials written to a long-lived file on disk.

I am not asking you to amend AC #2's predicate; it's Mike's contract and it stays exactly as written. Add a value-shape gate alongside the name gate — route a value through mask_url_password (or the equivalent) when it parses as a URL carrying credentials, independent of whether its name matched. Name-based and shape-based redaction are complementary, and the AC caps neither.

A test with a DATABASE_URL=mysql://user:hunter2@host/db fixture, asserting hunter2 is absent from all four serialized surfaces and from captured log output, pins it the way the name-based cases are already pinned.

🔴 The (bool) env(...) branch is unreachable — the new doc comment and test narrative both claim otherwise

laravel-lsp/src/main.rs:14956-14969 (branch), :14979-14984 (the doc comment this PR added)

env_pattern at :14934 is unanchored, so it matches the env('API_TOKEN') substring inside (bool) env('API_TOKEN') and returns at :14941 before bool_env_pattern is ever consulted. The second branch cannot execute.

The redaction behaviour is still correct — both branches call env_display_value, so nothing leaks. What's wrong is the claim written on top of it. Your new doc comment states "Both env() regex branches above route through this — a fix applied to only one of them leaves the other echoing credentials," and env_value_redaction.rs:327-336 describes its fixture as exercising the two spellings as separate branches. Deleting the env_display_value call from the bool_env_pattern block would fail no test. AC #5's "both regex branches" coverage therefore isn't real — it reads as satisfied while only one path is driven.

Two lenses found this independently. Either make the second branch reachable (anchor env_pattern, or test bool_env_pattern directly), or delete the dead branch and correct the comment and the test's doc to say so. Don't leave a comment asserting a safety property the code can't have.

🔴 Two in-PR test assertions weaker than they read

• tests/env_value_redaction.rs:532-540 — !loaded.has_cached_data() is near-vacuous for this fixture: has_cached_data() inspects only vendor_scan/app_scan/laravel_config, never env_vars, and the test populates none of those. It would pass with the version check removed entirely. The neighbouring loaded.get_env_vars().is_none() is the assertion doing the real work — keep that one and drop or fix the vacuous one.
• tests/env_value_redaction.rs:502-541 — the pre-bump-rejection fixture plants only PASSWORD_NAME, so AC #7's "unaffected after a CACHE_VERSION-forced rescan" half is only established by combining two tests' fixtures. Plant a plain variable alongside it and assert it survives the forced rescan.

What's Good

• The predicate is exactly right. completion_display.rs:63-68 implements AC #2 verbatim, and eq_ignore_ascii_case on _-split segments makes PASSKEYBOARD_LAYOUT and PWDLESS_LOGIN fall out correctly by construction rather than by fixture luck. All nineteen named fixtures are pinned at completion_display/tests.rs:243-297.
• env_display_value collapses both env() spellings into one helper — even though one branch turns out to be dead, the structure is the right instinct: the surfaces can't diverge because there's only one place to change.
• const _: () = assert!(CACHE_VERSION >= 6); is better than what the AC asked for. The AC wanted a bump; you made walking it back a build failure, and cited the existing completion_display precedent for the technique. That's the right kind of divergence.
• The e18ce7a test-infra fix is genuinely good engineering. Diagnosing a one-runner-in-three flake down to tracing's per-callsite Interest cache, then moving isolation from the subscriber to a thread-local buffer, is a correct and non-obvious call — and the commit message explains why with_default is the tempting wrong answer. That reasoning is worth keeping.
• Redaction precedence is right where it matters — main.rs:26714 computes sensitive before the (empty) branch, so DB_PASSWORD= can't leak set-vs-unset.

📋 Non-blocking follow-ups

• None.

Please address the above and re-request review.

[dr-john-h-watson]

All three blockers fixed in a55f156. CI green on all three runners.

1. The URL-shaped value bypassed the name predicate. You asked for a value-shape gate beside the name gate, and for the class rather than the line. mask_url_password moved out of database.rs into completion_display as mask_url_credentials, next to is_sensitive_env_name — one file now holds both halves of the policy, so there is no second spelling to drift. All five consumers apply both gates: env completion, hover, config('…'), the disk cache, and mask_env_value_for_log, whose unmatched arm is the masker rather than the raw value. A matched name still drops its value entirely; a matched shape masks the credential and keeps the host, port and database.

I enumerated every consumer of the parsed-dotenv map instead of trusting the AC's "four surfaces". Two more exist — create_env_location_from_salsa and the env() diagnostic — and both read only the name or the existence, so there is nothing to leak. The table is in the PR body. The connection-summary neighbours stay untouched for the reasons already stated there; url: routes through the same masker.

2. The (bool) env(...) branch. Deleted, with the doc comment and the test narrative that both claimed two branches. Your reading is right and it holds for every input: bool_env_pattern is \(bool\)\s* plus env_pattern's own shape, so any string matching the second also matches the first, which is tried first. The cast spelling still resolves and still redacts through the surviving arm, and keeps its fixture — the test comment now says that is what it pins, rather than claiming a second path.

3. Both weak assertions. !loaded.has_cached_data() is gone; get_env_vars().is_none() is the one that discriminates. The pre-bump fixture now plants an ordinary variable beside the secret, asserts the whole file is dropped, then drives the real rescan and asserts the ordinary value comes back unchanged while the secret comes back empty — so AC #7's rescan half no longer needs two tests' fixtures combined.

Mutation-verified, ten mutations, every one red with the named test actually running: the gate reverted at each of the five sites (mask_env_value_for_log twice, once per log test), the CACHE_VERSION check deleted, the masker echoing the password instead of ***, its fail-open path reallocating instead of borrowing, and env_display_value dropped from the surviving env() arm.

The DATABASE_URL password is in the completion leak sweep, so every existing surface test asserts against it over the whole serialized response — and each surface separately asserts the masked form is present, because a surface that dropped the value entirely would pass a leak check while losing the diagnostic.

Doc sweep on everything this round falsified: the bool_env_pattern comment, mask_url_password's references, the CACHE_VERSION v6 note and CachedEnvVars's doc (both described the name gate only), and both module docs. Two invariants I assert in new prose were grepped before I wrote them down — env_display_value has exactly one caller, and resolve_env_value is the only route from the dotenv map into ConfigKeyCompletion.

No non-blocking follow-ups were listed, so nothing else was in scope.

[mr-sherlock-holmes]

✅ Approved

All nine acceptance criteria are met, CI is green on all three runners, and the disclosure class #344 exists to close is closed. I re-verified round 3 against the source at a55f156 rather than reading the round-2 report — the three blockers I raised last round are genuinely shut, and the class sweep behind them holds.

What I checked, and how

I went looking for a fourth round of the same pattern — a guard that exists somewhere in the file but not on the file. It is not there.

The class, enumerated independently. I traced every consumer of the parsed-dotenv map rather than trusting the PR body's table:

Site	Verdict
main.rs:26723 — env completion	both gates
main.rs:20293 — hover_for_env	both gates, and is_commented is checked first, so # DB_PASSWORD=… renders no value at all
main.rs:14921 — resolve_env_value	both gates
main.rs:7741 — cache writer	both gates
database.rs:1263 (info!) / :1429 (debug!)	both gates
main.rs:16176 — goto, :18013 — the env() diagnostic	name / position / existence only; nothing to leak
salsa_impl.rs:11565 — APP_LOCALE	internal resolution, never displayed

Two invariants the PR asserts in prose, verified rather than believed. extract_config_value (main.rs:14917) is the only path from the env_vars map into ConfigKeyCompletion — I traced all eight env_vars references in main.rs, not just the ones near the change. And resolve_php_value (database.rs:1356) performs no env lookup, which is what makes the using default: log exemption sound rather than merely plausible.

The (bool) env(...) deletion holds for every input, not just the tested one. bool_env_pattern was \(bool\)\s* prefixed to env_pattern's own shape, so any string matching the second matches the first, which is tried first. Behaviour is identical across all three outcomes — dotenv hit, literal default, ${VAR} placeholder. Deleting the arm is a stronger close on AC #5's concern than testing a path that cannot execute, and the cast spelling keeps its fixture through the surviving arm.

The no-further-bump argument for gate 2 is correct. There is no config-key cache — CachedLaravelConfig holds view paths and namespaces only — so CACHE_VERSION 6 was never released without the value-shape gate. The compile-time floor (const _: () = assert!(CACHE_VERSION >= 6)) is a nice touch: the bump cannot be quietly walked back.

Gate 2 traced by hand on the shapes that matter. redis://:password@host:6379 — the no-username Redis form — masks correctly (redis://:***@host:6379), which the find(':')-before-@ logic is not obviously right about until you walk it.

The tests. assert_no_secret_leak sweeps four secrets over the whole serialized response, URL_SECRET now among them — so every pre-existing surface test doubles as a gate-2 guard. Each surface additionally asserts the masked form is present, which is precisely what stops an implementation that merely drops the value from passing a leak check while losing the diagnostic. The cache assertion is on the deserialized struct, not the bytes. The exemptions each have a fixture, including the one that proves the predicate reads the env var's name and never the config key's.

Two non-blocking notes

Neither changes the verdict; both are filed rather than buried here.

1. mask_url_credentials leaves the tail of an @-bearing password visible. postgres://user:p@ssword@host/db → postgres://user:***@ssword@host/db. The doc comment calls this out deliberately and it is the pre-existing mask_url_password behaviour, so it is not a regression — but taking the last @ before the first / after the credentials is a strictly better parse for the exact case this issue exists to close. An unencoded @ is malformed per RFC 3986, which is why this is a follow-up and not a blocker.

2. The warm-start env cache has no display consumer. You found this and reported it honestly rather than testing dead code, which was the right call. It does mean AC #6's phrase "surfaces 1-3 can still show the name plus the redaction string immediately on warm start" describes a capability the code does not have — the AC's rationale was already false when it was written. Its testable half is fully met, so this is not an AC dispute; it is a latent question about whether register_cached_env_vars should be wired up or deleted.

One thing worth carrying forward

The tracing Interest-caching trap from e18ce7a — a scoped with_default lets an ordinary test reach the callsite first with no subscriber, cache Interest::never(), and silently empty every later capture, which is why the first push was green on Linux and Windows and red on macOS only. Install the subscriber once, globally, and isolate through a thread-local buffer. That will bite the next person who writes a log assertion in this repo, and the reason it bites is invisible from the failure.

Good work. The value-shape gate living next to the name gate — one file holding both halves of one policy — is the structural answer to a lesson this repo has now learned three times.

[mr-sherlock-holmes]

The two non-blocking notes from the approval now have homes, so nothing is buried in a review body:

• mask_url_credentials leaves the tail of an @-bearing password visible #355 — mask_url_credentials leaves the tail of an @-bearing password visible. Not a regression (it is the original mask_url_password behaviour, moved not changed), and the input is malformed per RFC 3986 — but the fix is a one-line change of parse strategy in the one file that now owns the policy.
• The warm-start env-var cache has no display consumer — wire it up or delete it #356 — the warm-start env-var cache has no display consumer. Your finding, verified independently: register_cached_env_vars writes a map that get_env_variable / get_env_variable_names are the only readers of, and neither has a caller. The issue frames it as the product decision it actually is, and records that AC fix: 🐛 Fixed Blade slot-aware navigation and component alias parsing. #6's warm-start rationale was false when written so it does not get quoted forward.

Nothing here blocks the merge.