fix: acp process env #1121

zerob13 · 2025-11-24T07:21:59Z

Summary by CodeRabbit

Bug Fixes
- Enhanced environment variable handling for process execution to improve runtime stability and compatibility.
- Tightened iframe sandbox permissions for artifact rendering to strengthen security posture.
Chores
- Updated application packaging to include CDN resources in the distribution build.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

coderabbitai · 2025-11-24T07:22:10Z

Walkthrough

Changes include: (1) adding CDN resource extraction to two electron-builder configuration files to include unpacked resources in the app bundle, (2) refactoring environment variable handling in the process manager to use full propagation with enhanced PATH augmentation and shell environment merging, (3) reducing iframe sandbox permissions by removing same-origin access.

Changes

Cohort / File(s)	Summary
Build Configuration Updates `electron-builder-macx64.yml`, `electron-builder.yml`	Adds extraResources entries to copy and unpack `./resources/cdn/` into `app.asar.unpacked/resources/cdn`, matching existing runtime unpacking patterns across both build configurations.
Process Manager Environment Handling `src/main/presenter/llmProviderPresenter/agent/acpProcessManager.ts`	Replaces environment variable whitelisting with full propagation from `process.env`, then augments with shell environment. Implements multi-source PATH construction combining existing paths, shell environment, default runtime paths, and conditionally adds builtin runtime paths. Adds registry-related environment variables (npm_config_registry, UV_DEFAULT_INDEX, PIP_INDEX_URL) for builtin runtime. Merges agent-provided env vars with PATH-key special handling. Improves logging for debugging.
React Artifact Security `src/renderer/src/components/artifacts/ReactArtifact.vue`	Removes `allow-same-origin` permission from iframe sandbox attribute, restricting sandbox from accessing same-origin resources while preserving `allow-scripts`.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

acpProcessManager.ts requires careful review of the environment variable propagation logic, multiple conditional branches for PATH construction, and interaction with shell environment retrieval, including Windows vs. non-Windows path handling.
ReactArtifact.vue security change warrants verification that removing allow-same-origin doesn't break iframe functionality or communication patterns.
Confirm alignment between the two electron-builder configuration files remains consistent after adding CDN extraResources.

Possibly related PRs

fix: acp spawn issue #1106: Overlapping changes to acpProcessManager.ts for environment variable propagation, PATH normalization, and runtime-aware process spawning.
fix: artifacts code not streaming #732: CDN resource extraction in build configs relates to path-resolution logic expecting cdn resources under app.asar.unpacked/resources/.

Poem

🐰 A bundler hops with resources in tow,
CDN riches now unpacked and aglow,
Paths merge like clover, environments bloom,
Sandboxes tighten—more secure the room! 🌿

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title 'fix: acp process env' directly relates to the main change: fixing environment variable handling in acpProcessManager.ts by replacing whitelist approach with full env propagation.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✨ Finishing touches

📝 Generate docstrings

🧪 Generate unit tests (beta)

Create PR with unit tests
Post copyable unit tests in a comment
Commit unit tests in branch bugfix/acp-process-env-path

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

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

coderabbitai

Actionable comments posted: 0

🧹 Nitpick comments (2)

electron-builder-macx64.yml (1)

32-34: CDN resources mapping looks correct and consistent

The new extraResources entry mirrors the existing ./runtime/ mapping and targets the unpacked ASAR tree under app.asar.unpacked/resources/cdn, which should make CDN assets available at runtime as intended.

As a minor maintenance note, keep electron-builder.yml and electron-builder-macx64.yml in sync whenever adding future resource roots so platform-specific configs don’t drift.

src/main/presenter/llmProviderPresenter/agent/acpProcessManager.ts (1)

271-353: Env seeding and PATH aggregation for Node/UV commands look sound, with a few edge cases to keep in mind

The updated logic to:

seed env from process.env,

enrich it with filtered getShellEnvironment() vars (excluding PATH-like keys), and

rebuild PATH from shellEnv + existing PATH variants + default/runtime paths via normalizePathEnv

is a solid approach for fixing ACP PATH issues, especially for Node/UV commands started from Electron where the GUI’s PATH is often incomplete.

A few small points to consider:

PATH key canonicalization on Windows
Because env is first populated from process.env, you can end up with both PATH and Path entries before normalizePathEnv runs and sets env[pathKey]. On Windows this is case-insensitive, but Node still passes both keys through. It’s probably fine, but worth confirming that RuntimeHelper.normalizePathEnv always returns a canonical key (Path on win32) so the spawned process sees a single authoritative PATH.

Logging verbosity
pathValue can be quite large after merging shell/default/runtime paths. If this log runs frequently in production, you may eventually want to downgrade or gate it behind a debug flag, though it’s very helpful while validating this change.

Extensibility of isNodeCommand
If you later support additional Node-based CLIs (e.g., pnpm, yarn), consider adding them to the isNodeCommand list so they also benefit from this richer PATH construction.

Functionally, the current change aligns well with the PR goal and should significantly reduce “tool not found” issues for ACP agents across platforms.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 50b6f11 and 2686e42.

📒 Files selected for processing (4)

electron-builder-macx64.yml (1 hunks)
electron-builder.yml (1 hunks)
src/main/presenter/llmProviderPresenter/agent/acpProcessManager.ts (1 hunks)
src/renderer/src/components/artifacts/ReactArtifact.vue (1 hunks)

🧰 Additional context used

🧬 Code graph analysis (1)

src/main/presenter/llmProviderPresenter/agent/acpProcessManager.ts (1)

src/main/presenter/llmProviderPresenter/agent/shellEnvHelper.ts (1)

getShellEnvironment (160-240)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)

GitHub Check: build-check (x64)

🔇 Additional comments (2)

electron-builder.yml (1)

33-35: Consistent CDN resources inclusion across platforms

This extraResources entry cleanly aligns with the macx64 config and the existing ./runtime/ mapping, so CDN assets should now be available from the unpacked ASAR on all targets.

Recommend running at least one full build per target to confirm the app.asar.unpacked/resources/cdn tree is populated as expected.

src/renderer/src/components/artifacts/ReactArtifact.vue (1)

7-7: Stronger iframe sandbox with no apparent functional regressions

Dropping allow-same-origin while keeping allow-scripts is a good hardening step here: the component only sets srcdoc and an onload handler and does not access the iframe’s DOM or contentWindow, so it shouldn’t depend on same-origin privileges.

Please double-check any embedded React artifacts that might rely on document cookies/localStorage or parent-window access, just to ensure none of them implicitly depended on same-origin behavior.

zerob13 added 2 commits November 24, 2025 13:47

fix: acp process env

6710073

fix: react artifact failed after build #1116

2686e42

coderabbitai bot reviewed Nov 24, 2025

View reviewed changes

zerob13 merged commit a6b6268 into dev Nov 24, 2025
2 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix: acp process env #1121

fix: acp process env #1121

Uh oh!

zerob13 commented Nov 24, 2025 •

edited by coderabbitai bot

Loading

Uh oh!

coderabbitai bot commented Nov 24, 2025 •

edited

Loading

Uh oh!

coderabbitai bot left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

fix: acp process env #1121

fix: acp process env #1121

Uh oh!

Conversation

zerob13 commented Nov 24, 2025 • edited by coderabbitai bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary by CodeRabbit

Uh oh!

coderabbitai bot commented Nov 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Estimated code review effort

Possibly related PRs

Poem

Pre-merge checks and finishing touches

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

zerob13 commented Nov 24, 2025 •

edited by coderabbitai bot

Loading

coderabbitai bot commented Nov 24, 2025 •

edited

Loading