feat: LoginShell environment policy and include_identity hygiene#12
Conversation
GUI-launched apps on macOS and Linux inherit a minimal PATH that omits login-shell entries (Homebrew, version-manager shims), so tools the app shells out to cannot be found. Wire EnvironmentPolicy::LoginShell to the fix-path-env crate (Tauri's login-shell repair, Apache-2.0 OR MIT) and apply it in Preflight before anything is spawned. The repair mutates the process environment via std::env::set_var, which is UB on Unix under concurrent env access. The variant's rustdoc makes the precondition the caller's explicit obligation: select it only from a single-threaded main() before any thread spawns. This is the same objection that keeps a Custom(vars) variant out of the enum; it is accepted here only because the repair is a single vetted operation, not an open-ended env hook. A failed repair is logged and falls back to the inherited environment, never aborting startup. fix-path-env is pulled in only on macOS/Linux; Windows GUI processes inherit PATH correctly, so the policy is a documented no-op there.
The build.rs-generated identity source hardcoded the absolute path ::gpui_component_manifest::schema::IdentityRef, forcing every consuming app to carry gpui-component-manifest as a runtime dependency (plus a cargo-machete ignore) even when it only reached the macro through gpui-component-app's re-export. Emit the schema types unqualified and have include_identity! expand to a private module that imports them with `use $crate::schema::*` before include!()-ing the generated file. $crate resolves to the defining crate by def-id, so it works transitively through a re-export: an app calling gpui_component_app::include_identity!() now needs the manifest crate only as a build-dependency. Drop the direct runtime dep and machete ignore from both example apps, and switch the downstream fixture to reach the macro through a local re-export shim so it proves the dependency-free path in a hermetic test.
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 18 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (11)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
1 participant
Supersedes closed PR #10, which auto-closed when its base branch
feat/app-platformmerged into main. Rebased onto (merged with) current main, preserving main's CodeRabbit-triage and machete fixes.Two changes:
EnvironmentPolicy::LoginShellPATH repair — GUI apps launched from Finder/dock inherit a strippedPATH. The new policy runsfix-path-envto copy the login shell's environment in, on macOS/Linux only (pulled in via a target-gated dep). Windows is a documented no-op; the policy never aborts startup.include_identity!hygiene — the macro is now dependency-free via$crate, so downstream consumers no longer need a direct dep to expand it. Covered by a newmanifest-reexportfixture and downstream test.Gates:
app/manifesttests, workspace clippy--deny warnings, fmt, and bothapp_shellexamples--smokeall green.