diff --git a/.claude/commands/ship.md b/.claude/commands/ship.md index 7fa1591..bf9530d 100644 --- a/.claude/commands/ship.md +++ b/.claude/commands/ship.md @@ -6,7 +6,7 @@ Perform a complete git workflow to ship the current changes in this repo. - **Branch convention:** `feat/`, `fix/`, `chore/` - **Target branch:** `main` -- **CI:** `build-linux.yml` runs automatically on PR open (tests + compile) +- **CI:** `build-linux.yml (Build CI)` runs automatically on PR open (tests + compile) - **Release:** done separately via `git tag` — do NOT tag here ## Steps @@ -43,7 +43,7 @@ Perform a complete git workflow to ship the current changes in this repo. - Otherwise: `gh pr create` with: - Title: same as commit subject - Body: what changed, why, and any manual verification steps - - Include note that `build-linux.yml` CI will run automatically + - Include note that `build-linux.yml (Build CI)` CI will run automatically - If $ARGUMENTS is provided, use it as extra context for the PR body ## Notes diff --git a/.claude/docs/versioning.md b/.claude/docs/versioning.md index 955a4fd..acf0337 100644 --- a/.claude/docs/versioning.md +++ b/.claude/docs/versioning.md @@ -14,7 +14,7 @@ Suffix is optional; use `-alpha` / `-beta` / `-rc` for pre-releases. ``` feat/ branch → PR opened - └─ build-linux.yml runs automatically (tests + compile check) + └─ build-linux.yml runs automatically (tests + Linux/Windows builds) → PR merged to main → git tag vX.Y.Z[-suffix] ← release trigger → git push origin vX.Y.Z[-suffix] @@ -41,7 +41,8 @@ Then go to **GitHub → Releases** and publish the draft once the workflow compl **`build-linux.yml`** (PR + push to main): - Runs frontend tests (Vitest) + Go tests -- Builds Linux binary (artifact only, not released) +- Builds Linux binary + Windows binary + NSIS installer (artifacts only, not released) +- PR artifacts auto-expire after 3 days **`release.yml`** (tag push): - Builds `KeyLint-vX.Y.Z-linux-amd64` and `KeyLint-vX.Y.Z-windows-amd64.exe` diff --git a/.github/workflows/build-linux.yml b/.github/workflows/build-linux.yml index 0d2dc4b..9f24e0c 100644 --- a/.github/workflows/build-linux.yml +++ b/.github/workflows/build-linux.yml @@ -1,4 +1,4 @@ -name: Build Linux +name: Build on: pull_request: @@ -6,7 +6,7 @@ on: branches: [main] jobs: - build: + test: runs-on: ubuntu-latest steps: - uses: actions/checkout@v4 @@ -50,6 +50,44 @@ jobs: - name: Run Go tests run: go test ./internal/... + build-linux: + needs: test + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + with: + fetch-depth: 0 + + - name: Set up Go + uses: actions/setup-go@v5 + with: + go-version: '1.26' + cache: true + + - name: Set up Node.js + uses: actions/setup-node@v4 + with: + node-version: '24' + cache: 'npm' + cache-dependency-path: frontend/package-lock.json + + - name: Install system deps + uses: awalsh128/cache-apt-pkgs-action@latest + with: + packages: pkg-config libgtk-3-dev libwebkit2gtk-4.1-dev gcc + version: 1.0 + + - name: Install Wails v3 CLI + run: go install github.com/wailsapp/wails/v3/cmd/wails3@v3.0.0-alpha.72 + + - name: Install frontend dependencies + working-directory: frontend + run: npm ci + + - name: Build frontend + working-directory: frontend + run: npm run build + - name: Build Linux binary run: | export PATH=$PATH:$HOME/go/bin @@ -61,3 +99,93 @@ jobs: with: name: KeyLint-linux-amd64 path: bin/KeyLint + retention-days: 3 + + build-windows: + needs: test + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + with: + fetch-depth: 0 + + - name: Set up Go + uses: actions/setup-go@v5 + with: + go-version: '1.26' + cache: true + + - name: Set up Node.js + uses: actions/setup-node@v4 + with: + node-version: '24' + cache: 'npm' + cache-dependency-path: frontend/package-lock.json + + - name: Install system deps + uses: awalsh128/cache-apt-pkgs-action@latest + with: + packages: pkg-config libgtk-3-dev libwebkit2gtk-4.1-dev gcc gcc-mingw-w64 + version: 1.0 + + - name: Install Wails v3 CLI + run: go install github.com/wailsapp/wails/v3/cmd/wails3@v3.0.0-alpha.72 + + - name: Install frontend dependencies + working-directory: frontend + run: npm ci + + - name: Build frontend + working-directory: frontend + run: npm run build + + - name: Generate Windows resource file + run: | + export PATH=$PATH:$HOME/go/bin + wails3 generate syso \ + -arch amd64 \ + -icon build/windows/icon.ico \ + -manifest build/windows/wails.exe.manifest \ + -info build/windows/info.json \ + -out wails_windows_amd64.syso + + - name: Build Windows binary + run: | + export PATH=$PATH:$HOME/go/bin + VERSION=$(git describe --tags --always --dirty 2>/dev/null || echo "dev") + GOOS=windows GOARCH=amd64 CGO_ENABLED=1 CC=x86_64-w64-mingw32-gcc \ + go build -tags production -trimpath \ + -ldflags="-w -s -H windowsgui -X main.AppVersion=${VERSION}" \ + -o bin/KeyLint.exe . + + - name: Install NSIS + uses: awalsh128/cache-apt-pkgs-action@latest + with: + packages: nsis + version: 1.0 + + - name: Download WebView2 bootstrapper + run: | + curl -sSL "https://go.microsoft.com/fwlink/p/?LinkId=2124703" \ + -o build/windows/nsis/MicrosoftEdgeWebview2Setup.exe + + - name: Build NSIS installer + run: | + VERSION=$(git describe --tags --always --dirty 2>/dev/null || echo "dev") + makensis -DARG_WAILS_AMD64_BINARY=../../../bin/KeyLint.exe \ + -DINFO_PRODUCTVERSION="${VERSION#v}" \ + build/windows/nsis/project.nsi + + - name: Upload Windows binary + uses: actions/upload-artifact@v4 + with: + name: KeyLint-windows-amd64 + path: bin/KeyLint.exe + retention-days: 3 + + - name: Upload Windows installer + uses: actions/upload-artifact@v4 + with: + name: KeyLint-windows-amd64-setup + path: bin/KeyLint-amd64-installer.exe + retention-days: 3 diff --git a/docs/superpowers/plans/2026-04-05-updater-platform-aware-install.md b/docs/superpowers/plans/2026-04-05-updater-platform-aware-install.md new file mode 100644 index 0000000..bfe7a6a --- /dev/null +++ b/docs/superpowers/plans/2026-04-05-updater-platform-aware-install.md @@ -0,0 +1,687 @@ +# Platform-Aware Updater Fix — Implementation Plan + +> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. + +**Goal:** Fix GH #18 — the "Download & Install" button fails on Windows because `selfupdate.Apply()` cannot write to `C:\Program Files\` and the downloaded asset is an NSIS installer, not a raw binary. + +**Architecture:** Split `DownloadAndInstall()` into a shared download-to-temp phase and platform-specific install phase. Windows launches the NSIS installer and quits the app. Linux applies the raw binary via `selfupdate.Apply()`. A `quitFunc` callback lets the service trigger app shutdown without importing Wails directly. + +**Tech Stack:** Go 1.26, minio/selfupdate v0.6.0, Wails v3, Angular v21, Vitest + +**Spec:** `docs/superpowers/specs/2026-04-05-updater-platform-aware-install-design.md` + +--- + +## File Structure + +| File | Purpose | +|------|---------| +| `internal/features/updater/model.go` | Add `InstallResult` type | +| `internal/features/updater/service.go` | Refactor `DownloadAndInstall()` — download to temp, delegate to `applyPlatformUpdate()`, add `quitFunc` | +| `internal/features/updater/install_windows.go` | `applyPlatformUpdate()`: launch NSIS installer via `exec.Command` | +| `internal/features/updater/install_linux.go` | `applyPlatformUpdate()`: `selfupdate.Apply()` from temp file | +| `internal/features/updater/install_test.go` | Regression test + all new behavior tests | +| `main.go` | Set `quitFunc` on updater service after Wails app is created | +| `frontend/src/app/core/wails.service.ts` | Update `downloadAndInstall()` return type to `Promise` | +| `frontend/src/testing/wails-mock.ts` | Update mock to return `InstallResult` | +| `frontend/src/app/features/settings/settings.component.ts` | Handle `restart_required` in UI | +| `frontend/src/app/features/settings/settings.component.spec.ts` | Test restart-required UI path | + +--- + +### Task 1: Regression Test — Prove the Bug + +**Files:** +- Create: `internal/features/updater/install_test.go` + +This test documents WHY the old `selfupdate.Apply()` approach was replaced: it fails with a permission error when the executable lives in a read-only directory (like `C:\Program Files\`). + +- [ ] **Step 1: Write the regression test** + +Create `internal/features/updater/install_test.go`: + +```go +package updater + +import ( + "os" + "path/filepath" + "strings" + "testing" + + "github.com/minio/selfupdate" +) + +// TestSelfupdateFailsOnReadOnlyDirectory documents the root cause of GH #18: +// selfupdate.Apply() cannot write to directories that require elevated permissions +// (e.g. C:\Program Files\ on Windows). This test ensures we never regress to the +// old in-place replacement approach for directories that aren't user-writable. +func TestSelfupdateFailsOnReadOnlyDirectory(t *testing.T) { + dir := t.TempDir() + fakeBin := filepath.Join(dir, "KeyLint.exe") + if err := os.WriteFile(fakeBin, []byte("old binary"), 0o755); err != nil { + t.Fatal(err) + } + // Make the directory read-only so selfupdate cannot create .KeyLint.exe.new + if err := os.Chmod(dir, 0o555); err != nil { + t.Fatal(err) + } + t.Cleanup(func() { os.Chmod(dir, 0o755) }) + + err := selfupdate.Apply(strings.NewReader("new binary"), selfupdate.Options{ + TargetPath: fakeBin, + }) + + if err == nil { + t.Fatal("expected permission error when applying update to read-only directory, got nil — " + + "this means the old selfupdate.Apply() approach would silently corrupt the install") + } + + // The error message varies by OS: "permission denied" (Linux) or "Access is denied" (Windows). + errMsg := strings.ToLower(err.Error()) + if !strings.Contains(errMsg, "permission denied") && !strings.Contains(errMsg, "access is denied") { + t.Errorf("expected permission-related error, got: %v", err) + } +} +``` + +- [ ] **Step 2: Run the test to confirm it passes (proving the bug exists)** + +Run: `go test ./internal/features/updater/ -run TestSelfupdateFailsOnReadOnlyDirectory -v` + +Expected: PASS — the test confirms that `selfupdate.Apply()` fails on a read-only directory, which is exactly the bug from GH #18. + +- [ ] **Step 3: Commit** + +```bash +git add internal/features/updater/install_test.go +git commit -m "test(updater): regression test proving GH #18 selfupdate permission failure" +``` + +--- + +### Task 2: Backend — Add InstallResult Type, applyFunc Hook, and Platform Files + +**Files:** +- Modify: `internal/features/updater/model.go` — add `InstallResult` +- Modify: `internal/features/updater/service.go` — add `quitFunc`, `SetQuitFunc()`, `applyFunc` test hook, refactor `DownloadAndInstall()` +- Create: `internal/features/updater/install_linux.go` — Linux `applyPlatformUpdate()` +- Create: `internal/features/updater/install_windows.go` — Windows `applyPlatformUpdate()` +- Modify: `internal/features/updater/install_test.go` — tests for new download + install flow + +**Design note:** `selfupdate.Apply()` with default options replaces `os.Executable()` — the test runner itself. To avoid corrupting the test binary, the Service has an `applyFunc` field that overrides the platform function in tests. This is only used in tests; production code always uses the real `applyPlatformUpdate`. + +- [ ] **Step 1: Write tests for the new download + install flow** + +Append to `internal/features/updater/install_test.go`: + +```go +import ( + "bytes" + "net/http" + "net/http/httptest" + "os" + // ... existing imports plus these +) + +// serveAsset creates an httptest server that serves the given payload for any request. +func serveAsset(payload []byte) *httptest.Server { + return httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Write(payload) + })) +} + +// makeReleaseWithURL creates a release whose asset URL points to a custom server. +func makeReleaseWithURL(tag string, assetName string, assetURL string) githubRelease { + return githubRelease{ + TagName: tag, + Name: tag, + Body: "Release " + tag, + Draft: false, + Prerelease: false, + Assets: []githubAsset{{ + Name: assetName, + BrowserDownloadURL: assetURL, + }}, + } +} + +func TestDownloadAndInstall_DownloadsToTempAndCallsApply(t *testing.T) { + payload := []byte("fake-installer-payload-12345") + assetSrv := serveAsset(payload) + defer assetSrv.Close() + + releases := []githubRelease{ + makeReleaseWithURL("v9.0.0", "KeyLint-v9.0.0-linux-amd64", assetSrv.URL+"/dl"), + } + releaseSrv := serveGitHubReleases(t, releases) + defer releaseSrv.Close() + + svc := newTestService("1.0.0", releaseSrv) + + // Override the platform apply function to capture and verify the temp file + // instead of calling selfupdate.Apply (which would corrupt the test binary). + var capturedPath string + var capturedContent []byte + svc.applyFunc = func(_ *Service, tmpPath string) (InstallResult, error) { + capturedPath = tmpPath + data, err := os.ReadFile(tmpPath) + if err != nil { + t.Fatalf("applyFunc: failed to read temp file: %v", err) + } + capturedContent = data + return InstallResult{RestartRequired: false}, nil + } + + result, err := svc.DownloadAndInstall() + if err != nil { + t.Fatalf("DownloadAndInstall() error: %v", err) + } + if result.RestartRequired { + t.Error("expected RestartRequired=false from test applyFunc") + } + if capturedPath == "" { + t.Fatal("applyFunc was never called — download phase may have failed") + } + if !bytes.Equal(capturedContent, payload) { + t.Errorf("temp file content = %q, want %q", capturedContent, payload) + } +} + +func TestDownloadAndInstall_ApplyFuncRestartRequired(t *testing.T) { + assetSrv := serveAsset([]byte("installer")) + defer assetSrv.Close() + + releases := []githubRelease{ + makeReleaseWithURL("v9.0.0", "KeyLint-v9.0.0-linux-amd64", assetSrv.URL+"/dl"), + } + releaseSrv := serveGitHubReleases(t, releases) + defer releaseSrv.Close() + + svc := newTestService("1.0.0", releaseSrv) + svc.applyFunc = func(_ *Service, _ string) (InstallResult, error) { + return InstallResult{RestartRequired: true}, nil + } + + result, err := svc.DownloadAndInstall() + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if !result.RestartRequired { + t.Error("expected RestartRequired=true (simulating Windows path)") + } +} + +func TestDownloadAndInstall_NoUpdateAvailable(t *testing.T) { + releases := []githubRelease{ + makeRelease("v1.0.0", false, false, "KeyLint-v1.0.0-linux-amd64"), + } + srv := serveGitHubReleases(t, releases) + defer srv.Close() + + svc := newTestService("1.0.0", srv) + _, err := svc.DownloadAndInstall() + if err == nil { + t.Fatal("expected error for no-update-available") + } + if !strings.Contains(err.Error(), "no update available") { + t.Errorf("unexpected error: %v", err) + } +} + +func TestDownloadAndInstall_DownloadHTTPError(t *testing.T) { + errorSrv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + http.Error(w, "not found", http.StatusNotFound) + })) + defer errorSrv.Close() + + releases := []githubRelease{ + makeReleaseWithURL("v9.0.0", "KeyLint-v9.0.0-linux-amd64", errorSrv.URL+"/missing"), + } + releaseSrv := serveGitHubReleases(t, releases) + defer releaseSrv.Close() + + svc := newTestService("1.0.0", releaseSrv) + _, err := svc.DownloadAndInstall() + if err == nil { + t.Fatal("expected error for HTTP 404 download") + } + if !strings.Contains(err.Error(), "404") { + t.Errorf("expected status code in error, got: %v", err) + } +} + +func TestDownloadAndInstall_EmptyBody(t *testing.T) { + emptySrv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + // 200 OK but empty body + })) + defer emptySrv.Close() + + releases := []githubRelease{ + makeReleaseWithURL("v9.0.0", "KeyLint-v9.0.0-linux-amd64", emptySrv.URL+"/empty"), + } + releaseSrv := serveGitHubReleases(t, releases) + defer releaseSrv.Close() + + svc := newTestService("1.0.0", releaseSrv) + _, err := svc.DownloadAndInstall() + if err == nil { + t.Fatal("expected error for empty download body") + } + if !strings.Contains(err.Error(), "empty") { + t.Errorf("expected 'empty' in error, got: %v", err) + } +} +``` + +- [ ] **Step 2: Run tests to confirm they fail (functions don't exist yet or signatures mismatch)** + +Run: `go test ./internal/features/updater/ -run "TestDownloadAndInstall" -v` + +Expected: FAIL — `DownloadAndInstall()` still returns `error` (not `InstallResult, error`), and `applyFunc` field doesn't exist. + +- [ ] **Step 3: Add InstallResult to model.go** + +Add to `internal/features/updater/model.go`, after the `UpdateInfo` struct: + +```go +// InstallResult is returned by DownloadAndInstall to indicate the outcome. +// On Windows, RestartRequired is true because the NSIS installer needs the app to exit. +type InstallResult struct { + RestartRequired bool `json:"restart_required"` +} +``` + +- [ ] **Step 4: Add quitFunc, applyFunc, and refactor DownloadAndInstall in service.go** + +In `internal/features/updater/service.go`, update the imports, Service struct, add `SetQuitFunc`, and replace `DownloadAndInstall`: + +```go +import ( + "errors" + // ... existing imports plus: + "io" + "os" +) + +type Service struct { + currentVersion string + releasesAPIURL string + client *http.Client + settingsSvc *settings.Service + quitFunc func() // called after launching installer on Windows; set via SetQuitFunc + applyFunc func(svc *Service, tmpPath string) (InstallResult, error) // override for testing; nil uses applyPlatformUpdate +} + +// SetQuitFunc sets the callback invoked after launching the installer on Windows. +// The callback should wait briefly (for the frontend to display a message) then quit the app. +func (s *Service) SetQuitFunc(fn func()) { + s.quitFunc = fn +} +``` + +Replace the `DownloadAndInstall` method entirely: + +```go +// DownloadAndInstall fetches the release asset for the current platform, saves it +// to a temp file, and delegates to the platform-specific installer. +// On Windows this launches the NSIS setup and returns InstallResult{RestartRequired: true}. +// On Linux this applies the binary in-place via selfupdate. +func (s *Service) DownloadAndInstall() (InstallResult, error) { + updateInfo, err := s.CheckForUpdate() + if err != nil { + return InstallResult{}, fmt.Errorf("checking for update: %w", err) + } + if !updateInfo.IsAvailable { + return InstallResult{}, fmt.Errorf("no update available") + } + if updateInfo.ReleaseURL == "" { + return InstallResult{}, fmt.Errorf("no download URL for current platform") + } + + resp, err := s.client.Get(updateInfo.ReleaseURL) + if err != nil { + return InstallResult{}, fmt.Errorf("downloading update: %w", err) + } + defer resp.Body.Close() + + if resp.StatusCode != http.StatusOK { + return InstallResult{}, fmt.Errorf("download returned status %d", resp.StatusCode) + } + + // Write to a temp file so platform-specific code can work with a path on disk. + tmpFile, err := os.CreateTemp("", "KeyLint-update-*.exe") + if err != nil { + return InstallResult{}, fmt.Errorf("creating temp file: %w", err) + } + + n, err := io.Copy(tmpFile, resp.Body) + if err != nil { + tmpFile.Close() + os.Remove(tmpFile.Name()) + return InstallResult{}, fmt.Errorf("writing update to temp file: %w", err) + } + tmpFile.Close() + + if n == 0 { + os.Remove(tmpFile.Name()) + return InstallResult{}, errors.New("downloaded update is empty") + } + + applyFn := applyPlatformUpdate + if s.applyFunc != nil { + applyFn = s.applyFunc + } + return applyFn(s, tmpFile.Name()) +} +``` + +Remove the old unused imports (`"github.com/minio/selfupdate"`) from `service.go` — selfupdate is now only used in `install_linux.go`. + +- [ ] **Step 5: Create the Linux install implementation** + +Create `internal/features/updater/install_linux.go`: + +```go +//go:build !windows + +package updater + +import ( + "fmt" + "os" + + "github.com/minio/selfupdate" +) + +// applyPlatformUpdate applies the downloaded binary in-place using selfupdate. +// On Linux the release asset is a raw executable, so direct replacement works +// as long as the target directory is writable. +func applyPlatformUpdate(svc *Service, tmpPath string) (InstallResult, error) { + f, err := os.Open(tmpPath) + if err != nil { + return InstallResult{}, fmt.Errorf("opening downloaded update: %w", err) + } + defer f.Close() + defer os.Remove(tmpPath) + + if err := selfupdate.Apply(f, selfupdate.Options{}); err != nil { + return InstallResult{}, fmt.Errorf("applying update: %w — if this is a permission error, "+ + "the downloaded file is at %s — install manually or run with appropriate permissions", + err, tmpPath) + } + + return InstallResult{RestartRequired: false}, nil +} +``` + +- [ ] **Step 6: Create the Windows install implementation** + +Create `internal/features/updater/install_windows.go`: + +```go +//go:build windows + +package updater + +import ( + "fmt" + "os" + "os/exec" +) + +// applyPlatformUpdate launches the downloaded NSIS installer and signals the app to quit. +// The NSIS installer's own manifest requests UAC elevation, so no special privileges +// are needed here — Windows will show the UAC prompt to the user. +func applyPlatformUpdate(svc *Service, tmpPath string) (InstallResult, error) { + cmd := exec.Command(tmpPath) + cmd.Stdout = os.Stdout + cmd.Stderr = os.Stderr + + if err := cmd.Start(); err != nil { + os.Remove(tmpPath) + return InstallResult{}, fmt.Errorf("launching installer: %w", err) + } + + // Schedule app quit so the installer can replace the locked executable. + // The quitFunc is set by main.go and includes a short delay for the + // frontend to display the "closing" message before the app exits. + if svc.quitFunc != nil { + go svc.quitFunc() + } + + return InstallResult{RestartRequired: true}, nil +} +``` + +- [ ] **Step 7: Run all updater tests** + +Run: `go test ./internal/features/updater/ -v` + +Expected: ALL PASS — regression test still passes (proving the old bug), new tests pass with the refactored code. + +- [ ] **Step 8: Commit** + +```bash +git add internal/features/updater/model.go internal/features/updater/service.go \ + internal/features/updater/install_linux.go internal/features/updater/install_windows.go \ + internal/features/updater/install_test.go +git commit -m "fix(updater): platform-aware install — launch NSIS on Windows, selfupdate on Linux + +Fixes #18. The old approach used selfupdate.Apply() which fails on Windows +because (a) the release asset is an NSIS installer, not a raw binary, and +(b) Program Files requires UAC elevation for writes." +``` + +--- + +### Task 3: Wire quitFunc in main.go + +**Files:** +- Modify: `main.go:88-89` — store updater service in a variable, call `SetQuitFunc` + +- [ ] **Step 1: Update main.go to set quitFunc on the updater service** + +In `main.go`, replace line 88–89: + +```go +// OLD: +wailsApp.RegisterService(application.NewService(updater.NewService(AppVersion, services.Settings))) + +// NEW: +updaterSvc := updater.NewService(AppVersion, services.Settings) +updaterSvc.SetQuitFunc(func() { + // Brief delay so the frontend can display the "closing" message. + time.Sleep(2 * time.Second) + wailsApp.Quit() +}) +wailsApp.RegisterService(application.NewService(updaterSvc)) +``` + +Add `"time"` to the imports in `main.go`. + +- [ ] **Step 2: Build to verify compilation** + +Run: `go build -o bin/KeyLint .` + +Expected: Compiles cleanly. + +- [ ] **Step 3: Regenerate Wails bindings (return type changed)** + +Run: `wails3 generate bindings` + +Note: This updates the auto-generated TypeScript bindings in `frontend/bindings/` to reflect the new `(InstallResult, error)` return type. + +- [ ] **Step 4: Commit** + +```bash +git add main.go frontend/bindings/ +git commit -m "fix(updater): wire quitFunc in main.go, regenerate bindings" +``` + +--- + +### Task 4: Frontend — Handle InstallResult + +**Files:** +- Modify: `frontend/src/app/core/wails.service.ts:163-165` — update return type +- Modify: `frontend/src/testing/wails-mock.ts:59` — update mock +- Modify: `frontend/src/app/features/settings/settings.component.ts:524-537,262-268` — handle `restart_required` + +- [ ] **Step 1: Write the frontend test for restart-required** + +Append to the `describe('About tab', ...)` block in `frontend/src/app/features/settings/settings.component.spec.ts`, after the existing `installUpdate()` test: + +```typescript + it('installUpdate() shows restart message when restart_required is true', async () => { + wailsMock.downloadAndInstall.mockResolvedValue({ restart_required: true }); + component.updateInfo = { ...defaultUpdateInfo, is_available: true, latest_version: '3.7.0' }; + await component.installUpdate(); + expect(component.updateSuccess).toBe(true); + expect(component.updateRestartRequired).toBe(true); + }); + + it('installUpdate() shows standard success when restart_required is false', async () => { + wailsMock.downloadAndInstall.mockResolvedValue({ restart_required: false }); + component.updateInfo = { ...defaultUpdateInfo, is_available: true, latest_version: '3.7.0' }; + await component.installUpdate(); + expect(component.updateSuccess).toBe(true); + expect(component.updateRestartRequired).toBe(false); + }); +``` + +- [ ] **Step 2: Run tests to confirm they fail** + +Run: `cd frontend && npm test` + +Expected: FAIL — `updateRestartRequired` property doesn't exist yet. + +- [ ] **Step 3: Update wails.service.ts** + +In `frontend/src/app/core/wails.service.ts`, change the `downloadAndInstall` method. First check the generated bindings to see the exact return type name, then update: + +```typescript +downloadAndInstall(): Promise { + return UpdaterService.DownloadAndInstall(); +} +``` + +Add the `InstallResult` type to the imports from the bindings (or define it in the `wails.service.ts` types section if that's where types live). The type shape is: + +```typescript +export interface InstallResult { + restart_required: boolean; +} +``` + +- [ ] **Step 4: Update wails-mock.ts** + +In `frontend/src/testing/wails-mock.ts`, change line 59: + +```typescript +// OLD: +downloadAndInstall: vi.fn().mockResolvedValue(undefined), + +// NEW: +downloadAndInstall: vi.fn().mockResolvedValue({ restart_required: false }), +``` + +- [ ] **Step 5: Update settings.component.ts — add state and handle result** + +In `frontend/src/app/features/settings/settings.component.ts`, add a new property after `updateSuccess`: + +```typescript +updateRestartRequired = false; +``` + +Update the `installUpdate()` method: + +```typescript +async installUpdate(): Promise { + this.updateInstalling = true; + this.updateError = ''; + this.updateSuccess = false; + this.updateRestartRequired = false; + try { + const result = await this.wails.downloadAndInstall(); + this.updateSuccess = true; + this.updateRestartRequired = result.restart_required; + this.updateInfo = null; + } catch (e) { + this.updateError = `Install failed: ${e instanceof Error ? e.message : String(e)}`; + } finally { + this.updateInstalling = false; + this.cdr.detectChanges(); + } + } +``` + +- [ ] **Step 6: Update the template success message** + +In `frontend/src/app/features/settings/settings.component.ts`, replace the `updateSuccess` message block (lines 262-268): + +```html + @if (updateSuccess) { + + } +``` + +- [ ] **Step 7: Run frontend tests** + +Run: `cd frontend && npm test` + +Expected: ALL PASS — new tests pass, existing `installUpdate()` test passes because the mock now returns `{ restart_required: false }`. + +- [ ] **Step 8: Commit** + +```bash +git add frontend/src/app/core/wails.service.ts \ + frontend/src/testing/wails-mock.ts \ + frontend/src/app/features/settings/settings.component.ts \ + frontend/src/app/features/settings/settings.component.spec.ts +git commit -m "feat(updater): frontend handles restart_required from updater" +``` + +--- + +### Task 5: Final Verification + +- [ ] **Step 1: Run all Go tests** + +Run: `go test ./internal/... -v` + +Expected: ALL PASS. + +- [ ] **Step 2: Run all frontend tests** + +Run: `cd frontend && npm test` + +Expected: ALL PASS, 0 failures. + +- [ ] **Step 3: Build the project** + +Run: `cd frontend && npm run build && cd .. && go build -o bin/KeyLint .` + +Expected: Compiles cleanly. + +- [ ] **Step 4: Review the regression test one more time** + +Run: `go test ./internal/features/updater/ -run TestSelfupdateFailsOnReadOnlyDirectory -v` + +Expected: PASS — the regression test still documents that the old approach would fail, proving the bug existed and our new code path avoids it. + +- [ ] **Step 5: Commit any remaining changes** + +If the bindings regeneration or build produced changes not yet committed: + +```bash +git add -A # review what's staged first +git commit -m "chore(updater): final verification — all tests pass, build clean" +``` diff --git a/docs/superpowers/specs/2026-04-05-updater-platform-aware-install-design.md b/docs/superpowers/specs/2026-04-05-updater-platform-aware-install-design.md new file mode 100644 index 0000000..8cf420e --- /dev/null +++ b/docs/superpowers/specs/2026-04-05-updater-platform-aware-install-design.md @@ -0,0 +1,106 @@ +# Platform-Aware Update Installation + +**Date:** 2026-04-05 +**Issue:** [#18](https://github.com/0xMMA/KeyLint/issues/18) — App update via "Download & Install" button fails on Windows +**Status:** Approved + +## Problem + +`DownloadAndInstall()` uses `minio/selfupdate.Apply()` to replace the running executable in-place. This fails on Windows for two reasons: + +1. **Wrong asset type**: The Windows release asset (`*-windows-amd64-setup`) is an NSIS installer, not a raw binary. Piping it through `selfupdate.Apply()` would produce a corrupt executable even if it succeeded. +2. **No elevation**: The app installs to `C:\Program Files\KeyLint\KeyLint\`. Writing to Program Files requires admin privileges. `selfupdate.Apply()` creates `.KeyLint.exe.new` in the same directory without UAC elevation, triggering "Access is denied." + +## Solution + +Split `DownloadAndInstall()` into platform-specific install strategies. Both platforms share the download-to-temp phase; the install phase diverges. + +### Flow + +``` +DownloadAndInstall() + 1. CheckForUpdate() // unchanged + 2. HTTP GET release asset → temp file // shared + 3. Platform-specific install: + ├── [windows] os.StartProcess(temp.exe) → return ErrRestartRequired + └── [linux] selfupdate.Apply(tempFile) → return nil +``` + +### Windows Strategy + +1. Download the NSIS installer to `os.CreateTemp("", "KeyLint-update-*.exe")`. +2. Launch the installer via `os.StartProcess` (detached — survives parent exit). The NSIS installer's own manifest requests UAC elevation. +3. Return sentinel `ErrRestartRequired`. The caller (frontend bridge or main.go) uses this to quit the app gracefully so the installer can replace the locked executable. + +### Linux Strategy + +1. Download the raw binary to `os.CreateTemp("", "KeyLint-update-*")`. +2. Apply via `selfupdate.Apply(file, selfupdate.Options{})`. +3. If permission error, return a descriptive message: `"update downloaded to — install manually or run with appropriate permissions"`. + +### Sentinel Error + +```go +var ErrRestartRequired = errors.New("update requires application restart") +``` + +Callers check `errors.Is(err, ErrRestartRequired)` to distinguish "quit now, installer is running" from actual failures. + +## File Changes + +| File | Change | +|------|--------| +| `internal/features/updater/service.go` | Refactor `DownloadAndInstall()` to download to temp, delegate to `applyPlatformUpdate()` | +| `internal/features/updater/install_windows.go` | `applyPlatformUpdate()`: launch NSIS installer, return `ErrRestartRequired` | +| `internal/features/updater/install_linux.go` | `applyPlatformUpdate()`: `selfupdate.Apply()` from temp file | +| `internal/features/updater/install_test.go` | Regression test (prove bug) + new behavior tests | +| `frontend/.../settings.component.ts` | Handle restart-required response (show "closing" message, trigger app quit) | +| `frontend/.../settings.component.spec.ts` | Test for restart-required UI path | +| `frontend/.../wails.service.ts` | No change — `downloadAndInstall()` RPC signature unchanged | + +## Testing Plan + +### Regression Tests (prove the bug) + +1. **Test selfupdate fails on read-only directory**: Create a temp executable in a read-only dir, attempt `selfupdate.Apply()` targeting it, assert permission error. Documents WHY the old approach was replaced. +2. **Test Windows asset is an installer, not a raw binary**: Assert `matchPlatformAsset()` returns a `*-setup*` URL on Windows — proving the downloaded file is an NSIS installer that cannot be used as a binary replacement. + +### Fix Tests (prove it works) + +3. **Test download-to-temp**: httptest server serves fake bytes → assert temp file exists with correct content and size. +4. **Test Windows install path**: Provide a mock/stub process launcher → assert it receives the temp `.exe` path, assert `ErrRestartRequired` is returned. +5. **Test Linux install path**: `selfupdate.Apply()` against a writable temp target → assert success, assert no `ErrRestartRequired`. +6. **Test error paths**: Download returns HTTP 500 → error. Download returns empty body → error. Temp file creation fails → error. + +### Frontend Tests + +7. **Test restart-required UI**: Mock `downloadAndInstall()` to return restart-required → assert "closing" message shown. +8. **Test error UI**: Mock `downloadAndInstall()` to throw → assert error message shown (existing test, verify still passes). + +### Manual Verification + +- Build Windows installer, install to Program Files, trigger update → NSIS wizard launches, app closes, update completes. +- This step cannot be automated in CI (requires Windows GUI + real GitHub release). + +## Frontend UX Change + +Current behavior after successful install: +> "Update installed! Restart the app to use the new version." + +New behavior (Windows): +> "Installing update... the app will close shortly." + +Then the app quits after a brief delay (1-2s) to let the user read the message. The NSIS installer takes over. + +Linux behavior unchanged — same "Restart" message as before. + +## Dependencies + +- No new dependencies. `minio/selfupdate` stays for Linux. `os.StartProcess` is stdlib. +- NSIS installer is already built by CI (`release.yml`). + +## Out of Scope + +- Delta updates, silent background updates (future: Velopack when Go SDK matures). +- Automatic restart after update on Linux. +- macOS support. diff --git a/frontend/bindings/keylint/internal/features/logger/service.js b/frontend/bindings/keylint/internal/features/logger/service.js index 4a07df1..3ced463 100644 --- a/frontend/bindings/keylint/internal/features/logger/service.js +++ b/frontend/bindings/keylint/internal/features/logger/service.js @@ -13,6 +13,8 @@ import { Call as $Call, CancellablePromise as $CancellablePromise, Create as $Cr /** * Log writes a frontend message at the given level into debug.log. + * The msg is wrapped in Redact() because frontend messages may contain user text. + * Error and warn levels log the msg directly (operational). * @param {string} level * @param {string} msg * @returns {$CancellablePromise} diff --git a/frontend/bindings/keylint/internal/features/settings/models.js b/frontend/bindings/keylint/internal/features/settings/models.js index 9ab2949..1fea0c2 100644 --- a/frontend/bindings/keylint/internal/features/settings/models.js +++ b/frontend/bindings/keylint/internal/features/settings/models.js @@ -179,7 +179,7 @@ export class Settings { } if (!("log_level" in $$source)) { /** - * "off" | "trace" | "debug" | "info" | "warning" | "error" + * "off"|"trace"|"debug"|"info"|"warning"|"error" * @member * @type {string} */ diff --git a/frontend/bindings/keylint/internal/features/updater/index.js b/frontend/bindings/keylint/internal/features/updater/index.js index e5c5c4d..54b3643 100644 --- a/frontend/bindings/keylint/internal/features/updater/index.js +++ b/frontend/bindings/keylint/internal/features/updater/index.js @@ -8,5 +8,6 @@ export { }; export { + InstallResult, UpdateInfo } from "./models.js"; diff --git a/frontend/bindings/keylint/internal/features/updater/models.js b/frontend/bindings/keylint/internal/features/updater/models.js index dafb4e7..675f1ba 100644 --- a/frontend/bindings/keylint/internal/features/updater/models.js +++ b/frontend/bindings/keylint/internal/features/updater/models.js @@ -6,6 +6,38 @@ // @ts-ignore: Unused imports import { Create as $Create } from "@wailsio/runtime"; +/** + * InstallResult is returned by DownloadAndInstall to indicate the outcome. + * On Windows, RestartRequired is true because the NSIS installer needs the app to exit. + */ +export class InstallResult { + /** + * Creates a new InstallResult instance. + * @param {Partial} [$$source = {}] - The source object to create the InstallResult. + */ + constructor($$source = {}) { + if (!("restart_required" in $$source)) { + /** + * @member + * @type {boolean} + */ + this["restart_required"] = false; + } + + Object.assign(this, $$source); + } + + /** + * Creates a new InstallResult instance from a string or object. + * @param {any} [$$source = {}] + * @returns {InstallResult} + */ + static createFrom($$source = {}) { + let $$parsedSource = typeof $$source === 'string' ? JSON.parse($$source) : $$source; + return new InstallResult(/** @type {Partial} */($$parsedSource)); + } +} + /** * UpdateInfo is returned to the frontend to describe whether an update is available. */ diff --git a/frontend/bindings/keylint/internal/features/updater/service.js b/frontend/bindings/keylint/internal/features/updater/service.js index 4a69e23..1fc46a5 100644 --- a/frontend/bindings/keylint/internal/features/updater/service.js +++ b/frontend/bindings/keylint/internal/features/updater/service.js @@ -3,7 +3,7 @@ // This file is automatically generated. DO NOT EDIT /** - * Service checks for updates and can apply them in-place using minio/selfupdate. + * Service checks for updates and can apply them using platform-specific strategies. * @module */ @@ -26,11 +26,16 @@ export function CheckForUpdate() { } /** - * DownloadAndInstall fetches the binary for the current platform and replaces the running exe. - * @returns {$CancellablePromise} + * DownloadAndInstall fetches the release asset for the current platform, saves it + * to a temp file, and delegates to the platform-specific installer. + * On Windows this launches the NSIS setup and returns InstallResult{RestartRequired: true}. + * On Linux this applies the binary in-place via selfupdate. + * @returns {$CancellablePromise<$models.InstallResult>} */ export function DownloadAndInstall() { - return $Call.ByID(1125161478); + return $Call.ByID(1125161478).then(/** @type {($result: any) => any} */(($result) => { + return $$createType1($result); + })); } /** @@ -41,5 +46,16 @@ export function GetVersion() { return $Call.ByID(4045591078); } +/** + * SetQuitFunc sets the callback invoked after launching the installer on Windows. + * The callback should wait briefly (for the frontend to display a message) then quit the app. + * @param {any} fn + * @returns {$CancellablePromise} + */ +export function SetQuitFunc(fn) { + return $Call.ByID(1352960079, fn); +} + // Private type creation functions const $$createType0 = $models.UpdateInfo.createFrom; +const $$createType1 = $models.InstallResult.createFrom; diff --git a/frontend/src/app/core/wails.service.ts b/frontend/src/app/core/wails.service.ts index f9ba151..f2de1ed 100644 --- a/frontend/src/app/core/wails.service.ts +++ b/frontend/src/app/core/wails.service.ts @@ -12,10 +12,10 @@ import * as UpdaterService from '../../../bindings/keylint/internal/features/upd import * as LoggerService from '../../../bindings/keylint/internal/features/logger/service.js'; import * as PyramidizeService from '../../../bindings/keylint/internal/features/pyramidize/service.js'; import { Settings, KeyStatus } from '../../../bindings/keylint/internal/features/settings/models.js'; -import { UpdateInfo } from '../../../bindings/keylint/internal/features/updater/models.js'; +import { UpdateInfo, InstallResult } from '../../../bindings/keylint/internal/features/updater/models.js'; import type { PyramidizeRequest, PyramidizeResult, RefineGlobalRequest, RefineGlobalResult, SpliceRequest, SpliceResult, AppPreset } from '../../../bindings/keylint/internal/features/pyramidize/models.js'; -export type { Settings, KeyStatus, UpdateInfo }; +export type { Settings, KeyStatus, UpdateInfo, InstallResult }; export type { PyramidizeRequest, PyramidizeResult, RefineGlobalRequest, RefineGlobalResult, SpliceRequest, SpliceResult, AppPreset }; @@ -160,7 +160,7 @@ export class WailsService implements OnDestroy { return UpdaterService.CheckForUpdate(); } - downloadAndInstall(): Promise { + downloadAndInstall(): Promise { return UpdaterService.DownloadAndInstall(); } diff --git a/frontend/src/app/features/settings/settings.component.spec.ts b/frontend/src/app/features/settings/settings.component.spec.ts index ac8e2ed..79a4e12 100644 --- a/frontend/src/app/features/settings/settings.component.spec.ts +++ b/frontend/src/app/features/settings/settings.component.spec.ts @@ -241,11 +241,27 @@ describe('SettingsComponent', () => { }); it('installUpdate() sets updateSuccess on success', async () => { - wailsMock.downloadAndInstall.mockResolvedValue(undefined); + wailsMock.downloadAndInstall.mockResolvedValue({ restart_required: false }); component.updateInfo = { ...defaultUpdateInfo, is_available: true, latest_version: '3.7.0' }; await component.installUpdate(); expect(component.updateSuccess).toBe(true); expect(wailsMock.downloadAndInstall).toHaveBeenCalled(); }); + + it('installUpdate() shows restart message when restart_required is true', async () => { + wailsMock.downloadAndInstall.mockResolvedValue({ restart_required: true }); + component.updateInfo = { ...defaultUpdateInfo, is_available: true, latest_version: '3.7.0' }; + await component.installUpdate(); + expect(component.updateSuccess).toBe(true); + expect(component.updateRestartRequired).toBe(true); + }); + + it('installUpdate() shows standard success when restart_required is false', async () => { + wailsMock.downloadAndInstall.mockResolvedValue({ restart_required: false }); + component.updateInfo = { ...defaultUpdateInfo, is_available: true, latest_version: '3.7.0' }; + await component.installUpdate(); + expect(component.updateSuccess).toBe(true); + expect(component.updateRestartRequired).toBe(false); + }); }); }); diff --git a/frontend/src/app/features/settings/settings.component.ts b/frontend/src/app/features/settings/settings.component.ts index 2f523a9..8b004e9 100644 --- a/frontend/src/app/features/settings/settings.component.ts +++ b/frontend/src/app/features/settings/settings.component.ts @@ -263,7 +263,9 @@ interface ProviderKey { } @@ -383,6 +385,7 @@ export class SettingsComponent implements OnInit { updateInstalling = false; updateError = ''; updateSuccess = false; + updateRestartRequired = false; // App Defaults tab state presets: AppPreset[] = []; @@ -525,9 +528,11 @@ export class SettingsComponent implements OnInit { this.updateInstalling = true; this.updateError = ''; this.updateSuccess = false; + this.updateRestartRequired = false; try { - await this.wails.downloadAndInstall(); + const result = await this.wails.downloadAndInstall(); this.updateSuccess = true; + this.updateRestartRequired = result.restart_required; this.updateInfo = null; } catch (e) { this.updateError = `Install failed: ${e instanceof Error ? e.message : String(e)}`; diff --git a/frontend/src/testing/wails-mock.ts b/frontend/src/testing/wails-mock.ts index 719f5db..d2403e1 100644 --- a/frontend/src/testing/wails-mock.ts +++ b/frontend/src/testing/wails-mock.ts @@ -1,6 +1,6 @@ import { Subject } from 'rxjs'; import { vi } from 'vitest'; -import type { Settings, KeyStatus, UpdateInfo } from '../app/core/wails.service'; +import type { Settings, KeyStatus, UpdateInfo, InstallResult } from '../app/core/wails.service'; export const defaultSettings: Settings = { active_provider: 'openai', @@ -56,7 +56,7 @@ export function createWailsMock() { resetSettings: vi.fn().mockResolvedValue(undefined), getVersion: vi.fn().mockResolvedValue('3.6.0'), checkForUpdate: vi.fn().mockResolvedValue({ ...defaultUpdateInfo }), - downloadAndInstall: vi.fn().mockResolvedValue(undefined), + downloadAndInstall: vi.fn().mockResolvedValue({ restart_required: false }), log: vi.fn().mockResolvedValue(undefined), pasteToForeground: vi.fn().mockResolvedValue(undefined), ngOnDestroy: vi.fn(), diff --git a/internal/features/updater/install_linux.go b/internal/features/updater/install_linux.go new file mode 100644 index 0000000..5084b6e --- /dev/null +++ b/internal/features/updater/install_linux.go @@ -0,0 +1,30 @@ +//go:build !windows + +package updater + +import ( + "fmt" + "os" + + "github.com/minio/selfupdate" +) + +// applyPlatformUpdate applies the downloaded binary in-place using selfupdate. +// On Linux the release asset is a raw executable, so direct replacement works +// as long as the target directory is writable. +func applyPlatformUpdate(svc *Service, tmpPath string) (InstallResult, error) { + f, err := os.Open(tmpPath) + if err != nil { + return InstallResult{}, fmt.Errorf("opening downloaded update: %w", err) + } + defer f.Close() + + if err := selfupdate.Apply(f, selfupdate.Options{}); err != nil { + return InstallResult{}, fmt.Errorf("applying update: %w — if this is a permission error, "+ + "the downloaded file is at %s — install manually or run with appropriate permissions", + err, tmpPath) + } + + os.Remove(tmpPath) + return InstallResult{RestartRequired: false}, nil +} diff --git a/internal/features/updater/install_test.go b/internal/features/updater/install_test.go new file mode 100644 index 0000000..206e49b --- /dev/null +++ b/internal/features/updater/install_test.go @@ -0,0 +1,219 @@ +package updater + +import ( + "bytes" + "fmt" + "net/http" + "net/http/httptest" + "os" + "path/filepath" + "strings" + "testing" + + "github.com/minio/selfupdate" +) + +// TestSelfupdateFailsOnReadOnlyDirectory documents the root cause of GH #18: +// selfupdate.Apply() cannot write to directories that require elevated permissions +// (e.g. C:\Program Files\ on Windows). This test ensures we never regress to the +// old in-place replacement approach for directories that aren't user-writable. +func TestSelfupdateFailsOnReadOnlyDirectory(t *testing.T) { + dir := t.TempDir() + fakeBin := filepath.Join(dir, "KeyLint.exe") + if err := os.WriteFile(fakeBin, []byte("old binary"), 0o755); err != nil { + t.Fatal(err) + } + // Make the directory read-only so selfupdate cannot create .KeyLint.exe.new + if err := os.Chmod(dir, 0o555); err != nil { + t.Fatal(err) + } + t.Cleanup(func() { os.Chmod(dir, 0o755) }) + + err := selfupdate.Apply(strings.NewReader("new binary"), selfupdate.Options{ + TargetPath: fakeBin, + }) + + if err == nil { + t.Fatal("expected permission error when applying update to read-only directory, got nil — " + + "this means the old selfupdate.Apply() approach would silently corrupt the install") + } + + // The error message varies by OS: "permission denied" (Linux) or "Access is denied" (Windows). + errMsg := strings.ToLower(err.Error()) + if !strings.Contains(errMsg, "permission denied") && !strings.Contains(errMsg, "access is denied") { + t.Errorf("expected permission-related error, got: %v", err) + } +} + +// serveAsset creates an httptest server that serves the given payload for any request. +func serveAsset(payload []byte) *httptest.Server { + return httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Write(payload) + })) +} + +// makeReleaseWithURL creates a release whose asset URL points to a custom server. +func makeReleaseWithURL(tag string, assetName string, assetURL string) githubRelease { + return githubRelease{ + TagName: tag, + Name: tag, + Body: "Release " + tag, + Draft: false, + Prerelease: false, + Assets: []githubAsset{{ + Name: assetName, + BrowserDownloadURL: assetURL, + }}, + } +} + +func TestDownloadAndInstall_DownloadsToTempAndCallsApply(t *testing.T) { + payload := []byte("fake-installer-payload-12345") + assetSrv := serveAsset(payload) + defer assetSrv.Close() + + releases := []githubRelease{ + makeReleaseWithURL("v9.0.0", "KeyLint-v9.0.0-linux-amd64", assetSrv.URL+"/dl"), + } + releaseSrv := serveGitHubReleases(t, releases) + defer releaseSrv.Close() + + svc := newTestService("1.0.0", releaseSrv) + + // Override the platform apply function to capture and verify the temp file + // instead of calling selfupdate.Apply (which would corrupt the test binary). + var capturedPath string + var capturedContent []byte + svc.applyFunc = func(_ *Service, tmpPath string) (InstallResult, error) { + capturedPath = tmpPath + data, err := os.ReadFile(tmpPath) + if err != nil { + t.Fatalf("applyFunc: failed to read temp file: %v", err) + } + capturedContent = data + return InstallResult{RestartRequired: false}, nil + } + + result, err := svc.DownloadAndInstall() + if err != nil { + t.Fatalf("DownloadAndInstall() error: %v", err) + } + if result.RestartRequired { + t.Error("expected RestartRequired=false from test applyFunc") + } + if capturedPath == "" { + t.Fatal("applyFunc was never called — download phase may have failed") + } + if !bytes.Equal(capturedContent, payload) { + t.Errorf("temp file content = %q, want %q", capturedContent, payload) + } +} + +func TestDownloadAndInstall_ApplyFuncRestartRequired(t *testing.T) { + assetSrv := serveAsset([]byte("installer")) + defer assetSrv.Close() + + releases := []githubRelease{ + makeReleaseWithURL("v9.0.0", "KeyLint-v9.0.0-linux-amd64", assetSrv.URL+"/dl"), + } + releaseSrv := serveGitHubReleases(t, releases) + defer releaseSrv.Close() + + svc := newTestService("1.0.0", releaseSrv) + svc.applyFunc = func(_ *Service, _ string) (InstallResult, error) { + return InstallResult{RestartRequired: true}, nil + } + + result, err := svc.DownloadAndInstall() + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if !result.RestartRequired { + t.Error("expected RestartRequired=true (simulating Windows path)") + } +} + +func TestDownloadAndInstall_NoUpdateAvailable(t *testing.T) { + releases := []githubRelease{ + makeRelease("v1.0.0", false, false, "KeyLint-v1.0.0-linux-amd64"), + } + srv := serveGitHubReleases(t, releases) + defer srv.Close() + + svc := newTestService("1.0.0", srv) + _, err := svc.DownloadAndInstall() + if err == nil { + t.Fatal("expected error for no-update-available") + } + if !strings.Contains(err.Error(), "no update available") { + t.Errorf("unexpected error: %v", err) + } +} + +func TestDownloadAndInstall_DownloadHTTPError(t *testing.T) { + errorSrv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + http.Error(w, "not found", http.StatusNotFound) + })) + defer errorSrv.Close() + + releases := []githubRelease{ + makeReleaseWithURL("v9.0.0", "KeyLint-v9.0.0-linux-amd64", errorSrv.URL+"/missing"), + } + releaseSrv := serveGitHubReleases(t, releases) + defer releaseSrv.Close() + + svc := newTestService("1.0.0", releaseSrv) + _, err := svc.DownloadAndInstall() + if err == nil { + t.Fatal("expected error for HTTP 404 download") + } + if !strings.Contains(err.Error(), "404") { + t.Errorf("expected status code in error, got: %v", err) + } +} + +func TestDownloadAndInstall_EmptyBody(t *testing.T) { + emptySrv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + // 200 OK but empty body + })) + defer emptySrv.Close() + + releases := []githubRelease{ + makeReleaseWithURL("v9.0.0", "KeyLint-v9.0.0-linux-amd64", emptySrv.URL+"/empty"), + } + releaseSrv := serveGitHubReleases(t, releases) + defer releaseSrv.Close() + + svc := newTestService("1.0.0", releaseSrv) + _, err := svc.DownloadAndInstall() + if err == nil { + t.Fatal("expected error for empty download body") + } + if !strings.Contains(err.Error(), "empty") { + t.Errorf("expected 'empty' in error, got: %v", err) + } +} + +func TestDownloadAndInstall_ApplyFuncError(t *testing.T) { + assetSrv := serveAsset([]byte("binary")) + defer assetSrv.Close() + + releases := []githubRelease{ + makeReleaseWithURL("v9.0.0", "KeyLint-v9.0.0-linux-amd64", assetSrv.URL+"/dl"), + } + releaseSrv := serveGitHubReleases(t, releases) + defer releaseSrv.Close() + + svc := newTestService("1.0.0", releaseSrv) + svc.applyFunc = func(_ *Service, _ string) (InstallResult, error) { + return InstallResult{}, fmt.Errorf("simulated install failure") + } + + _, err := svc.DownloadAndInstall() + if err == nil { + t.Fatal("expected error from applyFunc") + } + if !strings.Contains(err.Error(), "simulated install failure") { + t.Errorf("expected applyFunc error to propagate, got: %v", err) + } +} diff --git a/internal/features/updater/install_windows.go b/internal/features/updater/install_windows.go new file mode 100644 index 0000000..4503c3c --- /dev/null +++ b/internal/features/updater/install_windows.go @@ -0,0 +1,32 @@ +//go:build windows + +package updater + +import ( + "fmt" + "os" + "os/exec" +) + +// applyPlatformUpdate launches the downloaded NSIS installer and signals the app to quit. +// The NSIS installer's own manifest requests UAC elevation, so no special privileges +// are needed here — Windows will show the UAC prompt to the user. +func applyPlatformUpdate(svc *Service, tmpPath string) (InstallResult, error) { + cmd := exec.Command(tmpPath) + cmd.Stdout = os.Stdout + cmd.Stderr = os.Stderr + + if err := cmd.Start(); err != nil { + os.Remove(tmpPath) + return InstallResult{}, fmt.Errorf("launching installer: %w", err) + } + + // Schedule app quit so the installer can replace the locked executable. + // The quitFunc is set by main.go and includes a short delay for the + // frontend to display the "closing" message before the app exits. + if svc.quitFunc != nil { + go svc.quitFunc() + } + + return InstallResult{RestartRequired: true}, nil +} diff --git a/internal/features/updater/model.go b/internal/features/updater/model.go index 24757bc..9ef8ec0 100644 --- a/internal/features/updater/model.go +++ b/internal/features/updater/model.go @@ -24,6 +24,12 @@ type PlatformAsset struct { Signature string `json:"signature"` } +// InstallResult is returned by DownloadAndInstall to indicate the outcome. +// On Windows, RestartRequired is true because the NSIS installer needs the app to exit. +type InstallResult struct { + RestartRequired bool `json:"restart_required"` +} + // githubRelease represents a single release from the GitHub Releases API. type githubRelease struct { TagName string `json:"tag_name"` diff --git a/internal/features/updater/service.go b/internal/features/updater/service.go index b7c637d..5fa344d 100644 --- a/internal/features/updater/service.go +++ b/internal/features/updater/service.go @@ -2,7 +2,9 @@ package updater import ( "encoding/json" + "errors" "fmt" + "io" "net/http" "os" "runtime" @@ -10,18 +12,18 @@ import ( "strings" "keylint/internal/features/settings" - - "github.com/minio/selfupdate" ) const defaultReleasesAPIURL = "https://api.github.com/repos/0xMMA/KeyLint/releases?per_page=20" -// Service checks for updates and can apply them in-place using minio/selfupdate. +// Service checks for updates and can apply them using platform-specific strategies. type Service struct { - currentVersion string - releasesAPIURL string - client *http.Client - settingsSvc *settings.Service + currentVersion string + releasesAPIURL string + client *http.Client + settingsSvc *settings.Service + quitFunc func() // called after launching installer on Windows; set via SetQuitFunc + applyFunc func(svc *Service, tmpPath string) (InstallResult, error) // override for testing; nil uses applyPlatformUpdate } // NewService creates an updater Service with the given current version string. @@ -40,6 +42,12 @@ func (s *Service) GetVersion() string { return s.currentVersion } +// SetQuitFunc sets the callback invoked after launching the installer on Windows. +// The callback should wait briefly (for the frontend to display a message) then quit the app. +func (s *Service) SetQuitFunc(fn func()) { + s.quitFunc = fn +} + // CheckForUpdate fetches the GitHub Releases API and finds the best available update. func (s *Service) CheckForUpdate() (UpdateInfo, error) { info := UpdateInfo{CurrentVersion: s.currentVersion} @@ -141,40 +149,56 @@ func matchPlatformAsset(assets []githubAsset) string { return "" } -// DownloadAndInstall fetches the binary for the current platform and replaces the running exe. -func (s *Service) DownloadAndInstall() error { +// DownloadAndInstall fetches the release asset for the current platform, saves it +// to a temp file, and delegates to the platform-specific installer. +// On Windows this launches the NSIS setup and returns InstallResult{RestartRequired: true}. +// On Linux this applies the binary in-place via selfupdate. +func (s *Service) DownloadAndInstall() (InstallResult, error) { updateInfo, err := s.CheckForUpdate() if err != nil { - return fmt.Errorf("checking for update: %w", err) + return InstallResult{}, fmt.Errorf("checking for update: %w", err) } if !updateInfo.IsAvailable { - return fmt.Errorf("no update available") + return InstallResult{}, fmt.Errorf("no update available") } if updateInfo.ReleaseURL == "" { - return fmt.Errorf("no download URL for current platform") + return InstallResult{}, fmt.Errorf("no download URL for current platform") } resp, err := s.client.Get(updateInfo.ReleaseURL) if err != nil { - return fmt.Errorf("downloading update: %w", err) + return InstallResult{}, fmt.Errorf("downloading update: %w", err) } defer resp.Body.Close() if resp.StatusCode != http.StatusOK { - return fmt.Errorf("download returned status %d", resp.StatusCode) + return InstallResult{}, fmt.Errorf("download returned status %d", resp.StatusCode) } - if err := selfupdate.Apply(resp.Body, selfupdate.Options{}); err != nil { - return fmt.Errorf("applying update: %w", err) + // Write to a temp file so platform-specific code can work with a path on disk. + tmpFile, err := os.CreateTemp("", "KeyLint-update-*.exe") + if err != nil { + return InstallResult{}, fmt.Errorf("creating temp file: %w", err) } - // Restart the process so the new binary takes effect immediately. - executable, err := os.Executable() + n, err := io.Copy(tmpFile, resp.Body) if err != nil { - return fmt.Errorf("finding executable path: %w", err) + tmpFile.Close() + os.Remove(tmpFile.Name()) + return InstallResult{}, fmt.Errorf("writing update to temp file: %w", err) + } + tmpFile.Close() + + if n == 0 { + os.Remove(tmpFile.Name()) + return InstallResult{}, errors.New("downloaded update is empty") + } + + applyFn := applyPlatformUpdate + if s.applyFunc != nil { + applyFn = s.applyFunc } - _ = executable // exec.Command would be used here for a restart; omitted for simplicity. - return nil + return applyFn(s, tmpFile.Name()) } // parsedVersion holds the decomposed parts of a semver string with optional pre-release suffix. diff --git a/main.go b/main.go index f0945bf..33f9e50 100644 --- a/main.go +++ b/main.go @@ -6,6 +6,7 @@ import ( "fmt" "log" "os" + "time" "keylint/internal/app" "keylint/internal/cli" @@ -86,7 +87,13 @@ func main() { wailsApp.RegisterService(application.NewService(featurelogger.NewService())) // Updater service — AppVersion injected at build time via ldflags. - wailsApp.RegisterService(application.NewService(updater.NewService(AppVersion, services.Settings))) + updaterSvc := updater.NewService(AppVersion, services.Settings) + updaterSvc.SetQuitFunc(func() { + // Brief delay so the frontend can display the "closing" message. + time.Sleep(2 * time.Second) + wailsApp.Quit() + }) + wailsApp.RegisterService(application.NewService(updaterSvc)) // Dev-tools shortcut simulation service. sim := &simulateService{shortcut: services.Shortcut}