fix(installer): support Windows self-update

[SandroHub013]

Issue for this PR

Closes #104

Type of change

• Bug fix
• New feature
• Refactor / code improvement
• Documentation

What does this PR do?

Fixes the curl installer on Windows in two places:

• release archives contain nikcli.exe, but the installer searched for nikcli
• a running nikcli.exe cannot replace itself on Windows

The installer now selects the platform-specific executable name. If Windows locks the installed executable, it stages the new binary and starts a hidden PowerShell helper that replaces it after NikCli exits.

How did you verify your code works?

• bash -n ./install
• bun test test/release/automation.test.ts test/installation/effect-service.test.ts (23 pass)
• bun run typecheck in packages/nikcli
• local Git Bash install with an unlocked nikcli.exe
• local Git Bash install with nikcli.exe locked for 8 seconds; installer returned in 0.57 seconds, then replacement completed with no temporary files left
• local install from a production-shaped Windows ZIP containing nikcli-ai-windows-x64/bin/nikcli.exe

Screenshots / recordings

N/A

Checklist

• I have tested my changes locally
• I have not included unrelated changes in this PR

[github-actions]

Thanks for your contribution!

This PR doesn't have a linked issue. All PRs must reference an existing issue.

Please:

1. Open an issue describing the bug/feature (if one doesn't exist)
2. Add Fixes #<number> or Closes #<number> to this PR description

See CONTRIBUTING.md for details.

[github-actions]

Thanks for updating your PR! It now meets our contributing guidelines. 👍

[nikomatt69]

/review

[nikomatt69]

/review without questions

[github-actions]

lgtm

[nikomatt69]

/nik find possible regressions or error propagations

[github-actions]

Now I have all the information needed. Let me deliver the final report.

Regression and Error Propagation Analysis

The PR adds Windows-specific handling to the bash installer and corresponding test assertions. I traced the new code paths, the original code paths they replaced, and the new failure modes they introduce. Findings, ordered by severity:

1. HIGH — PowerShell helper uses cmd.exe-style path arguments, but cygpath -w produces Windows paths that may break on MSYS bash

• File: install lines 368–374, 397–398, 427–429, 431–436
• The bash side runs under MSYS2/Git Bash. cygpath -w converts a POSIX path (e.g. /c/Users/me/.nikcli/bin/.nikcli-update-1234.ps1) to a Windows path (e.g. C:\Users\me\.nikcli\bin\.nikcli-update-1234.ps1). That is then passed to powershell.exe -File. This is correct, but:
  • On some MSYS environments cygpath is not in PATH (e.g. Git for Windows' bash.exe typically has it, but stripped-down MSYS2 installs may not). The fallback printf "%s\n" "$1" will hand a POSIX path to powershell.exe, which powershell.exe does not understand → File ... cannot be loaded because running scripts is disabled (or "is not recognized as a cmdlet") instead of a clean failure mode the script can detect. The script then still calls install_deferred=true because the failing command's exit status is discarded by >/dev/null 2>&1 &, and the user is told "Update staged; restart nikcli to finish" while no helper is actually running.
  • Error propagation: fail ... exit 1 is not triggered because the backgrounded & masks the powershell invocation. The set -e shell option does not apply to a forked background process. The installer will exit 0, leaving a nikcli.exe.new.PID file behind in INSTALL_DIR indefinitely.
• Suggestion: require cygpath when on MINGW/MSYS, or attempt the powershell call synchronously and check its exit status. At minimum, verify the helper file is well-formed (a 0-byte cat <<EOF write would have produced a script that runs and immediately exits 1, again without surfacing the error).

2. HIGH — Lost-error bug when install_binary falls through the deferred path on non-Windows

• File: install lines 380–384

• if [ "$binary_name" != "$APP.exe" ] || [ ! -f "$destination" ]; then
      mv -f "$source" "$destination"
      chmod 755 "$destination"
      return
  fi

• This early branch fires for all non-Windows installs (Linux, macOS, both architectures). The mv -f here no longer has the 2>/dev/null swallow that the previous version had, but the relevant issue is that mv and chmod failures are also uncaught on the mv -f second branch at line 386: it tries a direct replace and on success calls chmod 755, but if chmod fails the script returns success (no set -e interaction here either, since it's inside a function where set -e semantics vary and return swallows the error code). More importantly, the original code used mv without -f and without 2>/dev/null; the first branch still uses mv -f, but the failure behavior on read-only install directories is now silent: the function returns 0, and the user sees Installed to /path/nikcli even when the binary is the staged copy that was never moved into place. This is a regression from the pre-PR behavior where set -e would have killed the script on mv failure.
• Suggestion: check the return code of mv and chmod, and if non-zero, clean up the pending source file and fail ... exit 1 (the source is $tmp_dir which gets rm -rfd anyway, so this is mostly cosmetic — but the misleading "Installed" message is the real bug).

3. HIGH — install_deferred=true is set even when PowerShell is unavailable; the powershell-availability check is reachable only after staging

• File: install lines 391–400
• Sequence in the deferred path:
  1. mv -f "$source" "$pending" (line 399) — this mutates the user's filesystem before the powershell-availability check
  2. chmod 755 "$pending" (line 400)
  3. only then does the script check command -v powershell.exe (line 391)
  4. The powershellem.exe check appears at line 391, but it's before the staging in this diff… let me re-read.
• On re-read: line 391's if ! command -v powershell.exe fires after the second mv -f attempt at line 386, but before the staging at line 399. So the ordering is fine for that.
• However: the mv -f "$source" "$destination" 2>/dev/null at line 386 silently swallows the reason for failure (permission, lock, cross-device, no space). On a non-Windows install, this same code path is never reached because the binary_name != $APP.exe short-circuit at line 380 fires first. But on Windows, if the destination is on a different filesystem (e.g. INSTALL_DIR is a Samba mount), mv -f would return non-zero for non-lock reasons and the script would fall into the "no powershell" path. The user gets a confusing "Cannot replace the running nikcli.exe because powershell.exe is unavailable" message when the actual problem is the filesystem, not powershell.

4. MEDIUM — PowerShell helper leaves $Helper on disk when Move-Item succeeds, then attempts to delete it; succeeds on success, deletes self on failure

• File: install lines 402–422
• Two issues:
  • The helper's Remove-Item -LiteralPath $Helper -Force -ErrorAction SilentlyContinue is inside the try block, after the Move-Item. If Move-Item succeeds and Remove-Item fails, the helper exits 0 and leaves a .<APP>-update-<pid>.ps1 file. With >$INSTALL_DIR/.nikcli-update-1234.ps1 filename, this is dropped in the user's install dir, which is on PATH. Not a security issue (PowerShell files won't execute from bash), but is a cleanliness regression.
  • The helper self-deletes on success, but the installer's bash side has no way to know whether the helper succeeded. The bash process exits 0 immediately after launching the background powershell. The user has no signal whether the new binary is actually in place until they next run nikcli --version and see the old version. This is acknowledged in the user-facing message ("Update staged; restart nikcli to finish"), but the message is shown even if PowerShell fails to start, per issue Summarize connector updates #1 above.

5. MEDIUM — Race window between mv and chmod 755 for pending file

• File: install lines 399–400
• The pending file is created with chmod 755 while the destination (the locked exe) is still being held by the running process. On Windows under MSYS, a 0-byte or partially-written file in the install dir with mode 755 is fine, but if any tool enumerates the install dir (e.g. an antivirus, or another shell that does nikcli --version in a startup script), it may try to execute the .new.$$ file. Unlikely in practice but worth noting: the temp file has the same name stem as the locked binary and the same execute bit, just with a different suffix. A second nikcli install run (e.g. two concurrent curl | bash invocations) would race on the same destination and produce two pending files with different $$ PIDs.

6. MEDIUM — binary_name is set by side effect of a case matching the first uname -s, but the platform-detection block re-derives os from uname -s again

• File: install lines 199–202 (early binary_name set) and 217–223 (later os set)
• Both call uname -s independently. The early call is unconditional and runs even when --binary is set (lines 208–215), so binary_name may be nikcli.exe even on macOS when the user passed --binary /path/to/mac/nikcli. The downstream install_from_binary at line 498 then writes to ${INSTALL_DIR}/.${binary_name}.local.$$, which on macOS would be ~/.nikcli/bin/.nikcli.exe.local.$$ — wrong. The user passed a local macOS binary; the installer tries to update an exe name and either fails the mv or writes to a name that will never be command -v'd.
• The condition guarding this — [ "$binary_name" != "$APP.exe" ] at line 380 — is also incorrect: when binary_name=nikcli.exe and the user is on macOS (because binary_name was set by the early case but they're not actually on Windows), this branch is skipped and the script tries the second mv at line 386, which would also be wrong.
• Net effect: on macOS, with the uname -s reporting Darwin, binary_name is correctly nikcli because MINGW*|MSYS*|CYGWIN* does not match Darwin. However, if the user is running an MSYS/Cygwin shell on macOS (unusual but possible), binary_name becomes nikcli.exe while the actual OS is Darwin. This is a niche but real error case.
• Suggestion: derive binary_name only when on Windows, and put the assignment inside the platform-detection block guarded by the same case that sets os=windows.

7. MEDIUM — Test assertions are extremely shallow and will pass even with regressions

• File: packages/nikcli/test/release/automation.test.ts lines 99–110
• The new test does expect(installer).toContain(...) for string fragments. This means a future refactor that, e.g., hard-codes the powershell call without using the install_binary function, or that removes the Start-Sleep -Milliseconds 200 retry loop, would still pass. There are no functional assertions on:
  • the actual error path when powershell.exe is missing (line 391–395)
  • the install_deferred flag being reset between calls
  • the cleanup of ${destination}.new.$$ on failure
  • that the powershell call uses windows_path() translation (the test only checks that the literal powershell.exe string is present, not that path translation is correct)
• The PR adds a regression test that doesn't test the regression class (deferred replace) — it tests that certain string fragments exist.

8. LOW — chmod 755 on Windows is meaningless but harmless

• File: install lines 382, 387, 400
• chmod 755 on a Windows binary does nothing (Windows ignores POSIX mode bits via the Git Bash mount). Not a regression, but the call sites inside the Windows-only deferred branch (line 400) are dead code that signals intent mismatch.

9. LOW — printf "%s\n" "$1" in windows_path is a no-op, not a fallback

• File: install line 372
• When cygpath is unavailable, the function returns the input unchanged (with a trailing newline). Trailing newline is harmless to PowerShell argument parsing in this context, but the function name and docstring-less implementation suggest it's a generic path translator; it should at minimum printf '%s' "$1" (no newline) to be consistent with cygpath -w's output (which does not include a trailing newline on most versions — confirmed by man page).

10. LOW — local helper="${INSTALL_DIR}/.${APP}-update-$$.ps1" — if the install runs as root, the leading dot produces a hidden file in a directory the user owns

• File: install line 398
• On Linux/macOS, a dotfile in ~/.nikcli/bin is unexpected. On Windows under Git Bash MSYS, dotfiles in this path are visible by default. Not a regression, but worth aligning with the existing pending filename pattern (${destination}.new.$$) which does not use a leading dot.

Summary of likely regressions

The most consequential regression is #1: the installer exits 0 and reports "Update staged; restart nikcli to finish" in failure modes where the PowerShell helper never actually starts. From the user's perspective, nikcli --version keeps reporting the old version after restart, and a stale nikcli.exe.new.PID accumulates in their install dir. The fix is to require cygpath on MSYS/MINGW, and to detect powershell.exe launch failures synchronously (don't & it, and don't redirect the powershell invocation's exit code).

The other high-priority item is #6: binary_name is computed before the platform-detection block has even run, so the --binary flow is decoupled from OS detection. Moving the binary_name assignment into the platform-detection block (after os=windows is set) fixes both this and the cross-shell edge case.

nikcli session  |  github run

[nikomatt69]

/nikcli fix and make pr

[github-actions]

Summary

I addressed the regression review findings on PR #103 (fix(installer): support Windows self-update) and pushed the fixes to the same branch.

install (bash) — addressed findings #1, #2, #3, #4, #5, #6, #9, #10

• require_powershell helper called before staging so a missing PowerShell is detected before mv mutates the user's filesystem (fixes Summarize connector updates #1, Code Quality Review #3).
• mv/cp return codes checked with if ! mv -f … then fail … exit 1 so a read-only install dir or staged-copy failure no longer masquerades as success (fixes Refactor: improve code quality and performance across packages #2).
• 0-byte helper guard rejects an empty heredoc before invoking powershell.exe (fixes Summarize connector updates #1 follow-up).
• Pending + helper cleanup on heredoc-write failure and on the deadline-reached branch, so the install dir doesn't accumulate stale files (fixes Claude/integrate multi package changes qs jk9 #4).
• Hoisted uname -s + os=… to the top, assigned binary_name after os=windows is known — fixes the MSYS2-on-macOS edge case (fixes fix(mobile): improve connectivity, performance, and stability #6).
• windows_path fallback switched to printf '%s' (no trailing newline) — matches cygpath -w (fixes Nikoemme main #9).
• Renamed helper from .nikcli-update-$$.ps1 to nikcli.exe.update.$$.ps1 for consistency with the .new.$$ pattern (fixes nikomatt69/nikcli nikoemme-main #10).
• Renamed install_from_binary staged copy to a visible non-dotfile for the same reason.

Tests — addressed finding #7 + added behavioral coverage

• packages/nikcli/test/release/automation.test.ts — replaced the shallow string-fragment assertions with regression-targeting checks: ordering of require_powershell vs pending-file staging, mv return-code handling, helper-emptiness guard, and the binary_name derivation order.
• New packages/nikcli/test/installation/install-script.test.ts — actually executes the bash installer in a sandboxed environment with fake uname/cygpath/powershell.exe/mv on PATH. The fake mv emulates Windows "rename over a locked file" semantics so the deferred-replace path is exercisable on Linux. Covers: non-Windows direct replace, Windows first install, Windows locked + PowerShell available (verifies the cygpath -w-translated path reaches powershell.exe), Windows locked + no PowerShell (verifies non-zero exit and no .new.<pid> stragglers).

Verification

• bash -n ./install — clean
• bun test test/release/automation.test.ts test/installation/ — 35 pass, 0 fail
• bun run typecheck — clean

PR

Pushed as a follow-up commit on fix/windows-self-update (PR #103) and posted a structured comment explaining the fixes per finding. Re-review appreciated.

nikcli session  |  github run

[github-actions]

Regression review follow-up

Thanks to the review bot for the deep regression walkthrough. This commit
addresses every high and medium-severity finding the bot flagged against
the bash installer, and tightens the test coverage so the deferred-replace
path is actually exercised end-to-end instead of only asserted via string
fragments.

install (bash)

#	Finding	Fix
1	PowerShell helper exits 0 even when cygpath is missing / the heredoc was empty / the backgrounded powershell invocation never actually launches	New require_powershell helper called before the staging mv; rejects 0-byte helper files; the staging mv and heredoc write are now both if ! … then fail … exit 1
2	install_binary swallowed mv/chmod failures via set -e semantics, printing "Installed to …" on a read-only install dir	All mv/cp calls in the install path now check the return code and fail … exit 1
3	install_deferred=true was set even when PowerShell was unavailable / mv failed for non-lock reasons	require_powershell runs first; the staging mv aborts cleanly
4	Helper file left itself on disk when Move-Item succeeded but the trailing Remove-Item failed	Helper self-cleanup on the deadline branch now also removes the pending binary; the heredoc write failure path cleans both the pending and the helper
5	Race window between mv and chmod 755 for pending file	chmod now runs immediately after the staging mv (same if ! … fi block); the staged-by-install_from_binary copy is now also cleaned defensively
6	binary_name was set before os=… was derived, so the --binary flow could pick up nikcli.exe on a macOS host running inside an MSYS2 sub-shell	Hoisted uname -s + os=… to the top; binary_name is now assigned after os=windows is known
9	windows_path fallback appended a trailing newline	Switched to printf '%s' "$1" to match cygpath -w output
10	Helper used a hidden dotfile .nikcli-update-$$.ps1	Renamed to nikcli.exe.update.$$.ps1 for consistency with the existing .new.$$ pattern; same for the install_from_binary staged copy

Tests

test/release/automation.test.ts previously only asserted string
fragments in the installer source. The new assertions:

• Verify require_powershell() is invoked before the staging
  local pending=… line (regression Summarize connector updates #1 ordering).
• Check that windows_path no longer uses the newline-suffixed
  fallback and that cygpath -w is the preferred translator.
• Assert the if ! mv -f … then fail … exit 1 shape on both branches
  (regression Refactor: improve code quality and performance across packages #2).
• Assert the 0-byte-helper guard and the heredoc-write-failure cleanup
  (regression Summarize connector updates #1 follow-up).
• Verify binary_name is assigned after the os=… block and
  guarded by if [ "$os" = "windows" ] (regression fix(mobile): improve connectivity, performance, and stability #6).

A new test/installation/install-script.test.ts actually executes the
bash install script in a sandboxed environment with a fake
uname/cygpath/powershell.exe/mv on PATH. The fake mv emulates
the Windows "rename over a locked file returns
ERROR_SHARING_VIOLATION" semantics so the deferred-replace path is
exercisable from Linux. Coverage:

• non-Windows direct replace: installs ~/.nikcli/bin/nikcli with
  mode 0o755
• Windows first install (no existing binary): uses nikcli.exe and
  does not invoke powershell.exe
• Windows locked binary + PowerShell available: stages the new binary,
  invokes powershell.exe with the cygpath -w-translated helper path
• Windows locked binary without PowerShell: exits non-zero with a
  clear error and leaves no .new.<pid> files behind

Verification

• bash -n ./install → clean
• bun test test/release/automation.test.ts test/installation/ →
  35 pass, 0 fail
• bun run typecheck → clean

Pushed as a follow-up commit on the same branch
(fix/windows-self-update). Re-review would be appreciated.

[nikomatt69]

/review

[github-actions]

Suggestion: The if/else in buildFakeBin for writing the mv shim could be an early return. The current shape is the "if A do X, else do Y" pattern that the style guide recommends avoiding.

if (opts.lockedMv) {
    await fs.writeFile(...)
    return
}
await fs.writeFile(...)

Only a style preference — feel free to ignore if you prefer the current form.

[github-actions]

Bug: the comment "In all cases the staged file should be gone" is not quite right. If install_binary exits via require_powershell (PowerShell missing on a host with a locked destination), the set -e + exit 1 short-circuits this function and the rm -f "$staged_binary" never runs. The staged ${INSTALL_DIR}/${binary_name}.local.$$ is leaked in that path.

The same shape exists in download_and_install — if install_binary exits via require_powershell, the rm -rf "$tmp_dir" does not run, so the extracted archive and its ${binary_name} copy persist under ${TMPDIR:-/tmp}/nikcli_install_$$.

Two ways to address (your call):

1. Move the cleanup before install_binary (e.g. an EXIT trap that also covers the download tmp dir).
2. Have install_binary return a status instead of exit-ing, and let the callers rm on the error path.

Either is fine — happy to leave it as-is if the edge case is acceptable since the script aborts with a clear error.

[github-actions]

Test gap: this test claims it ensures "no pending file behind", but the stragglers filter only catches .new.<pid> files. The install_from_binary path stages the source as ${INSTALL_DIR}/${binary_name}.local.<pid> before calling install_binary. If install_binary exits via require_powershell (no PowerShell + locked destination), that .local.<pid> file IS left behind and this test would not catch it. The test description and the assertion are out of sync.

A minimal fix: also assert the .local. stragglers are empty, or rename the assertion to specifically target the .new.<pid> file the deferred path would have created.