Skip to content

fix: Add dynamic Git detection; fix Windows 'command not found' and e…#749

Open
felix307253927 wants to merge 3 commits intoRightNow-AI:mainfrom
felix307253927:pr-mian-319
Open

fix: Add dynamic Git detection; fix Windows 'command not found' and e…#749
felix307253927 wants to merge 3 commits intoRightNow-AI:mainfrom
felix307253927:pr-mian-319

Conversation

@felix307253927
Copy link

@felix307253927 felix307253927 commented Mar 19, 2026

…ncoding issues.

Summary

Changes

feat: add dynamic Git Bash path detection
fix: resolve Windows encoding/garbled text issues
fix: handle "command not found" errors on Windows
fix: ensure Git built-in commands are found when using Git Bash on Windows

Testing

  • cargo clippy --workspace --all-targets -- -D warnings passes
  • cargo test --workspace passes
  • Live integration tested (if applicable)

Security

  • No new unsafe code
  • No secrets or API keys in diff
  • User input validated at boundaries

Copy link
Member

@jaberjaber23 jaberjaber23 left a comment

Choose a reason for hiding this comment

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

Security regression in the safe path. The original code intentionally runs commands without shell interpretation (direct argv). This PR wraps ALL safe-path commands in cmd /C on Windows, which reintroduces shell interpretation (pipes, redirects, & all become live). This defeats the entire purpose of the safe path.

Other issues:

  • Box::leak in get_git_sh_path() permanently leaks memory on every call. Use OnceLock or tokio::sync::OnceCell to compute and cache once.
  • Missing std::os::windows::process::CommandExt import for creation_flags(CREATE_NO_WINDOW) — won't compile on Windows.
  • PATH uses Unix colon separators but falls back to cmd.exe in some cases where semicolons are needed.

The cmd /C wrapping must be reverted from the safe path. The memory leak must be fixed. The missing import must be added.

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