From 756dec91866a9801e21b9fc2ac9bd44adc7f7981 Mon Sep 17 00:00:00 2001 From: anandgupta42 Date: Mon, 23 Mar 2026 15:56:10 -0700 Subject: [PATCH] fix: prevent silent upgrade failures from locking users on old versions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The upgrade path is the most critical code — if it breaks, users are permanently locked out. Two fixes: 1. Replace `.catch(() => {})` in worker.ts with error logging so upgrade failures are visible instead of silently swallowed 2. Add comprehensive tests (41 total) including: - semver bundling smoke tests (verify import works, not externalized) - Decision logic tests for every branch in upgrade() - Source assertion that checkUpgrade logs errors (not empty catch) - Installation.VERSION format validation Keeps semver — it bundles correctly into the compiled binary (verified: not in build.ts external list). The real issue was the silent .catch. Co-Authored-By: Claude Opus 4.6 (1M context) --- packages/opencode/src/cli/cmd/tui/worker.ts | 6 +- .../test/cli/upgrade-decision.test.ts | 367 +++++++----------- 2 files changed, 143 insertions(+), 230 deletions(-) diff --git a/packages/opencode/src/cli/cmd/tui/worker.ts b/packages/opencode/src/cli/cmd/tui/worker.ts index ef77fb9df7..342a87708f 100644 --- a/packages/opencode/src/cli/cmd/tui/worker.ts +++ b/packages/opencode/src/cli/cmd/tui/worker.ts @@ -286,7 +286,11 @@ export const rpc = { directory: input.directory, init: InstanceBootstrap, fn: async () => { - await upgrade().catch(() => {}) + await upgrade().catch((err) => { + // Never silently swallow upgrade errors — if this fails, users + // get locked on old versions with no way to self-heal. + console.error("[upgrade] check failed:", String(err)) + }) }, }) }, diff --git a/packages/opencode/test/cli/upgrade-decision.test.ts b/packages/opencode/test/cli/upgrade-decision.test.ts index ef30314eab..1003042ebf 100644 --- a/packages/opencode/test/cli/upgrade-decision.test.ts +++ b/packages/opencode/test/cli/upgrade-decision.test.ts @@ -5,14 +5,18 @@ import { Installation } from "../../src/installation" /** * Tests for the upgrade() decision logic in cli/upgrade.ts. * - * Since upgrade() depends on Config, Bus, and Installation with side effects, - * we test the decision logic directly — the same conditions that upgrade() checks. - * This validates the fix for: silent skip on unknown method, autoupdate=false, - * failed auto-upgrade, and downgrade prevention. + * These mirror the exact control flow in upgrade() so we can test every path + * without needing to mock Config, Bus, and Installation. + * + * The upgrade path is the most critical code in the CLI — if it breaks, + * users are permanently locked on old versions. These tests verify: + * 1. Decision logic for every branch + * 2. semver is importable and works correctly (bundling smoke test) + * 3. The upgrade() module itself can be imported + * 4. The silent .catch(() => {}) was replaced with logging */ -// ─── Decision Logic Extracted from upgrade() ───────────────────────────────── -// These mirror the exact checks in cli/upgrade.ts so we can test every path. +// ─── Decision Logic ───────────────────────────────────────────────────────── type Decision = "skip" | "notify" | "auto-upgrade" @@ -50,276 +54,183 @@ function upgradeDecision(input: { describe("upgrade decision logic", () => { describe("skip: no latest version available", () => { test("latest is undefined (network failure)", () => { - expect(upgradeDecision({ - latest: undefined, - currentVersion: "0.5.2", - autoupdate: undefined, - disableAutoupdate: false, - method: "npm", - })).toBe("skip") + expect(upgradeDecision({ latest: undefined, currentVersion: "0.5.2", autoupdate: undefined, disableAutoupdate: false, method: "npm" })).toBe("skip") }) - test("latest is empty string", () => { - expect(upgradeDecision({ - latest: "", - currentVersion: "0.5.2", - autoupdate: undefined, - disableAutoupdate: false, - method: "npm", - })).toBe("skip") + expect(upgradeDecision({ latest: "", currentVersion: "0.5.2", autoupdate: undefined, disableAutoupdate: false, method: "npm" })).toBe("skip") }) }) describe("skip: already up to date", () => { test("same version string", () => { - expect(upgradeDecision({ - latest: "0.5.7", - currentVersion: "0.5.7", - autoupdate: undefined, - disableAutoupdate: false, - method: "npm", - })).toBe("skip") + expect(upgradeDecision({ latest: "0.5.7", currentVersion: "0.5.7", autoupdate: undefined, disableAutoupdate: false, method: "npm" })).toBe("skip") }) }) describe("skip: downgrade prevention", () => { - test("current version is newer than latest (canary/preview user)", () => { - expect(upgradeDecision({ - latest: "0.5.7", - currentVersion: "0.6.0", - autoupdate: undefined, - disableAutoupdate: false, - method: "npm", - })).toBe("skip") + test("current version is newer than latest", () => { + expect(upgradeDecision({ latest: "0.5.7", currentVersion: "0.6.0", autoupdate: undefined, disableAutoupdate: false, method: "npm" })).toBe("skip") }) - test("current is prerelease of a newer version", () => { - expect(upgradeDecision({ - latest: "0.5.7", - currentVersion: "0.6.0-beta.1", - autoupdate: undefined, - disableAutoupdate: false, - method: "npm", - })).toBe("skip") + expect(upgradeDecision({ latest: "0.5.7", currentVersion: "0.6.0-beta.1", autoupdate: undefined, disableAutoupdate: false, method: "npm" })).toBe("skip") }) - - test("semver.gte catches equal versions even if string !== (edge case)", () => { - // This shouldn't happen in practice (both normalize), but tests the safety net - expect(upgradeDecision({ - latest: "0.5.7", - currentVersion: "0.5.7", - autoupdate: undefined, - disableAutoupdate: false, - method: "npm", - })).toBe("skip") - }) - test("local version bypasses downgrade check", () => { - // Dev mode: VERSION="local" should NOT be caught by semver guard - expect(upgradeDecision({ - latest: "0.5.7", - currentVersion: "local", - autoupdate: undefined, - disableAutoupdate: false, - method: "npm", - })).toBe("auto-upgrade") + expect(upgradeDecision({ latest: "0.5.7", currentVersion: "local", autoupdate: undefined, disableAutoupdate: false, method: "npm" })).toBe("auto-upgrade") }) - - test("invalid semver current version bypasses downgrade check", () => { - expect(upgradeDecision({ - latest: "0.5.7", - currentVersion: "dev-build-123", - autoupdate: undefined, - disableAutoupdate: false, - method: "npm", - })).toBe("auto-upgrade") + test("invalid semver bypasses downgrade check", () => { + expect(upgradeDecision({ latest: "0.5.7", currentVersion: "dev-build-123", autoupdate: undefined, disableAutoupdate: false, method: "npm" })).toBe("auto-upgrade") }) }) describe("notify: autoupdate disabled", () => { test("autoupdate is false", () => { - expect(upgradeDecision({ - latest: "0.5.7", - currentVersion: "0.5.2", - autoupdate: false, - disableAutoupdate: false, - method: "npm", - })).toBe("notify") + expect(upgradeDecision({ latest: "0.5.7", currentVersion: "0.5.2", autoupdate: false, disableAutoupdate: false, method: "npm" })).toBe("notify") }) - - test("OPENCODE_DISABLE_AUTOUPDATE flag is true", () => { - expect(upgradeDecision({ - latest: "0.5.7", - currentVersion: "0.5.2", - autoupdate: undefined, - disableAutoupdate: true, - method: "npm", - })).toBe("notify") + test("DISABLE_AUTOUPDATE flag", () => { + expect(upgradeDecision({ latest: "0.5.7", currentVersion: "0.5.2", autoupdate: undefined, disableAutoupdate: true, method: "npm" })).toBe("notify") }) - - test("both autoupdate=false and flag=true", () => { - expect(upgradeDecision({ - latest: "0.5.7", - currentVersion: "0.5.2", - autoupdate: false, - disableAutoupdate: true, - method: "npm", - })).toBe("notify") + test("both disabled", () => { + expect(upgradeDecision({ latest: "0.5.7", currentVersion: "0.5.2", autoupdate: false, disableAutoupdate: true, method: "npm" })).toBe("notify") }) - - test("autoupdate is 'notify'", () => { - expect(upgradeDecision({ - latest: "0.5.7", - currentVersion: "0.5.2", - autoupdate: "notify", - disableAutoupdate: false, - method: "npm", - })).toBe("notify") + test("notify mode", () => { + expect(upgradeDecision({ latest: "0.5.7", currentVersion: "0.5.2", autoupdate: "notify", disableAutoupdate: false, method: "npm" })).toBe("notify") }) }) - describe("notify: unknown or unsupported install method", () => { - test("method is 'unknown'", () => { - expect(upgradeDecision({ - latest: "0.5.7", - currentVersion: "0.5.2", - autoupdate: undefined, - disableAutoupdate: false, - method: "unknown", - })).toBe("notify") + describe("notify: unsupported method", () => { + test("unknown", () => { + expect(upgradeDecision({ latest: "0.5.7", currentVersion: "0.5.2", autoupdate: undefined, disableAutoupdate: false, method: "unknown" })).toBe("notify") }) - - test("method is 'yarn' (detected but not supported for auto-upgrade)", () => { - expect(upgradeDecision({ - latest: "0.5.7", - currentVersion: "0.5.2", - autoupdate: undefined, - disableAutoupdate: false, - method: "yarn", - })).toBe("notify") + test("yarn", () => { + expect(upgradeDecision({ latest: "0.5.7", currentVersion: "0.5.2", autoupdate: undefined, disableAutoupdate: false, method: "yarn" })).toBe("notify") }) - test("unknown method with autoupdate=false still notifies", () => { - expect(upgradeDecision({ - latest: "0.5.7", - currentVersion: "0.5.2", - autoupdate: false, - disableAutoupdate: false, - method: "unknown", - })).toBe("notify") + expect(upgradeDecision({ latest: "0.5.7", currentVersion: "0.5.2", autoupdate: false, disableAutoupdate: false, method: "unknown" })).toBe("notify") }) }) - describe("auto-upgrade: supported methods with autoupdate enabled", () => { - const supportedMethods = ["npm", "bun", "pnpm", "brew", "curl", "choco", "scoop"] - - for (const method of supportedMethods) { - test(`auto-upgrade for method: ${method}`, () => { - expect(upgradeDecision({ - latest: "0.5.7", - currentVersion: "0.5.2", - autoupdate: undefined, - disableAutoupdate: false, - method, - })).toBe("auto-upgrade") + describe("auto-upgrade: supported methods", () => { + for (const method of ["npm", "bun", "pnpm", "brew", "curl", "choco", "scoop"]) { + test(`method: ${method}`, () => { + expect(upgradeDecision({ latest: "0.5.7", currentVersion: "0.5.2", autoupdate: undefined, disableAutoupdate: false, method })).toBe("auto-upgrade") }) } - test("autoupdate=true explicitly", () => { - expect(upgradeDecision({ - latest: "0.5.7", - currentVersion: "0.5.2", - autoupdate: true, - disableAutoupdate: false, - method: "npm", - })).toBe("auto-upgrade") + expect(upgradeDecision({ latest: "0.5.7", currentVersion: "0.5.2", autoupdate: true, disableAutoupdate: false, method: "npm" })).toBe("auto-upgrade") }) }) - describe("the reported bug: user on 0.5.2, latest is 0.5.7", () => { - test("npm install, default config → should auto-upgrade", () => { - expect(upgradeDecision({ - latest: "0.5.7", - currentVersion: "0.5.2", - autoupdate: undefined, - disableAutoupdate: false, - method: "npm", - })).toBe("auto-upgrade") + describe("the reported bug scenario", () => { + test("npm default config → auto-upgrade", () => { + expect(upgradeDecision({ latest: "0.5.7", currentVersion: "0.5.2", autoupdate: undefined, disableAutoupdate: false, method: "npm" })).toBe("auto-upgrade") }) - - test("unknown method, default config → should notify (was silently skipped before fix)", () => { - expect(upgradeDecision({ - latest: "0.5.7", - currentVersion: "0.5.2", - autoupdate: undefined, - disableAutoupdate: false, - method: "unknown", - })).toBe("notify") + test("unknown method → notify (was silently skipped before fix)", () => { + expect(upgradeDecision({ latest: "0.5.7", currentVersion: "0.5.2", autoupdate: undefined, disableAutoupdate: false, method: "unknown" })).toBe("notify") }) - - test("autoupdate=false → should notify (was silently skipped before fix)", () => { - expect(upgradeDecision({ - latest: "0.5.7", - currentVersion: "0.5.2", - autoupdate: false, - disableAutoupdate: false, - method: "npm", - })).toBe("notify") + test("autoupdate=false → notify (was silently skipped before fix)", () => { + expect(upgradeDecision({ latest: "0.5.7", currentVersion: "0.5.2", autoupdate: false, disableAutoupdate: false, method: "npm" })).toBe("notify") }) + }) - test("DISABLE_AUTOUPDATE flag → should notify (was silently skipped before fix)", () => { - expect(upgradeDecision({ - latest: "0.5.7", - currentVersion: "0.5.2", - autoupdate: undefined, - disableAutoupdate: true, - method: "npm", - })).toBe("notify") + describe("version edge cases", () => { + test("patch bump", () => { + expect(upgradeDecision({ latest: "0.5.3", currentVersion: "0.5.2", autoupdate: undefined, disableAutoupdate: false, method: "npm" })).toBe("auto-upgrade") + }) + test("major bump", () => { + expect(upgradeDecision({ latest: "1.0.0", currentVersion: "0.5.2", autoupdate: undefined, disableAutoupdate: false, method: "npm" })).toBe("auto-upgrade") + }) + test("prerelease latest > stable current", () => { + expect(upgradeDecision({ latest: "1.0.0-beta.1", currentVersion: "0.5.2", autoupdate: undefined, disableAutoupdate: false, method: "npm" })).toBe("auto-upgrade") + }) + test("prerelease latest < stable current → skip", () => { + expect(upgradeDecision({ latest: "0.5.2-beta.1", currentVersion: "0.5.2", autoupdate: undefined, disableAutoupdate: false, method: "npm" })).toBe("skip") }) }) +}) - describe("version format edge cases", () => { - test("patch version bump", () => { - expect(upgradeDecision({ - latest: "0.5.3", - currentVersion: "0.5.2", - autoupdate: undefined, - disableAutoupdate: false, - method: "npm", - })).toBe("auto-upgrade") - }) +// ─── semver bundling smoke test ────────────────────────────────────────────── +// This is the critical guard: verify that semver is importable and works. +// If this test fails in CI, it means the build would ship a broken upgrade path. - test("major version bump", () => { - expect(upgradeDecision({ - latest: "1.0.0", - currentVersion: "0.5.2", - autoupdate: undefined, - disableAutoupdate: false, - method: "npm", - })).toBe("auto-upgrade") - }) +describe("semver bundling health", () => { + test("semver is importable", () => { + expect(typeof semver).toBe("object") + expect(typeof semver.valid).toBe("function") + expect(typeof semver.gte).toBe("function") + expect(typeof semver.compare).toBe("function") + }) - test("prerelease latest version vs stable current", () => { - // 1.0.0-beta.1 is greater than 0.5.2 - expect(upgradeDecision({ - latest: "1.0.0-beta.1", - currentVersion: "0.5.2", - autoupdate: undefined, - disableAutoupdate: false, - method: "npm", - })).toBe("auto-upgrade") - }) + test("semver.valid works", () => { + expect(semver.valid("1.0.0")).toBe("1.0.0") + expect(semver.valid("0.5.7")).toBe("0.5.7") + expect(semver.valid("invalid")).toBeNull() + expect(semver.valid("local")).toBeNull() + }) - test("same major.minor, prerelease latest < current release", () => { - // 0.5.2-beta.1 is LESS than 0.5.2 per semver - expect(upgradeDecision({ - latest: "0.5.2-beta.1", - currentVersion: "0.5.2", - autoupdate: undefined, - disableAutoupdate: false, - method: "npm", - })).toBe("skip") - }) + test("semver.gte works", () => { + expect(semver.gte("0.5.8", "0.5.7")).toBe(true) + expect(semver.gte("0.5.7", "0.5.7")).toBe(true) + expect(semver.gte("0.5.7", "0.5.8")).toBe(false) + }) + + test("semver.compare works for all orderings", () => { + expect(semver.compare("1.0.0", "2.0.0")).toBe(-1) + expect(semver.compare("2.0.0", "1.0.0")).toBe(1) + expect(semver.compare("1.0.0", "1.0.0")).toBe(0) + }) + + test("semver handles prerelease correctly", () => { + expect(semver.gte("1.0.0", "1.0.0-beta.1")).toBe(true) + expect(semver.gte("1.0.0-beta.1", "1.0.0")).toBe(false) + expect(semver.compare("1.0.0-alpha", "1.0.0-beta")).toBe(-1) + expect(semver.compare("1.0.0-alpha.1", "1.0.0-alpha.2")).toBe(-1) + }) + + test("semver is NOT in the build external list", () => { + // If semver is externalized, it won't be bundled in the compiled binary. + // This test reads build.ts to verify it's not listed. + const fs = require("fs") + const path = require("path") + const buildScript = fs.readFileSync( + path.join(import.meta.dir, "../../script/build.ts"), + "utf-8", + ) + // Extract the external array + const externalMatch = buildScript.match(/external:\s*\[([\s\S]*?)\]/) + if (externalMatch) { + expect(externalMatch[1]).not.toContain('"semver"') + expect(externalMatch[1]).not.toContain("'semver'") + } + }) +}) + +// ─── upgrade() module health ───────────────────────────────────────────────── + +describe("upgrade() module health", () => { + test("upgrade function can be imported", async () => { + const mod = await import("../../src/cli/upgrade") + expect(typeof mod.upgrade).toBe("function") + }) + + test("upgrade module imports semver without error", async () => { + // This catches the scenario where semver fails to resolve at import time. + // The import must not throw. + await expect(import("../../src/cli/upgrade")).resolves.toBeDefined() + }) + + test("worker.ts does not silently swallow upgrade errors", () => { + const fs = require("fs") + const path = require("path") + const workerSource = fs.readFileSync( + path.join(import.meta.dir, "../../src/cli/cmd/tui/worker.ts"), + "utf-8", + ) + // Find the checkUpgrade function and verify it logs errors + const checkUpgradeMatch = workerSource.match(/async checkUpgrade[\s\S]*?upgrade\(\)\.catch\(([\s\S]*?)\)/) + expect(checkUpgradeMatch).not.toBeNull() + // The catch handler should reference the error (not be empty) + const catchBody = checkUpgradeMatch![1] + expect(catchBody).toContain("err") }) }) @@ -330,11 +241,9 @@ describe("Installation.VERSION format", () => { expect(typeof Installation.VERSION).toBe("string") expect(Installation.VERSION.length).toBeGreaterThan(0) }) - test("does not have v prefix", () => { expect(Installation.VERSION.startsWith("v")).toBe(false) }) - test("is either 'local' or valid semver", () => { if (Installation.VERSION !== "local") { expect(semver.valid(Installation.VERSION)).not.toBeNull()