fix: native Windows auto-update, PATH cap, and Ctrl+C terminal restore - #124
Conversation
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
🤖 CodeAnt AI — Review Status
|
Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
| #[cfg(windows)] | ||
| if let Some(path) = sanitize_windows_path() { | ||
| command.env("PATH", path); |
There was a problem hiding this comment.
Suggestion: This replaces the inherited Windows PATH for every shell command and silently drops all entries after the 1900-character cap. Commands relying on a user-installed executable located later in an otherwise valid PATH will now fail, even when no PATH pollution exists. Restrict the sanitized PATH to the updater or preserve required entries through a more targeted resolution strategy. [api mismatch]
Severity Level: Major ⚠️
- ❌ Windows shell commands outside the retained PATH fail to resolve.
- ⚠️ UI shell actions and update-related commands lose user tools.
- ⚠️ Commands after the cap fail only under long PATH configurations.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** crates/forge_infra/src/executor.rs
**Line:** 60:62
**Comment:**
*Api Mismatch: This replaces the inherited Windows `PATH` for every shell command and silently drops all entries after the 1900-character cap. Commands relying on a user-installed executable located later in an otherwise valid PATH will now fail, even when no PATH pollution exists. Restrict the sanitized PATH to the updater or preserve required entries through a more targeted resolution strategy.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| let primary = std::env::var("HELIOSLITE_UPDATE_URL") | ||
| .unwrap_or_else(|_| "https://helioslite.dev/cli".to_string()); | ||
| let fallback = "https://forgecode.dev/cli"; | ||
| format!("(curl -fsSL {primary} || curl -fsSL {fallback}) | sh") |
There was a problem hiding this comment.
Suggestion: Untrusted environment data is interpolated directly into a shell command executed by execute_shell_command_raw. A value such as a URL containing shell metacharacters can terminate the curl command or add another command, resulting in arbitrary command execution whenever update checking runs. Validate the URL and pass it without shell interpolation, or quote it safely. [security]
Severity Level: Critical 🚨
- ❌ Update checking can execute attacker-controlled shell commands.
- ❌ Compromised environment configuration enables arbitrary local code execution.
- ⚠️ The documented update URL override expands the attack surface.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** crates/forge_main/src/update.rs
**Line:** 74:77
**Comment:**
*Security: Untrusted environment data is interpolated directly into a shell command executed by `execute_shell_command_raw`. A value such as a URL containing shell metacharacters can terminate the `curl` command or add another command, resulting in arbitrary command execution whenever update checking runs. Validate the URL and pass it without shell interpolation, or quote it safely.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| let install_dir = format!(r"{local_app_data}\Programs\Forge"); | ||
| let new_exe = format!(r"{install_dir}\forge.exe.new"); | ||
| let exe = format!(r"{install_dir}\forge.exe"); |
There was a problem hiding this comment.
Suggestion: The Windows updater hardcodes %LOCALAPPDATA%\Programs\Forge\forge.exe, but this fork's packaging installs forge-dev.exe under %LOCALAPPDATA%\forge-dev\bin. The updater will download into an unrelated directory, leave the actual installed binary unchanged, and attempt to relaunch a nonexistent executable. Derive the executable and install directory from the running binary or the packaging contract. [api mismatch]
Severity Level: Major ⚠️
- ❌ Packaged Windows HeliosLite installations remain outdated.
- ❌ Auto-update launches an unrelated or nonexistent `forge.exe`.
- ⚠️ Downloaded update files accumulate outside the actual install directory.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** crates/forge_main/src/update.rs
**Line:** 97:99
**Comment:**
*Api Mismatch: The Windows updater hardcodes `%LOCALAPPDATA%\Programs\Forge\forge.exe`, but this fork's packaging installs `forge-dev.exe` under `%LOCALAPPDATA%\forge-dev\bin`. The updater will download into an unrelated directory, leave the actual installed binary unchanged, and attempt to relaunch a nonexistent executable. Derive the executable and install directory from the running binary or the packaging contract.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| %SystemRoot%\\System32\\tasklist.exe /FI \"IMAGENAME eq forge.exe\" 2>nul | %SystemRoot%\\System32\\find.exe /I \"forge.exe\" >nul\r\n\ | ||
| if not errorlevel 1 (\r\n\ | ||
| %SystemRoot%\\System32\\timeout.exe /t 1 /nobreak >nul\r\n\ | ||
| goto wait\r\n\ | ||
| )\r\n\ | ||
| move /Y \"{new_exe}\" \"{exe}\"\r\n\ |
There was a problem hiding this comment.
Suggestion: The swap helper waits for any process named forge.exe, not the Forge process that launched the update. If another Forge instance or stale process is running, the helper waits for up to 900 seconds and then deletes the downloaded binary. The deferred failure is invisible because the parent only observes the PowerShell launch status. Track the specific parent process or use a process-specific synchronization mechanism. [possible bug]
Severity Level: Major ⚠️
- ❌ Concurrent Forge sessions can prevent Windows updates.
- ❌ The staged binary is deleted after the 900-second timeout.
- ⚠️ The parent reports success before deferred swapping completes.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** crates/forge_main/src/update.rs
**Line:** 111:116
**Comment:**
*Possible Bug: The swap helper waits for any process named `forge.exe`, not the Forge process that launched the update. If another Forge instance or stale process is running, the helper waits for up to 900 seconds and then deletes the downloaded binary. The deferred failure is invisible because the parent only observes the PowerShell launch status. Track the specific parent process or use a process-specific synchronization mechanism.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fixThere was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 55e8ef04cd
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| let install_dir = format!(r"{local_app_data}\Programs\Forge"); | ||
| let new_exe = format!(r"{install_dir}\forge.exe.new"); | ||
| let exe = format!(r"{install_dir}\forge.exe"); |
There was a problem hiding this comment.
When users install with the repository's Windows installer, install.ps1 places helioslite.exe under %LOCALAPPDATA%\helioslite\bin (or HELIOSLITE_INSTALL_DIR) and only creates forge.exe as an alias there, but this updater writes to %LOCALAPPDATA%\Programs\Forge\forge.exe and relaunches that separate path. In the standard Windows install this reports a successful auto-update while the PATH entry still points at the old helioslite.exe/alias, so the next invocation remains stale; derive the directory and executable name from the running binary or the installer setting instead.
Useful? React with 👍 / 👎.
| $dir = Join-Path $env:LOCALAPPDATA 'Programs\Forge' | ||
| New-Item -ItemType Directory -Force -Path $dir | Out-Null | ||
| $arch = if ($env:PROCESSOR_ARCHITECTURE -eq 'ARM64') {{ 'aarch64' }} else {{ 'x86_64' }} | ||
| $repo = if ($env:HELIOSLITE_REPO) { $env:HELIOSLITE_REPO } else { 'KooshaPari/forgecode' } |
There was a problem hiding this comment.
On Windows this #[cfg(windows)] branch is compiled, and the single { ... } blocks in the format! string are parsed as Rust formatting placeholders rather than literal PowerShell braces (unlike the escaped braces on the preceding line). That makes the Windows updater fail to compile before it can run; escape these braces with {{/}} or avoid format! for the literal script.
Useful? React with 👍 / 👎.
|
Dependency limit exceeded — report not shown. This pull request scan exceeded the 10,000-dependency limit applied to this scan, so the results are incomplete and may be inaccurate. To avoid reporting false positives, Socket has not posted a report. Upgrade your plan to raise the dependency limit and get complete reports, or view the partial scan in the dashboard. Socket is always free for open source. If this is a non-commercial open source project, contact us to request a free Team account. |
User description
Summary
Fixes two issues found on the user's machines (Windows auto-update failure + zsh Ctrl+C session corruption). This PR is a fork-internal integration of the fixes ??? not intended for upstream merge.
Changes
1. Windows auto-update:
'curl' is not recognized as an internal or external commandcrates/forge_main/src/update.rs??? the updater previously rancurl -fsSL https://forgecode.dev/cli | shviacmd.exe /C, which is POSIX-only and breaks on native Windows (nosh, and PATH pollution past cmd.exe's ~2 KB resolution limit makescurlunresolvable). Now it branches per-platform: Windows uses a native PowerShell command (absolutepowershell.exepath) that downloadsforge-{arch}-pc-windows-msvc.exefrom GitHub releases and atomically swaps the binary through a detached.cmdhelper.crates/forge_infra/src/executor.rs??? every Windows child process now gets a sanitized PATH:System32,Wbem, and PowerShellv1.0are always present first, entries are deduped case-insensitively, and the total is capped at 1900 chars. This fixes command resolution for spawnedcmd.exeeven when the user PATH is polluted beyond the batch-parser limit.2. zsh Ctrl+C corrupts the session (broken input, raw mode left on)
crates/forge_select/src/preview.rs+signal-hookdependency ??? a process-wide SIGINT safety-net flag restores the terminal (cursor, viewport, raw mode) before the picker returns, so Ctrl+C in raw mode can no longer strand the shell with broken input. Coexists with the existingtokio::signal::ctrl_c()in the main UI (signal-hook chains handlers).shell-plugin/lib/helpers.zsh???_forge_exec_interactivenow runs the interactive forge in its own process group (zle -I,setopt LOCAL_OPTIONS MONITOR, background&, thenfgwith afg || wait $!fallback). Ctrl+C targets forge only, never the ZLE widget, so_forge_resetalways runs and ZLE state stays clean.shell-plugin/lib/dispatcher.zsh???forge-accept-lineinstalls a function-scoped INT trap (setopt LOCAL_TRAPS) that restores the buffer/prompt if a stray signal reaches the widget, auto-clearing on every exit path (including early returns).Files
crates/forge_main/src/update.rscrates/forge_infra/src/executor.rscrates/forge_select/src/preview.rscrates/forge_select/Cargo.tomlCargo.lockshell-plugin/lib/helpers.zshshell-plugin/lib/dispatcher.zshValidation
cargo check -p forge_select -p forge_infra -p forge_main??? passescargo test -p forge_main --lib update::??? passeszsh -non both modified plugin files ??? passes (rc 0)forge-x86_64-pc-windows-msvc.exe, valid MZ PE header)cmd.execommand resolution; no signal handler on SIGINT)CodeAnt-AI Description
Fix Windows updates and prevent terminal corruption after Ctrl+C
What Changed
Impact
✅ Working native Windows auto-updates✅ Fewer command failures with long Windows PATH values✅ Usable terminal after Ctrl+C💡 Usage Guide
Checking Your Pull Request
Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.
Talking to CodeAnt AI
Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:
This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.
Example
Preserve Org Learnings with CodeAnt
You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:
This helps CodeAnt AI learn and adapt to your team's coding style and standards.
Example
Retrigger review
Ask CodeAnt AI to review the PR again, by typing:
Check Your Repository Health
To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.