fix(profile): empty allow_vars no longer strips all env vars#1204
fix(profile): empty allow_vars no longer strips all env vars#1204SequeI wants to merge 2 commits into
Conversation
PR Review SummarySize
Affected crates
Blast radius — ModerateThis PR touches: source code,documentation Updated automatically on each push to this PR. |
There was a problem hiding this comment.
Code Review
This pull request updates the environment variable filtering logic in profile_runtime.rs so that an empty allow_vars list returns None instead of activating an empty allow filter (which would block all environment variables). The feedback points out that the new tests duplicate production logic in a helper function, which can lead to test drift. It is recommended to refactor the tests to use the actual prepare_profile entry point with temporary files on disk to ensure the real production code path is validated.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
When a profile contained an `environment` block without explicit `allow_vars` (e.g. only `deny_vars` or `set_vars`), `allowed_env_vars` resolved to `Some([])` instead of `None`. Because `Some([])` activates the allow-list filter with nothing in it, every inherited env var was silently stripped — including `PATH` and `HOME`. Fix: change `.map` to `.and_then` and return `None` when `allow_vars` is empty, consistent with the existing `deny_vars` handling. Signed-off-by: Aleksy Siek <aleksy@alwaysfurther.ai>
Linked Issue
Ref #1203
Summary
When a profile contained an
environmentblock without explicitallow_vars(e.g. onlydeny_varsorset_vars),allowed_env_varsresolved toSome([])instead ofNone. BecauseSome([])activates the allow-list filter with nothing in it, every inherited env var was silently stripped — includingPATHandHOME.Fix: change
.mapto.and_thenand returnNonewhenallow_varsis empty, consistent with the existingdeny_varshandling.Test Plan
Checklist
CHANGELOG.mdif needed