Skip to content

fix(desktop): resolve login-shell PATH at Electron startup - #1316

Merged
Astro-Han merged 6 commits into
apache:mainfrom
Lynskylate:fix/resolve-login-shell-env
Jul 25, 2026
Merged

fix(desktop): resolve login-shell PATH at Electron startup#1316
Astro-Han merged 6 commits into
apache:mainfrom
Lynskylate:fix/resolve-login-shell-env

Conversation

@Lynskylate

@Lynskylate Lynskylate commented Jul 21, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Restore the user login-shell PATH before desktop stores, tools, and child processes are created, so Finder/Dock/Spotlight launches can find Homebrew, user-local, and version-manager binaries.
  • Import only PATH; shell startup files cannot inject Maka, Electron, renderer, or other application-control variables into the running main process.
  • Prefer the inherited shell, then the OS account shell, with platform-appropriate fallbacks. Capture is bounded, never logs raw shell stderr, degrades to the inherited PATH on failure, and terminates the dedicated process group on every completion path.
  • Keep E2E command resolution deterministic through MAKA_SKIP_SHELL_ENV.

Verification

  • npm run build:test
  • npm run typecheck
  • npm run lint
  • npm run format:check
  • npm --workspace @maka/desktop test — 2863 passed
  • node --test apps/desktop/dist/main/__tests__/shell-env.test.js — 18 passed; repeated runs passed
  • Desktop console, accessibility, and copy policy checks passed
  • Four independent final reviews: no P0–P3 findings, GO

Review focus

The boundary is intentionally PATH-only. The issue is missing CLI lookup paths in GUI launches; importing the full login environment would mix shell-owned data with application control state and require an open-ended denylist.

Comment thread apps/desktop/src/main/main.ts
Comment thread apps/desktop/src/main/shell-env.ts
Comment thread apps/desktop/src/main/shell-env.ts Outdated
Comment thread apps/desktop/src/main/shell-env.ts Outdated
Comment thread apps/desktop/src/main/shell-env.ts Outdated
Comment thread apps/desktop/src/main/shell-env.ts Outdated
Comment thread apps/desktop/src/main/shell-env.ts
@Astro-Han

Copy link
Copy Markdown
Contributor

Got some review comments, feel free to push back or resolve the convesation! None of the P2 P3s are blocking and it's just discussion. The P1 is a safety blocker we need to address.

@Lynskylate
Lynskylate force-pushed the fix/resolve-login-shell-env branch from 9b0c070 to e122cc0 Compare July 22, 2026 17:09
Lynskylate pushed a commit to Lynskylate/maka-agent that referenced this pull request Jul 22, 2026
Spawn the user's login shell at Electron startup to recover a full PATH for
apps launched from Finder/Dock/Spotlight (port of VS Code's shellEnv.ts).

Review (apache#1316) feedback folded in:
- short-circuit resolveShellEnv when launched from a terminal (TERM/COLORTERM)
- drop the dead xonsh capture branch (intentionally unsupported)
- shell-escape process.execPath per quoting context (POSIX/pwsh/nu)
- strip XDG_RUNTIME_DIR from the resolved env (microsoft/vscode#22593)
- preserve the Electron trio + MAKA_* via a prefix rule; drop MAKA_RESOLVING_ENVIRONMENT
- export mergeEnv/buildCaptureCommand/buildMarkerRegex + add unit tests
- E2E fixtures set MAKA_SKIP_SHELL_ENV=1 so the probe never taints the sanitized env (P1)

Rebased onto main (incorporates apache#1314 visual-smoke -> e2e-fixture rename).
@Lynskylate

Copy link
Copy Markdown
Contributor Author

Thanks @Astro-Han — all seven comments are addressed in e122cc0.

P1 (blocker) — E2E env taint. buildE2eEnv now sets MAKA_SKIP_SHELL_ENV=1, so resolveShellEnv never mutates the sanitized env (no re-imported *_API_KEY, no bootstrapped connection). Owning-layer fix in apps/desktop/e2e/fixtures.ts; deterministic under xvfb, unlike relying on TERM.

P2 — terminal short-circuit. resolveShellEnv now returns early when TERM / COLORTERM is set. LaunchServices (Finder/Dock/Spotlight) never sets either; terminals and CLI launches always do, so resolution is skipped when the session env is already complete.

#3 — dead xonsh branch. Removed. Its print(mark, json, mark) output was space-separated and could never match the flush-marker regex, so the branch was dead. xonsh now falls into the POSIX arm (intentionally unsupported — not a Maka audience); a real xonsh shell fails capture gracefully and the app keeps the original env.

#4 — shell-escape execPath. Extracted buildCaptureCommand(shellName, execPath, mark); process.execPath is escaped per quoting context (POSIX '\'', PowerShell '', nu left as-is — documented edge case).

#5XDG_RUNTIME_DIR. mergeEnv strips it from the resolved env (microsoft/vscode#22593) so the login shell's runtime dir doesn't persist into GUI children.

#6 — prefix-rule preservation + dead marker. Replaced the hand-maintained preservedKeys list with a prefix rule (ELECTRON_RUN_AS_NODE | ELECTRON_NO_ATTACH_CONSOLE | ORIGINAL_XDG_CURRENT_DESKTOP | MAKA_*) and removed the dead MAKA_RESOLVING_ENVIRONMENT. Applied for robustness against future MAKA_* renames.

Re: the rebase — you were right, the branch was stale. It's now rebased onto current main, which includes #1314's visual-smokee2e-fixture rename (MAKA_VISUAL_SMOKE_FIXTUREMAKA_E2E_FIXTURE, new buildE2eEnv signature). mergeable=true.

#7 — test gap. Exported mergeEnv / buildCaptureCommand / buildMarkerRegex and added apps/desktop/src/main/__tests__/shell-env.test.ts — 16 node:test cases covering per-shell quoting + apostrophe round-trips, marker adjacency (incl. the xonsh non-match that would have caught #3), and mergeEnv preservation/stripping.

Verified: build:main clean; all 16 tests green.

Lynskylate pushed a commit to Lynskylate/maka-agent that referenced this pull request Jul 22, 2026
…nsole

PR apache#1316's test job failed the workspace console.* audit: shell-env.ts
adds three startup diagnostics (capture-failure warn, shell-stderr debug,
PATH-entry-count log). Add the file to the ALLOW map — it matches the
sibling main-process diagnostic modules (config-file-watcher, daily-review,
app-lifecycle) and leaks no secrets (only PATH count, shell name, error class).
@Astro-Han

Copy link
Copy Markdown
Contributor

I took another pass over e122cc0. The E2E skip, terminal short-circuit, xonsh removal, quoting fix, XDG_RUNTIME_DIR handling, and added tests address the earlier review comments.

There is still one production-path issue. captureLoginShellEnv() adds ELECTRON_RUN_AS_NODE=1 and ELECTRON_NO_ATTACH_CONSOLE=1 to the probe environment, and both values come back in the captured JSON. mergeEnv() only preserves keys that already exist in process.env. On a normal GUI launch these two keys are absent, so the probe values get copied into the main process and inherited by later child processes.

The E2E skip does not cover this. It prevents the probe from running in E2E, while the leak happens during a normal Finder or Dock launch.

I would clean up the captured environment before returning it:

const PROBE_ENV_KEYS = [
  'ELECTRON_RUN_AS_NODE',
  'ELECTRON_NO_ATTACH_CONSOLE',
] as const;

export function sanitizeCapturedEnv(
  captured: Record<string, string>,
  original: NodeJS.ProcessEnv,
): Record<string, string> {
  const resolved = { ...captured };

  for (const key of PROBE_ENV_KEYS) {
    const value = original[key];
    if (value === undefined) {
      delete resolved[key];
    } else {
      resolved[key] = value;
    }
  }

  delete resolved.XDG_RUNTIME_DIR;
  return resolved;
}

Then snapshot the original environment before adding the probe flags:

const originalEnv = { ...process.env };
const env = {
  ...originalEnv,
  ELECTRON_RUN_AS_NODE: '1',
  ELECTRON_NO_ATTACH_CONSOLE: '1',
};

// After parsing the shell output:
resolve(sanitizeCapturedEnv(parsed, originalEnv));

Please add the two cases that match the real probe result: remove an injected value when the original value was absent, and restore the exact value when it was already present. Once cleanup happens here, the Electron-specific preservation in mergeEnv() can go away.

One separate cleanup remains: the test job currently fails check-console on the three new console.* calls in shell-env.ts. Those need to use the repository’s logging path or be explicitly allowed with a reason.

After these two fixes, I do not see anything else blocking this PR.

yiling and others added 3 commits July 25, 2026 20:21
Spawn the user's login shell at Electron startup to recover a full PATH for
apps launched from Finder/Dock/Spotlight (port of VS Code's shellEnv.ts).

Review (apache#1316) feedback folded in:
- short-circuit resolveShellEnv when launched from a terminal (TERM/COLORTERM)
- drop the dead xonsh capture branch (intentionally unsupported)
- shell-escape process.execPath per quoting context (POSIX/pwsh/nu)
- strip XDG_RUNTIME_DIR from the resolved env (microsoft/vscode#22593)
- preserve the Electron trio + MAKA_* via a prefix rule; drop MAKA_RESOLVING_ENVIRONMENT
- export mergeEnv/buildCaptureCommand/buildMarkerRegex + add unit tests
- E2E fixtures set MAKA_SKIP_SHELL_ENV=1 so the probe never taints the sanitized env (P1)

Rebased onto main (incorporates apache#1314 visual-smoke -> e2e-fixture rename).
…nsole

PR apache#1316's test job failed the workspace console.* audit: shell-env.ts
adds three startup diagnostics (capture-failure warn, shell-stderr debug,
PATH-entry-count log). Add the file to the ALLOW map — it matches the
sibling main-process diagnostic modules (config-file-watcher, daily-review,
app-lifecycle) and leaks no secrets (only PATH count, shell name, error class).
@Astro-Han
Astro-Han force-pushed the fix/resolve-login-shell-env branch from 3f29071 to 2b5ddb0 Compare July 25, 2026 12:28
@Astro-Han Astro-Han changed the title fix(desktop): resolve login-shell environment at Electron startup fix(desktop): resolve login-shell PATH at Electron startup Jul 25, 2026
@Astro-Han
Astro-Han merged commit cc7c32a into apache:main Jul 25, 2026
3 checks passed
@Lynskylate
Lynskylate deleted the fix/resolve-login-shell-env branch July 26, 2026 16:39
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.

2 participants