Skip to content

feat(app): LoginShell PATH repair and dependency-free include_identity!#10

Closed
BumpyClock wants to merge 2 commits into
feat/app-platformfrom
feat/app-platform-followups
Closed

feat(app): LoginShell PATH repair and dependency-free include_identity!#10
BumpyClock wants to merge 2 commits into
feat/app-platformfrom
feat/app-platform-followups

Conversation

@BumpyClock

Copy link
Copy Markdown
Owner

Two follow-ups on the AppShell platform stack. Stacks on #8 (base: feat/app-platform, not main); review/merge #8 first.

1. Real EnvironmentPolicy::LoginShell (tsq-5)

LoginShell previously logged a warning and behaved as Inherit. It now performs the macOS/Linux GUI-launch PATH repair that agent-term needs before it can migrate.

  • Repair runs in Preflight via the fix-path-env crate (Tauri's login-shell PATH fixer, Apache-2.0 OR MIT, the same crate + rev agent-term already uses). Pulled in only on macOS/Linux; Windows is a documented no-op (GUI processes inherit PATH from the launching context).
  • Soundness precondition made explicit. The repair copies the login shell's env into the process via std::env::set_var, which is UB on Unix under concurrent env access. The variant's rustdoc places the obligation on the caller: select LoginShell only from a single-threaded main() before any thread spawns. This mirrors why the deleted Custom(vars) variant was rejected — accepted here only because it's a single vetted repair, not an open-ended env hook. The no-Custom comment block is updated to reference it.
  • A failed repair is logged + falls back to Inherit, never a startup abort.
  • Tests cover the policy plumbing (default is Inherit, setter records LoginShell, Inherit is a no-op); actual PATH mutation is not asserted in CI.

2. include_identity! needs no direct manifest dependency (tsq-6)

The generated identity source hardcoded ::gpui_component_manifest::schema::IdentityRef, an absolute path that forced every consuming app onto a runtime dependency on gpui-component-manifest (plus a cargo-machete ignore) even when it reached the macro through gpui-component-app's re-export.

  • The generated source now names schema types unqualified. include_identity! expands to a private module that imports them with use $crate::schema::{…} before include!()-ing the file. $crate resolves to the defining crate by def-id, so it works transitively through a re-export.
  • Apps calling gpui_component_app::include_identity!() now need gpui-component-manifest only as a [build-dependencies] entry (for emit_identity() in build.rs).
  • Both example apps drop the direct runtime dep and the machete ignore. The downstream fixture now reaches the macro through a small local re-export shim, proving the dependency-free path in a hermetic test (no gpui compile). A build.rs unit test locks the "no absolute manifest path" contract.

Gates

  • cargo test -p gpui-component-manifest -p gpui-component-app — pass (incl. downstream fixture + new env/macro tests)
  • app_shell + app_shell_tray build and --smoke exit 0
  • cargo clippy --workspace --all-targets -- -D warnings — clean
  • cargo fmt --check — clean

Note: cargo machete reports two dependencies (gpui-component-assets in crates/app, smol in examples/system_monitor) — both pre-existing on feat/app-platform, untouched here. This PR introduces no new machete findings.

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.
@coderabbitai

coderabbitai Bot commented Jul 17, 2026

Copy link
Copy Markdown

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 08bc9b85-8d81-45ff-87ff-689eb3e083bc

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/app-platform-followups

Comment @coderabbitai help to get the list of available commands.

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