Skip to content

fix(core): bypass Windows Git lookup - #43998

Closed
opencode-agent[bot] wants to merge 1 commit into
v2from
windows-git-path
Closed

opencode-agent[bot] wants to merge 1 commit into
v2from
windows-git-path

Conversation

@opencode-agent

Copy link
Copy Markdown
Contributor

Summary

  • resolve the Git executable once on Windows through a shared Core helper
  • use the resolved executable for repository, worktree, snapshot, and VCS operations
  • bypass cross-spawn lookup for absolute native Windows .exe and .com commands
  • preserve existing lookup behavior for non-Windows and shell-backed commands

Verification

  • bun typecheck in packages/util
  • bun typecheck in packages/core
  • bun test test/git.test.ts test/vcs.test.ts in packages/core (12 passed)
  • git diff --check

Requested by: @neriousy (Filip via Slack)

@Enough1122

Copy link
Copy Markdown

AI code review — automated review for reference; please use your judgment.

  1. packages/core/src/util/git-executable.ts:4 — which("git") runs once at module import and is frozen for the process lifetime. If git is installed after startup, or the effective PATH changes (per-server/project env overrides flow through these spawns), every git call keeps using the stale-or-missing resolution with no way to refresh. Why it matters: a wrong pin silently persists across the whole session. Suggestion: memoize lazily behind a function (getGitExecutable()) with an explicit invalidation (or short TTL), so env changes and first-use timing are handled and tests can stub it.

  2. packages/core/src/util/git-executable.ts:4 — Resolution uses process.env.PATH, but the call sites pass per-invocation env with extendEnv: true; a deployment that intentionally puts a specific git first in the spawn env is now ignored in favor of the host-wide lookup. Why it matters: behavior change for env-customized setups, undocumented. Suggestion: either key the cache by the resolved PATH value or document that only the launcher's PATH decides which git runs.

  3. packages/util/src/cross-spawn-spawner.ts:265 — The native-vs-cross-spawn predicate (win32 && !shell && absolute && .exe/.com) embeds real security-relevant decisions (deliberately excluding .bat/.cmd) but is inline and untested, and CI likely never runs it. Suggestion: extract shouldSpawnNativeWindows(command, opts) as an exported pure function and add table tests covering extension casing, relative paths, shell opt, and non-win32.

  4. packages/core/src/util/git-executable.ts:6 — Falling back to "git" keeps today's failure mode, which is fine, but a one-time warning when which() finds nothing on Windows would turn "commands fail mysteriously" into a diagnosable setup error. Suggestion: log once (guarded by a module flag).

  5. packages/core/src/git.ts:318 (nit) — Worth adding a guardrail against regressions: a lint rule (no-restricted-syntax) or CI grep asserting no new ChildProcess.make("git", …) literals sneak back in now that gitExecutable exists — the diff touches four call sites, and a fifth already exists somewhere else is exactly how this class of fix rots.

— automated review (ox-alpha, round2)

@github-actions

Copy link
Copy Markdown
Contributor

Automated PR Cleanup

Thank you for contributing to opencode.

Due to the high volume of PRs from users and AI agents, we periodically close older PRs using automated criteria so maintainers can focus review time on the most active and community-supported contributions.

This PR was closed because it matched the following cleanup criteria:

  • The PR was created more than 1 month ago
  • The PR had fewer than 2 positive reactions
  • Positive reactions are counted as thumbs-up, heart, celebration, or rocket reactions on the PR

PRs created within the last month are not affected by this cleanup.

If you believe this PR was closed incorrectly, or if you are still actively working on it, please leave a comment explaining why it should be reopened. A maintainer can review and reopen it if appropriate.

Thanks again for taking the time to contribute.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants