Skip to content

fix: preserve shell path for gh cli#172

Merged
Astro-Han merged 1 commit into
devfrom
codex/fix-i170-gh-cli-path
Apr 22, 2026
Merged

fix: preserve shell path for gh cli#172
Astro-Han merged 1 commit into
devfrom
codex/fix-i170-gh-cli-path

Conversation

@Astro-Han

@Astro-Han Astro-Han commented Apr 22, 2026 •
edited by coderabbitai Bot
Loading

Copy link
Copy Markdown
Owner

Summary

Preserve the login shell PATH when PawWork prepares the embedded server environment, so Homebrew-installed commands such as gh remain discoverable from PawWork shell flows.

Why

PR #167 correctly preserved GitHub CLI auth by deriving GH_CONFIG_DIR before PawWork overrides XDG_CONFIG_HOME, but it also changed the general env merge so a narrow macOS GUI process.env.PATH could override the login shell PATH. This made gh look missing inside PawWork even though the normal terminal could resolve it.

Related Issue

Fixes #170.

How To Verify

cd packages/desktop-electron
bun test src/main/server.test.ts
bun test src/main/shell-env.test.ts
bun run typecheck

Screenshots or Recordings

Not applicable. No visible UI change.

Checklist

  • I linked the related issue, or stated why there is no issue
  • This PR has type, scope, and priority labels, or I requested maintainer labeling
  • I listed the relevant verification steps, including tests when behavior changed
  • I manually checked visible UI or copy changes when needed, with screenshots or recordings
  • I considered macOS and Windows impact for desktop, packaging, updater, signing, paths, shell, or permissions changes
  • I called out docs, release notes, dependencies, permissions, credentials, deletion behavior, or generated/local file changes when relevant
  • I am targeting dev, and my PR title and commit messages use Conventional Commits in English

Summary by CodeRabbit

  • Bug Fixes

    • Improved environment variable precedence handling to correctly prioritize shell-level settings over process-level configurations.

  • Tests

    • Added test cases to verify correct precedence ordering for environment variables across different configuration scenarios.

@Astro-Han Astro-Han added bug Something isn't working P1 High priority platform Electron shell, OS integration, packaging, updater, signing, paths, and permissions labels Apr 22, 2026
@coderabbitai

coderabbitai Bot commented Apr 22, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 75bb6294-755a-48ea-97e2-6c59c68bef65

📥 Commits

Reviewing files that changed from the base of the PR and between d42139f and 6cb558f.

📒 Files selected for processing (2)
  • packages/desktop-electron/src/main/server.test.ts
  • packages/desktop-electron/src/main/server.ts
📝 Walkthrough

Walkthrough

This change fixes environment variable precedence in buildServerEnv to ensure shell PATH takes priority over process PATH, preventing the GitHub CLI from being masked. Two test cases verify that shell environment variables are correctly preserved and that configuration directory precedence works as expected.

Changes

Cohort / File(s) Summary
Server Environment Construction
packages/desktop-electron/src/main/server.ts		 Modified buildServerEnv to compute ghConfigDir from merged originalEnv (shell + process), then explicitly set PATH from shellEnv when available, ensuring shell PATH is not overridden by process PATH.
Test Coverage
packages/desktop-electron/src/main/server.test.ts		 Added two non-Windows test cases: one verifies shell PATH precedence over process PATH; the other validates GH_CONFIG_DIR from process, PATH from shell, and XDG_CONFIG_HOME forced to server roots.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Possibly related PRs

Poem

🐰 The PATH was lost, gh could not be found,
But shell and process tangled all around!
Now order reigns—shell leads the way,
And gh commands work again, hooray! ✨

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly summarizes the main change: preserving shell PATH for the gh CLI, which is the core objective of the PR.
Description check ✅ Passed The description covers all key sections: Summary explains the problem, Why provides context of the regression, Related Issue links #170, How To Verify includes test commands, and the checklist is fully completed.
Linked Issues check ✅ Passed The code changes directly address issue #170 by preserving the login shell PATH in buildServerEnv while computing GH_CONFIG_DIR from a proper merge to maintain GitHub CLI auth.
Out of Scope Changes check ✅ Passed All changes are scoped to fixing the PATH precedence regression in buildServerEnv and adding corresponding test coverage; no unrelated modifications.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch codex/fix-i170-gh-cli-path

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

gemini-code-assist[bot]
gemini-code-assist Bot reviewed Apr 22, 2026
View reviewed changes

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request updates the environment variable merging logic in the desktop server to ensure that the PATH from the login shell is preserved, preventing it from being overridden by the process environment. This ensures that Homebrew commands and other shell-specific paths remain discoverable. The changes also maintain the priority of GitHub CLI configuration from the process environment. Corresponding test cases have been added to verify these behaviors. I have no feedback to provide.

@Astro-Han Astro-Han merged commit 0e39c3e into dev Apr 22, 2026
26 checks passed
@Astro-Han Astro-Han deleted the codex/fix-i170-gh-cli-path branch April 22, 2026 21:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@gemini-code-assist gemini-code-assist[bot] gemini-code-assist[bot] left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

bug Something isn't working P1 High priority platform Electron shell, OS integration, packaging, updater, signing, paths, and permissions

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[Bug] GitHub CLI is missing from PawWork shell environment

1 participant

@Astro-Han