Skip to content

fix(desktop): stop simple-git custom-binary warning spam on Windows - #91798

Open
andrexibiza wants to merge 2 commits into
NousResearch:mainfrom
andrexibiza:fix/desktop-simple-git-warning-current-main
Open

fix(desktop): stop simple-git custom-binary warning spam on Windows#91798
andrexibiza wants to merge 2 commits into
NousResearch:mainfrom
andrexibiza:fix/desktop-simple-git-warning-current-main

Conversation

@andrexibiza

@andrexibiza andrexibiza commented Aug 21, 2026

Copy link
Copy Markdown
Contributor

Summary

Supersedes stale/conflicting #79250 while preserving @hernanda-git's exact implementation and Git authorship.

Hermes Desktop resolves Git from trusted process-local locations. On Windows that is normally C:\Program Files\Git\cmd\git.exe, so gitFor() must opt into simple-git's unsafe.allowUnsafeCustomBinary escape hatch. Simple-git emits the same warning on every client construction even when that opt-in is deliberate; the coding/review rail builds clients repeatedly, flooding the terminal and burying real Desktop errors.

This change suppresses only the known custom-binary warning, only during the synchronous simpleGit() construction call, and restores console.warn in finally. Every unrelated warning remains visible.

Field reproduction

A real Windows packaged-app receipt showed the message repeated continuously:

Invalid value supplied for custom binary, restricted characters must be removed or supply the unsafe.allowUnsafeCustomBinary option

The same receipt also contains genuine ENOENT/API/network failures in the same stream. This PR removes the deterministic noise source without suppressing those independent errors.

Provenance

The original #79250 commit by @hernanda-git was replayed through GitHub onto exact upstream main at ac8dff4fbcf47a392a3cddcbec068aa05930ab47; the rebased commit retains the original Git author:

Tests

The focused tests prove:

  • repeated construction through a spaced Windows Git path emits zero copies of the known warning;
  • console.warn is restored immediately, so unrelated warnings still surface;
  • the existing real Git-through-spaced-path contract remains intact.

Exact-head verification

All required hosted workflows attached directly to e90dc2a747d03e124f74b6e41f481d5f296f5119 are green:

GitHub currently reports the PR mergeable, rebaseable, and mergeable_state: clean. No status is inherited from #79250 or an adjacent object.

gitFor() opts into simple-git's unsafe.allowUnsafeCustomBinary when the
resolved git binary path contains a space (the default Windows install,
C:\Program Files\Git\cmd\git.exe). That opt-in trades simple-git's throw
for a console.warn emitted on EVERY client construction, and gitFor() is
not memoized — the coding rail rebuilds a client on each status poll, so
the desktop log filled with hundreds of identical lines that buried real
errors.

Suppress only that known warning, only for the duration of the
construction call, and restore console.warn in a finally so unrelated
warnings still surface.

Fixes NousResearch#79245
@alt-glitch alt-glitch added type/bug Something isn't working P3 Low — cosmetic, nice to have comp/desktop Electron desktop app (apps/desktop/*) platform/windows Native Windows-specific behavior or breakage sweeper:risk-platform-windows Sweeper risk: may break or behave differently on native Windows duplicate This issue or pull request already exists labels Aug 21, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

This was generated by AI during triage.

Duplicate of #79250 — this PR has the same focused simple-git warning-suppression implementation and tests for spaced Windows Git paths.

@alt-glitch alt-glitch removed the duplicate This issue or pull request already exists label Aug 21, 2026
@andrexibiza
andrexibiza requested a review from a team August 21, 2026 23:36
@andrexibiza
andrexibiza force-pushed the fix/desktop-simple-git-warning-current-main branch from c515cd9 to fe5fbdd Compare August 22, 2026 00:01
Preserve @hernanda-git's exact implementation commit as first parent while composing the current upstream main tree as second parent. This reissues the identical tree after an unrelated midnight date-boundary UI test flake; no implementation bytes are regenerated.
@andrexibiza
andrexibiza force-pushed the fix/desktop-simple-git-warning-current-main branch from fe5fbdd to 24f8b20 Compare August 22, 2026 00:14

Copy link
Copy Markdown
Contributor Author

Current-main composition is complete and merge-authorized.

Exact head: 24f8b20ab4e9541420e973e8822942fb97a38695

The commit is a deliberate two-parent composition:

  1. e90dc2a747d03e124f74b6e41f481d5f296f5119@hernanda-git's exact implementation remains literal first parent;
  2. b6bcb3e791c673e63974029bbab40cc9326803ff — exact current upstream main remains literal second parent.

The resulting PR still changes only git-review-ops.ts and its focused test. No implementation bytes were regenerated and no sibling logic was copied.

Fresh exact-head hosted acceptance:

  • CI 32539654520 — success
  • Docker 32539654000 — success
  • Nix 32539653971 — success
  • GitHub — mergeable; zero unresolved review threads

The immediately preceding identical-tree attempt failed only the unrelated midnight boundary in session-row.test.tsx: the test expected Today while the clock had crossed to Yesterday at 11:59 PM. The identical two-parent tree was reissued rather than changing product code, and the complete matrix is now green.

Copy link
Copy Markdown
Contributor Author

Current submitted object has moved since the body’s original receipt, so I re-verified the live head rather than inheriting e90dc2a... evidence.

Current head: 24f8b20ab4e9541420e973e8822942fb97a38695 (Merge current main into desktop git warning repair). The merge commit records that @hernanda-git’s implementation bytes are retained while current upstream main is composed as the second parent.

Exact-head hosted receipts:

  • CI 32539654520 — success
  • Docker Build, Test, and Publish 32539654000 — success
  • Nix flake check 32539653971 — success
  • unresolved inline review threads — 0

This supersedes the body’s historical e90dc2a... workflow receipt for merge-readiness purposes. The PR is currently mergeable; no green status from the predecessor head is being used as authority for the live object.

Copy link
Copy Markdown
Contributor Author

Closure reconciliation against the current object: head 24f8b20ab4e9541420e973e8822942fb97a38695 is mergeable and exact-head green. CI 32539654520, Docker 32539654000, and Nix 32539653971 all completed successfully on this SHA. No implementation work remains on the author side at this generation; the remaining transition is maintainer review/merge.

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

Labels

comp/desktop Electron desktop app (apps/desktop/*) P3 Low — cosmetic, nice to have platform/windows Native Windows-specific behavior or breakage sweeper:risk-platform-windows Sweeper risk: may break or behave differently on native Windows type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants