Skip to content

feat: complete native Windows 11 support#285

Merged
benvinegar merged 4 commits into
modem-dev:mainfrom
Ofekw:feat/windows-support
May 11, 2026
Merged

feat: complete native Windows 11 support#285
benvinegar merged 4 commits into
modem-dev:mainfrom
Ofekw:feat/windows-support

Conversation

@Ofekw
Copy link
Copy Markdown
Contributor

@Ofekw Ofekw commented May 11, 2026

In hopes of moving #283 along.

Heads-up on scope

The biggest call here is replacing three bash scripts (build-bin, build-npm, install-bin) with TypeScript. The smaller alternatives I considered:

  • Keep the bash scripts and document Git Bash as a Windows requirement. This is what already happens implicitly in CI (windows-latest ships Git Bash), but it pushes real friction onto Windows contributors and doesn't fix the bash: not found errors on stock Windows.
  • Ship parallel .ps1 and .sh pairs. More files, two ways to drift, and the rest of this directory is already TS.

If you'd rather take a smaller version of this PR, the docs and the small fixes (stage-prebuilt-npm.ts dist/hunk.exe lookup, npm.cmd resolution, PATH case-collision) can land first and the bash → TS port can wait. Happy to split.


Builds on #282. With that merged, the release pipeline knows how to publish a Windows binary, but the local build and packaging scripts still assume bash. They fall over before reaching the new code path. After this PR, bun run build:prebuilt:npm, bun run check:prebuilt-pack, and bun run smoke:prebuilt-install all complete on native Windows 11 x64.

Changes

feat(scripts): port build/install bash scripts to TypeScript

  • scripts/build-bin.shscripts/build-bin.ts. Writes dist/hunk on Unix and dist/hunk.exe on Windows, matching what bun build --compile actually emits.
  • scripts/build-npm.shscripts/build-npm.ts. Skips chmod 0755 on Windows and replaces the shell glob with readdirSync.
  • scripts/install-bin.shscripts/install-bin.ts. Defaults to %LOCALAPPDATA%\Programs\hunk on Windows and ~/.local/bin on Unix, still honors HUNK_INSTALL_DIR, and case-folds before checking PATH on Windows.
  • package.json invokes bun run ./scripts/*.ts instead of bash ./scripts/*.sh.

fix(scripts): make repo scripts work on native Windows

A new scripts/script-helpers.ts holds two helpers, both threaded through the existing scripts:

  • npmCommand resolves to npm.cmd on Windows. Bun.spawn can't exec the Unix-style npm shim that Node ships next to npm.cmd (same applies to Node child_process without shell: true).
  • envWithPath(path) strips every case-variant of PATH/Path/etc. before setting PATH. The reason: on Windows, { ...process.env, PATH: sanitized } leaves the original Path key in the object, the child inherits both, and binaries resolve from the un-sanitized union. The smoke test's bun unexpectedly available guard was silently passing the wrong PATH because of this.

Three smaller fixes:

  • stage-prebuilt-npm.ts hardcoded dist/hunk for the host-only artifact. Now calls binaryFilenameForSpec(getHostPlatformPackageSpec()).
  • smoke-prebuilt-install.ts no longer requires bash on PATH on Windows. The npm-installed hunk.cmd shim doesn't shell out via bash.
  • check-pack.ts, check-prebuilt-pack.ts, and publish-prebuilt-npm.ts all use npmCommand.

docs: declare native Windows support

  • README requirements: Windows listed alongside macOS and Linux.
  • CONTRIBUTING dev-setup: notes that native Windows works without WSL or Git Bash.
  • CHANGELOG [Unreleased]: records the port and the docs alongside the existing PR feat(release): publish Windows prebuilt artifacts #282 entry.

Verification (Windows 11 x64, Bun 1.3.11, Node 24.14.0)

Command Before After
bun run build:bin bash: not found ✅ builds dist/hunk.exe
bun run build:npm bash: not found ✅ builds dist/npm/main.js + opentui types
bun run check:pack spawn npm ENOENT Verified npm pack output for hunkdiff@…
bun run build:prebuilt:npm Missing compiled binary at dist/hunk ✅ stages hunkdiff + hunkdiff-windows-x64
bun run check:prebuilt-pack spawn npm ENOENT Verified prebuilt npm packages
bun run smoke:prebuilt-install bash: not found, or bun unexpectedly available once bash is on PATH Verified prebuilt npm install smoke test with hunkdiff-windows-x64
bun run install:bin with HUNK_INSTALL_DIR=<tmp> Installed …\hunk.exe
bun run typecheck and bun run lint

bun run format:check flags the same pre-existing CLAUDE.md issue on main; this PR doesn't touch it.

Out of scope

  • Shipping hunkdiff-windows-x64 to npm. That's a tag, not a code change. The release workflow handles it on the next tag.
  • The 5 jj test failures locally. They need Jujutsu on the host; CI installs jj-cli via taiki-e/install-action.

@Ofekw Ofekw marked this pull request as ready for review May 11, 2026 05:02
@Ofekw
Copy link
Copy Markdown
Contributor Author

Ofekw commented May 11, 2026

Just an example screenshot of this PR of hunk running locally on my windows 11 machine:
image

@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented May 11, 2026

Greptile Summary

This PR completes native Windows 11 support by porting three bash scripts (build-bin, build-npm, install-bin) to cross-platform TypeScript and fixing a handful of Windows-specific bugs in the existing scripts.

  • Bash → TypeScript port: build-bin.ts, build-npm.ts, and install-bin.ts replace their .sh counterparts. Each script handles platform differences explicitly: .exe suffix on Windows, chmod guarded behind a non-Windows check, and a %LOCALAPPDATA%\Programs\hunk default install path.
  • script-helpers.ts: Introduces npmCommand (npm.cmd on Windows) and envWithPath, which strips all case-variants of the PATH key before setting it — fixing a subtle Windows env-inheritance bug where { ...process.env, PATH: x } silently inherits the original Path key alongside the new PATH key.
  • stage-prebuilt-npm.ts / smoke-prebuilt-install.ts: Fix hardcoded dist/hunk binary lookup and propagate npmCommand/envWithPath throughout.

Confidence Score: 4/5

Safe to merge. All changed paths are build/install scripts; no production runtime code is touched.

The Windows-specific logic is well-reasoned and the envWithPath helper correctly addresses the case-collision problem described in the PR. The two style observations (redundant if (!entry) guard and a second call to getHostPlatformPackageSpec() in the log path) are trivial and carry no correctness risk.

No files require special attention beyond the two minor style nits in scripts/install-bin.ts and scripts/stage-prebuilt-npm.ts.

Important Files Changed

Filename Overview
scripts/script-helpers.ts New shared helpers: npmCommand (resolves npm.cmd on Windows) and envWithPath (strips all case-variants of PATH before setting it, avoiding the Windows case-collision bug).
scripts/install-bin.ts Bash-to-TypeScript port of install-bin. Adds Windows default install dir, case-insensitive PATH check, and omits chmod on Windows. Minor: filter(Boolean) before the .some() callback already removes empty entries, making the if (!entry) return false guard redundant.
scripts/build-bin.ts Bash-to-TypeScript port. Correctly resolves hunk.exe on Windows vs hunk on Unix and cleans up the legacy otdiff/otdiff.exe binary.
scripts/build-npm.ts Bash-to-TypeScript port. Replaces shell glob with readdirSync for .d.ts copy and guards chmodSync behind a non-Windows check.
scripts/smoke-prebuilt-install.ts Updated to use npmCommand, envWithPath, and conditionally omit bash from the sanitized PATH on Windows. commandPath uses where on Windows, bash -lc on Unix.
scripts/stage-prebuilt-npm.ts Fixed hardcoded dist/hunk lookup to use binaryFilenameForSpec, but the log message at the end re-invokes getHostPlatformPackageSpec() when the IIFE's hostSpec is out of scope.
package.json Three script entries updated from bash ./scripts/*.sh to bun run ./scripts/*.ts; no other changes.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A["bun run build:bin\nbuild-bin.ts"] --> B{"process.platform"}
    B -- win32 --> C["dist/hunk.exe"]
    B -- unix --> D["dist/hunk"]

    E["bun run build:npm\nbuild-npm.ts"] --> F["dist/npm/main.js"]
    E --> G["dist/npm/opentui/index.js + *.d.ts"]
    F -- "non-Windows only" --> H["chmodSync 0755"]

    I["bun run install:bin\ninstall-bin.ts"] --> A
    I --> J{"HUNK_INSTALL_DIR?"}
    J -- set --> K["custom dir"]
    J -- unset + win32 --> L["%LOCALAPPDATA%\\Programs\\hunk"]
    J -- unset + unix --> M["~/.local/bin"]
    K & L & M --> N["copyFileSync binary to installPath"]
    N --> O["PATH check\ncase-insensitive on win32"]

    P["bun run build:prebuilt:npm\nstage-prebuilt-npm.ts"] --> E
    P --> A
    P --> Q["binaryFilenameForSpec(hostSpec)\ndist/hunk OR dist/hunk.exe"]

    R["smoke-prebuilt-install.ts"] --> S["npmCommand pack/install"]
    S --> T["envWithPath\nstrips PATH case-variants"]
    T --> U["run hunk --help / --version"]
Loading
Prompt To Fix All With AI
Fix the following 2 code review issues. Work through them one at a time, proposing concise fixes.

---

### Issue 1 of 2
scripts/install-bin.ts:48-51
The `.filter(Boolean)` on line 48 already removes all falsy entries (including empty strings) from `pathEntries`, so the `if (!entry) return false` guard inside the `.some()` callback is unreachable. The inner check can be dropped.

```suggestion
const pathEntries = (process.env.PATH ?? "").split(path.delimiter).filter(Boolean);
const installDirOnPath = pathEntries.some((entry) => {
  // Windows paths are case-insensitive; normalize both sides for the comparison.
```

### Issue 2 of 2
scripts/stage-prebuilt-npm.ts:174-184
`getHostPlatformPackageSpec()` is called twice in the non-`artifactRoot` path: once inside the IIFE (line 177) and again in the closing log (line 202). The IIFE's `hostSpec` is out of scope by then, so the function is invoked a second time. Hoisting `hostSpec` eliminates the duplicate call and removes the need for the IIFE pattern entirely.

```suggestion
const hostSpec = artifactRoot ? undefined : getHostPlatformPackageSpec();
const artifacts = artifactRoot
  ? collectArtifactSpecs(artifactRoot)
  : [
      {
        spec: hostSpec!,
        compiledBinary: path.join(repoRoot, "dist", binaryFilenameForSpec(hostSpec!)),
      },
    ];
```

Reviews (1): Last reviewed commit: "docs: declare native Windows support" | Re-trigger Greptile

Comment thread scripts/install-bin.ts
Comment thread scripts/stage-prebuilt-npm.ts Outdated
@Ofekw Ofekw mentioned this pull request May 11, 2026
Copy link
Copy Markdown
Member

@benvinegar benvinegar left a comment

Choose a reason for hiding this comment

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

I reviewed the Windows script-port changes and ran the relevant Linux-side checks locally:

  • bun run typecheck
  • bun run lint
  • bun test scripts/prebuilt-package-helpers.test.ts
  • bun run check:pack
  • bun run build:prebuilt:npm
  • bun run check:prebuilt-pack
  • bun run smoke:prebuilt-install
  • bun run build:bin
  • bun run install:bin

The script changes look reasonable to me. The one merge blocker I see is that this branch is currently conflicting with main, and only the Socket checks are showing on the PR. Please update/rebase it so the full CI matrix runs before merge.

When resolving, make sure to preserve the recent main changes from the Nix follow-up: the Unreleased Nix changelog entry, the CONTRIBUTING dependency-lock section, package.json's nix:update-lock value, and the pinned @types/bun version.

This comment was generated by Pi using GPT-5

Ofekw and others added 4 commits May 11, 2026 15:28
Replace �ash ./scripts/{build-bin,build-npm,install-bin}.sh with
Bun-runnable TypeScript so native Windows contributors no longer need
Git Bash or WSL to build or install Hunk locally.

The new scripts:
- scripts/build-bin.ts writes dist/hunk (POSIX) or dist/hunk.exe
  (Windows), matching what �un build --compile actually emits.
- scripts/build-npm.ts skips the Unix-only chmod 0755 on Windows,
  iterates the type-declaration files instead of relying on a shell
  glob, and otherwise preserves the original build flags.
- scripts/install-bin.ts defaults to %LOCALAPPDATA%\\Programs\\hunk
  on Windows and ~/.local/bin on Unix, still honours
  HUNK_INSTALL_DIR, and uses path.delimiter plus a
  case-insensitive comparison when checking PATH.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Adds a small scripts/script-helpers.ts module and wires it into the
existing TS scripts to address four native-Windows quirks:

- npm spawning: Bun.spawn cannot exec the Unix-style \
pm\ shell shim
  that Node ships alongside \
pm.cmd\. The new \
pmCommand\ constant
  resolves to \
pm.cmd\ on Windows and \
pm\ elsewhere, and is used
  from check-pack, check-prebuilt-pack, publish-prebuilt-npm, and the
  prebuilt-install smoke test.

- PATH case-collision: \{ ...process.env, PATH: sanitized }\ leaves
  the inherited \Path\ key in place on Windows, so the child sees
  both the original and the sanitized PATH and resolves binaries via
  the unsanitized union. The new \�nvWithPath\ helper strips every
  case-variant before setting PATH, restoring the smoke test's
  \�un unexpectedly available\ guarantee on Windows.

- Host artifact path: \stage-prebuilt-npm.ts\ hardcoded
  \dist/hunk\, but \�un build --compile\ emits \dist/hunk.exe\ on
  Windows. Use \�inaryFilenameForSpec(getHostPlatformPackageSpec())\
  so the host-only staging path matches the on-disk filename.

- bash on PATH: the prebuilt-install smoke test required \�ash\ on
  PATH to build a sanitized PATH for the npm-installed wrapper. The
  Windows \hunk.cmd\ shim does not need bash, so \�ashDir\ is now
  optional on Windows and filtered out of the sanitized PATH.

With these fixes \�un run build:prebuilt:npm\,
\�un run check:prebuilt-pack\, and \�un run smoke:prebuilt-install\
all complete successfully on native Windows 11 x64.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- README: list Windows alongside macOS and Linux under Requirements.
- CONTRIBUTING: add Windows to the development-setup platforms and
  note that the native Windows path uses Node/Bun directly (no WSL or
  Git Bash needed) now that the build/install scripts are TypeScript.
- CHANGELOG: record the build-script port and Windows documentation
  under [Unreleased]; the entries about the prebuilt Windows artifact
  pipeline (PR modem-dev#282) remain.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- install-bin.ts: drop the unreachable \if (!entry) return false\ guard
  inside the PATH membership check. \.filter(Boolean)\ on the line
  above already strips every falsy entry.

- stage-prebuilt-npm.ts: hoist \hostSpec\ out of the IIFE so the
  trailing \Artifacts source:\ log can reuse it instead of calling
  \getHostPlatformPackageSpec()\ a second time. Removes the IIFE
  pattern entirely.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@benvinegar
Copy link
Copy Markdown
Member

Eh I'll rebase it's just CHANGELOG.md

@benvinegar benvinegar force-pushed the feat/windows-support branch from 4a565bb to a38e516 Compare May 11, 2026 19:28
@benvinegar
Copy link
Copy Markdown
Member

Rebased this onto current main and resolved the small conflicts. Preserved the recent Nix follow-up changes while keeping the Windows script-port changes.

Local validation after the rebase:

  • bun run format:check
  • bun run typecheck
  • bun run lint
  • bun test scripts/prebuilt-package-helpers.test.ts
  • bun run check:pack
  • bun run build:prebuilt:npm
  • bun run check:prebuilt-pack
  • bun run smoke:prebuilt-install

This comment was generated by Pi using GPT-5

@benvinegar benvinegar merged commit f8bae97 into modem-dev:main May 11, 2026
5 checks passed
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