From e98ba6daaf21909f1b4653bef600b9dd8edee0a3 Mon Sep 17 00:00:00 2001 From: Sarav Date: Fri, 11 Sep 2026 15:48:11 +0530 Subject: [PATCH 1/9] fix: resolve the install from the running binary; make upgrade failures diagnosable (#1305) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `Installation.method()` never established where the running executable came from. It guessed two ways, and both were unsound: - A substring test on `process.execPath`. `~/.local/bin` is a generic user bin dir, so an npm install with `npm config set prefix ~/.local` was classified `curl`, and `altimate upgrade` ran `curl | bash` — silently converting an npm install into a standalone one and leaving the npm copy orphaned on PATH. - A probe loop (`npm list -g`, `brew list`, ...) returning the first manager whose output mentioned the package. That answers "is this installed anywhere?", not "did THIS binary come from you", so it picked arbitrarily whenever several installs existed. Replaced with `resolveInstall()`, which resolves `realpath(process.execPath)` and matches the package segment. The npm `bin/altimate` shim `spawnSync()`s the per-platform package, so execPath always lands under `node_modules` for package-manager installs; the optional `--` suffix is matched explicitly. Removes up to seven subprocess spawns from the startup update-check path. Added a writability preflight so an upgrade that cannot succeed is refused before shelling out, with a message naming the directory and the exact remedy. Uses `npm root -g` rather than `/lib/node_modules`, which is Unix-only, and derives the bin dir from `npm prefix -g` because `npm bin -g` was removed in npm 9. Also fixed an asymmetry in the failure branch: the success path logged the real stdout/stderr while the failure path discarded them, so every non-permission failure (network, `E404`, `ENOSPC`, a failing lifecycle script) collapsed into an identical `Upgrade failed for npm (exit code N).` with nothing written anywhere. The real output is now logged locally, the message carries a classified hint plus a pointer to the log, and telemetry records a stable classification code instead of the generic string — previously every failed upgrade looked identical on a dashboard. The user-facing message and the telemetry payload stay redacted. Four existing tests asserted on the source text or the exact error string and were updated to track the new contract while preserving their intent (brand guard, redaction guards). Co-Authored-By: Claude Opus 5 (1M context) --- packages/opencode/src/installation/index.ts | 381 +++++++++++++----- .../branding/upstream-merge-guard.test.ts | 19 +- .../test/install/upgrade-method.test.ts | 26 +- .../test/installation/installation.test.ts | 16 +- .../test/installation/resolve-install.test.ts | 82 ++++ .../windows-installer-930.test.ts | 26 +- 6 files changed, 422 insertions(+), 128 deletions(-) create mode 100644 packages/opencode/test/installation/resolve-install.test.ts diff --git a/packages/opencode/src/installation/index.ts b/packages/opencode/src/installation/index.ts index 0fb8ab275..bb0a82d6b 100644 --- a/packages/opencode/src/installation/index.ts +++ b/packages/opencode/src/installation/index.ts @@ -8,6 +8,8 @@ import { errorMessage } from "@/util/error" import { ChildProcess } from "effect/unstable/process" import { AppProcess } from "@opencode-ai/core/process" import path from "path" +import fs from "fs" +import { Global } from "@opencode-ai/core/global" import { EventV2 } from "@opencode-ai/core/event" import { makeRuntime } from "@opencode-ai/core/effect/runtime" import semver from "semver" @@ -37,6 +39,112 @@ const UPGRADE_INSTALL_PS_URL = "https://www.altimate.sh/install.ps1" const UPGRADE_FETCH_TIMEOUT_MS = 15_000 // altimate_change end +// altimate_change start — deterministic install resolution (#1305) +// Detection used to guess two ways, and both were unsound: a substring test on +// process.execPath (`.local/bin` is a generic user bin dir, so an npm install with +// `npm config set prefix ~/.local` was classified "curl" and upgraded via +// `curl | bash`, orphaning the npm copy), and a probe loop asking each package +// manager "do you have this package?" — which answers a different question than +// "did THIS running binary come from you", so it picked arbitrarily whenever more +// than one install existed. +// +// The running binary's own path is the ground truth. The npm `bin/altimate` shim is +// a Node script that spawnSync()s the PLATFORM package's binary, so inside the CLI +// process.execPath is: +// /lib/node_modules/@altimateai/altimate-code/node_modules/ +// @altimateai/altimate-code-darwin-arm64/bin/altimate-code +// i.e. it always lands under node_modules for every package-manager install. Match +// the optional `--` suffix explicitly rather than relying on the +// wrapper name happening to be a prefix of the platform package name. +const PKG_SEGMENT_RE = + /[\\/]node_modules[\\/]@altimateai[\\/]altimate-code(?:-[a-z0-9]+-[a-z0-9]+(?:-[a-z0-9]+)?)?(?:[\\/]|$)/i +// pnpm global installs may expose the package via the `.pnpm` virtual store OR via a +// plain `pnpm/global/` link path (no `.pnpm` segment), so match both spellings — +// otherwise the plain layout falls through to the npm default and routes upgrades at +// the wrong manager. +const PNPM_SEGMENT_RE = /[\\/](?:\.pnpm|pnpm)[\\/]/i +const BUN_SEGMENT_RE = /[\\/]\.bun[\\/]/i +const YARN_SEGMENT_RE = /[\\/](?:\.yarn|yarn[\\/]global)[\\/]/i +// Homebrew bin entries are symlinks into Cellar, so realpath lands there. Match the +// Cellar segment rather than the prefix: /usr/local is also a common npm prefix. +const BREW_SEGMENT_RE = /[\\/]Cellar[\\/]altimate-code[\\/]/i +const SCOOP_SEGMENT_RE = /[\\/]scoop[\\/]apps[\\/]/i +const CHOCO_SEGMENT_RE = /[\\/]chocolatey[\\/]/i +// The standalone (curl / install.ps1 / `install --binary`) layout. `.opencode/bin` is +// the pre-v0.7.1 directory name, kept for users who have not re-installed since. +// NOTE: `.local/bin` is deliberately NOT here — see the comment above. +const STANDALONE_SEGMENT_RE = /[\\/]\.(?:altimate|opencode)[\\/]bin[\\/]/i + +export interface ResolvedInstall { + readonly method: Method + /** Directory the upgrade would mutate. Only set where we can name it without a subprocess. */ + readonly root?: string +} + +/** Resolve the install that produced THIS process. + * + * Pure in (execPath, env) so it can be unit-tested against fabricated layouts + * without spawning real installs. */ +export function resolveInstall( + execPath: string = realExecPath(), + env: NodeJS.ProcessEnv = process.env, +): ResolvedInstall { + // The shim honours ALTIMATE_CODE_BIN_PATH ahead of everything else, so the running + // binary is whatever the user pointed at — not something an installer manages. + // Never auto-upgrade a pinned path. + if (env["ALTIMATE_CODE_BIN_PATH"]) return { method: "unknown" } + + if (PKG_SEGMENT_RE.test(execPath)) { + if (PNPM_SEGMENT_RE.test(execPath)) return { method: "pnpm" } + if (BUN_SEGMENT_RE.test(execPath)) return { method: "bun" } + if (YARN_SEGMENT_RE.test(execPath)) return { method: "yarn" } + return { method: "npm" } + } + if (BREW_SEGMENT_RE.test(execPath)) return { method: "brew" } + if (SCOOP_SEGMENT_RE.test(execPath)) return { method: "scoop" } + if (CHOCO_SEGMENT_RE.test(execPath)) return { method: "choco" } + if (STANDALONE_SEGMENT_RE.test(execPath)) return { method: "curl", root: path.dirname(execPath) } + return { method: "unknown" } +} + +/** realpath so a symlinked bin entry (npm, brew) resolves to the file it points at. + * Falls back to the raw path when the file is gone or unreadable. */ +function realExecPath(): string { + try { + return fs.realpathSync(process.execPath) + } catch { + return process.execPath + } +} + +function isWritable(dir: string): boolean { + try { + fs.accessSync(dir, fs.constants.W_OK) + return true + } catch { + return false + } +} + +/** Classify a failed upgrade into a stable code plus a message safe to show. + * + * Deliberately does NOT echo the package manager's stderr — it can carry tokens and + * environment. The classification is derived from it, the raw text is only logged + * locally (see the logWarning in upgrade()). */ +function classifyFailure(stderr: string, stdout: string): { code: string; hint?: string } { + const t = `${stderr}\n${stdout}` + if (/EACCES|EPERM|permission denied/i.test(t)) + return { code: "permission", hint: "the install directory is not writable" } + if (/ENOTFOUND|ECONNREFUSED|ETIMEDOUT|EAI_AGAIN|network|ENETUNREACH/i.test(t)) + return { code: "network", hint: "the registry could not be reached" } + if (/E404|404 Not Found/i.test(t)) return { code: "not-found", hint: "that version does not exist in the registry" } + if (/ENOSPC|no space left/i.test(t)) return { code: "disk-full", hint: "the disk is full" } + if (/ETARGET|No matching version/i.test(t)) + return { code: "no-matching-version", hint: "no published version satisfies that range" } + return { code: "unknown" } +} +// altimate_change end + export type Method = "curl" | "npm" | "yarn" | "pnpm" | "bun" | "brew" | "scoop" | "choco" | "unknown" export type ReleaseType = "patch" | "minor" | "major" @@ -177,51 +285,123 @@ export const layer: Layer.Layer/node_modules and the shims at itself, so the Unix + // /lib/node_modules is wrong there. `npm bin -g` was REMOVED in npm 9 + // ("Unknown command: bin"), so derive the bin dir from the prefix instead. + const root = (yield* text(["npm", "root", "-g"])).trim() + const prefix = (yield* text(["npm", "prefix", "-g"])).trim() + const bin = prefix ? (process.platform === "win32" ? prefix : path.join(prefix, "bin")) : "" + return [root, bin].filter(Boolean) + } + case "pnpm": { + // Both: a global install writes the store root AND the shim dir; checking only + // one lets the other fail with EACCES after we have already shelled out. + const root = (yield* text(["pnpm", "root", "-g"])).trim() + const bin = (yield* text(["pnpm", "bin", "-g"])).trim() + return [root, bin].filter(Boolean) + } + case "bun": { + const bin = (yield* text(["bun", "pm", "bin", "-g"])).trim() + return [bin].filter(Boolean) + } + case "yarn": { + const dir = (yield* text(["yarn", "global", "dir"])).trim() + const bin = (yield* text(["yarn", "global", "bin"])).trim() + return [dir, bin].filter(Boolean) + } + case "curl": { + const resolved = resolveInstall() + return resolved.root ? [resolved.root] : [] + } + // brew / scoop / choco own their own elevation and policy — do not second-guess them. + default: + return [] as string[] + } + }) + + const remediation = (m: Method, dir: string, target: string) => { + const pkg = `@altimateai/altimate-code@${target}` + switch (m) { + case "npm": + return `Cannot write to the npm global prefix (${dir}). Run \`sudo npm install -g ${pkg}\`, or switch to a user-owned prefix with \`npm config set prefix ~/.npm-global\`.` + case "pnpm": + return `Cannot write to the pnpm global directory (${dir}). Run \`pnpm setup\` to use a user-owned location, or re-run the install with elevated permissions.` + case "bun": + return `Cannot write to the bun global bin directory (${dir}). Set BUN_INSTALL to a user-owned location, or re-run the install with elevated permissions.` + case "yarn": + return `Cannot write to the yarn global directory (${dir}). Set a user-owned prefix with \`yarn config set prefix ~/.yarn\`, or re-run with elevated permissions.` + case "curl": + return `Cannot write to the install directory (${dir}). Fix its permissions, or re-run the installer.` + default: + return `Cannot write to the install directory (${dir}).` + } + } + + /** Returns an error message when the upgrade cannot possibly succeed, else undefined. + * + * Checking first means we never shell out to a command that is going to fail on + * permissions — which is what produced the old, undiagnosable + * "Upgrade failed for npm (exit code 243)." */ + const preflight = Effect.fnUntraced(function* (m: Method, target: string) { + const dirs = yield* globalDirs(m) + for (const dir of dirs) { + if (!dir) continue + // A directory that does not exist yet is not a permission problem: the package + // manager creates it. Only an EXISTING, unwritable directory is a hard stop. + if (!fs.existsSync(dir)) continue + if (!isWritable(dir)) return remediation(m, dir, target) + } + return undefined + }) + // altimate_change end + const upgradeScriptShell = Effect.fnUntraced(function* () { const bashVersion = yield* text(["bash", "--version"]) if (bashVersion) return "bash" return "sh" }) - const upgradeCurl = Effect.fnUntraced( - function* (target: string) { - // altimate_change start — friendly fetch error + manual-recovery hint, branded install URL, bounded timeout - const response = yield* httpOk - .execute(HttpClientRequest.get(UPGRADE_INSTALL_URL)) - .pipe( - Effect.timeout(UPGRADE_FETCH_TIMEOUT_MS), - Effect.mapError( - (err) => - new UpgradeFailedError({ - stderr: - `Could not download install script from ${UPGRADE_INSTALL_URL}: ${errorMessage(err)}. ` + - `Re-run the install manually: curl -fsSL ${UPGRADE_INSTALL_URL} | bash — ` + - `or download a release binary directly from https://github.com/AltimateAI/altimate-code/releases/latest`, - }), - ), - ) - const body = yield* response.text.pipe( - Effect.mapError(() => new UpgradeFailedError({ stderr: upgradeFailure("curl") })), - ) - // altimate_change end - const bodyBytes = new TextEncoder().encode(body) - const shell = yield* upgradeScriptShell() - const result = yield* appProcess - .run( - ChildProcess.make(shell, [], { - stdin: Stream.make(bodyBytes), - env: { VERSION: target }, - extendEnv: true, + const upgradeCurl = Effect.fnUntraced(function* (target: string) { + // altimate_change start — friendly fetch error + manual-recovery hint, branded install URL, bounded timeout + const response = yield* httpOk.execute(HttpClientRequest.get(UPGRADE_INSTALL_URL)).pipe( + Effect.timeout(UPGRADE_FETCH_TIMEOUT_MS), + Effect.mapError( + (err) => + new UpgradeFailedError({ + stderr: + `Could not download install script from ${UPGRADE_INSTALL_URL}: ${errorMessage(err)}. ` + + `Re-run the install manually: curl -fsSL ${UPGRADE_INSTALL_URL} | bash — ` + + `or download a release binary directly from https://github.com/AltimateAI/altimate-code/releases/latest`, }), - ) - .pipe(Effect.mapError(() => new UpgradeFailedError({ stderr: upgradeFailure("curl") }))) - return { - code: result.exitCode, - stdout: result.stdout.toString("utf8"), - stderr: result.stderr.toString("utf8"), - } - }, - ) + ), + ) + const body = yield* response.text.pipe( + Effect.mapError(() => new UpgradeFailedError({ stderr: upgradeFailure("curl") })), + ) + // altimate_change end + const bodyBytes = new TextEncoder().encode(body) + const shell = yield* upgradeScriptShell() + const result = yield* appProcess + .run( + ChildProcess.make(shell, [], { + stdin: Stream.make(bodyBytes), + env: { VERSION: target }, + extendEnv: true, + }), + ) + .pipe(Effect.mapError(() => new UpgradeFailedError({ stderr: upgradeFailure("curl") }))) + return { + code: result.exitCode, + stdout: result.stdout.toString("utf8"), + stderr: result.stderr.toString("utf8"), + } + }) // altimate_change start — Windows curl-install upgrade via PowerShell // The curl/standalone install on native Windows lives in %USERPROFILE%\.altimate\bin @@ -231,20 +411,18 @@ export const layer: Layer.Layer - new UpgradeFailedError({ - stderr: - `Could not download install script from ${UPGRADE_INSTALL_PS_URL}: ${errorMessage(err)}. ` + - `Re-run the install manually: powershell -c "irm ${UPGRADE_INSTALL_PS_URL} | iex" — ` + - `or download a release binary directly from https://github.com/AltimateAI/altimate-code/releases/latest`, - }), - ), - ) + yield* httpOk.execute(HttpClientRequest.head(UPGRADE_INSTALL_PS_URL)).pipe( + Effect.timeout(UPGRADE_FETCH_TIMEOUT_MS), + Effect.mapError( + (err) => + new UpgradeFailedError({ + stderr: + `Could not download install script from ${UPGRADE_INSTALL_PS_URL}: ${errorMessage(err)}. ` + + `Re-run the install manually: powershell -c "irm ${UPGRADE_INSTALL_PS_URL} | iex" — ` + + `or download a release binary directly from https://github.com/AltimateAI/altimate-code/releases/latest`, + }), + ), + ) return yield* run( ["powershell", "-NoProfile", "-ExecutionPolicy", "Bypass", "-Command", `irm ${UPGRADE_INSTALL_PS_URL} | iex`], { env: { VERSION: target } }, @@ -260,52 +438,13 @@ export const layer: Layer.Layer Effect.Effect }> = [ - { name: "npm", command: () => text(["npm", "list", "-g", "--depth=0"]) }, - { name: "yarn", command: () => text(["yarn", "global", "list"]) }, - { name: "pnpm", command: () => text(["pnpm", "list", "-g", "--depth=0"]) }, - { name: "bun", command: () => text(["bun", "pm", "ls", "-g"]) }, - // altimate_change start — brew formula name - { name: "brew", command: () => text(["brew", "list", "--formula", "altimate-code"]) }, - // altimate_change end - { name: "scoop", command: () => text(["scoop", "list", "opencode"]) }, - { name: "choco", command: () => text(["choco", "list", "--limit-output", "opencode"]) }, - ] - - checks.sort((a, b) => { - const aMatches = exec.includes(a.name) - const bMatches = exec.includes(b.name) - if (aMatches && !bMatches) return -1 - if (!aMatches && bMatches) return 1 - return 0 - }) - - for (const check of checks) { - const output = yield* check.command() - // altimate_change start — package names for detection - const installedName = - check.name === "brew" - ? "altimate-code" - : check.name === "choco" || check.name === "scoop" - ? "opencode" - : "@altimateai/altimate-code" - // altimate_change end - if (output.includes(installedName)) { - return check.name - } - } - - return "unknown" as Method }), latest: Effect.fn("Installation.latest")(function* (installMethod?: Method) { const detectedMethod = installMethod || (yield* result.method()) @@ -376,12 +515,15 @@ export const layer: Layer.Layer (exit code N)." with nothing written anywhere. + // The log file is local and already carries this content on success, so logging + // it here is consistency, not new exposure — the user-facing message and the + // telemetry payload both stay redacted. + const classified = classifyFailure(upgradeResult?.stderr ?? "", upgradeResult?.stdout ?? "") + yield* Effect.logWarning("upgrade failed", { + method: m, + target, + code: upgradeResult?.code, + reason: classified.code, + stdout: upgradeResult?.stdout, + stderr: upgradeResult?.stderr, + }) + const base = upgradeFailure(m, upgradeResult) + const stderr = [ + base, + classified.hint ? `Likely cause: ${classified.hint}.` : undefined, + `Details were written to ${Global.Path.log}.`, + ] + .filter(Boolean) + .join(" ") const T = yield* Effect.promise(() => getTelemetry()) T.track({ type: "upgrade_attempted", @@ -449,9 +611,12 @@ export const layer: Layer.Layer layer.pipe(Layer.provide(FetchHttpClient.layer), Layer.provide(AppProcess.defaultLayer))) +export const defaultLayer = Layer.suspend(() => + layer.pipe(Layer.provide(FetchHttpClient.layer), Layer.provide(AppProcess.defaultLayer)), +) // altimate_change end const { runPromise } = makeRuntime(Service, defaultLayer) diff --git a/packages/opencode/test/branding/upstream-merge-guard.test.ts b/packages/opencode/test/branding/upstream-merge-guard.test.ts index 28d441cfa..8d4714f8c 100644 --- a/packages/opencode/test/branding/upstream-merge-guard.test.ts +++ b/packages/opencode/test/branding/upstream-merge-guard.test.ts @@ -51,13 +51,26 @@ describe("Installation script branding", () => { }) test("method() detects npm-installed @altimateai/altimate-code, not opencode-ai", () => { - // The installedName for npm/bun/pnpm must be our scoped package, not upstream + // altimate_change start — #1305: detection moved out of the `method:` block into + // resolveInstall()/PKG_SEGMENT_RE, so slicing between the `method:` and `latest:` + // markers no longer covers it. Assert on the package segment that detection actually + // matches; the brand intent (our scope, never upstream's) is unchanged. + const segment = installSrc.slice( + installSrc.indexOf("const PKG_SEGMENT_RE"), + installSrc.indexOf("export interface ResolvedInstall"), + ) + expect(segment).toContain("@altimateai") + expect(segment).toContain("altimate-code") + expect(segment).not.toContain("opencode-ai") + // The resolver must be what method() returns, so the guard cannot be bypassed by + // leaving a stale detection path behind. const methodBlock = installSrc.slice( installSrc.indexOf('method: Effect.fn("Installation.method")'), installSrc.indexOf('latest: Effect.fn("Installation.latest")'), ) - expect(methodBlock).toContain("@altimateai/altimate-code") - expect(methodBlock).not.toMatch(/installedName[^@]*opencode-ai/) + expect(methodBlock).toContain("resolveInstall()") + expect(methodBlock).not.toMatch(/opencode-ai/) + // altimate_change end }) test("method() detects brew formula as altimate-code, not opencode", () => { diff --git a/packages/opencode/test/install/upgrade-method.test.ts b/packages/opencode/test/install/upgrade-method.test.ts index 653ee77e4..a4823a9cb 100644 --- a/packages/opencode/test/install/upgrade-method.test.ts +++ b/packages/opencode/test/install/upgrade-method.test.ts @@ -8,10 +8,7 @@ import { describe, test, expect } from "bun:test" import fs from "fs" import path from "path" -const INSTALLATION_SRC = fs.readFileSync( - path.resolve(import.meta.dir, "../../src/installation/index.ts"), - "utf-8", -) +const INSTALLATION_SRC = fs.readFileSync(path.resolve(import.meta.dir, "../../src/installation/index.ts"), "utf-8") const CORE_VERSION_SRC = fs.readFileSync( path.resolve(import.meta.dir, "../../../../packages/core/src/installation/version.ts"), "utf-8", @@ -31,9 +28,24 @@ describe("installation method detection", () => { expect(INSTALLATION_SRC).toContain('"brew", "list", "--formula"') }) - test("method detection prioritizes matching exec path", () => { - // checks.sort puts the manager matching process.execPath first - expect(INSTALLATION_SRC).toContain("exec.includes(a.name)") + test("method detection resolves the running binary, not a package-manager listing", () => { + // altimate_change start — #1305: detection no longer sorts a probe list by execPath + // substring. It resolves realpath(process.execPath) and matches the package segment, + // so the assertion tracks the new contract rather than the deleted `checks` array. + expect(INSTALLATION_SRC).toContain("resolveInstall(") + expect(INSTALLATION_SRC).toContain("fs.realpathSync(process.execPath)") + // The probe loop must stay gone: it answered "is this package installed anywhere?", + // which picks arbitrarily when more than one install exists. + expect(INSTALLATION_SRC).not.toContain("exec.includes(a.name)") + // altimate_change end + }) + + test("`.local/bin` is not treated as a standalone install", () => { + // altimate_change start — #1305: `.local/bin` is a generic user bin dir. Treating it + // as curl misrouted `npm config set prefix ~/.local` installs into `curl | bash`, + // which orphaned the npm copy and left two binaries fighting over PATH. + expect(INSTALLATION_SRC).not.toMatch(/path\.join\("\.local", "bin"\)/) + // altimate_change end }) }) diff --git a/packages/opencode/test/installation/installation.test.ts b/packages/opencode/test/installation/installation.test.ts index bb57f836c..40f625fab 100644 --- a/packages/opencode/test/installation/installation.test.ts +++ b/packages/opencode/test/installation/installation.test.ts @@ -186,10 +186,16 @@ describe("installation", () => { Effect.gen(function* () { const error = yield* Effect.flip(Installation.use.upgrade("npm", "9.9.9")) expect(error).toBeInstanceOf(Installation.UpgradeFailedError) - expect(error.stderr).toBe("Upgrade failed for npm (exit code 1).") + // altimate_change start — #1305: the message now also points at the local log, + // where the REAL stderr is written. Redaction is what this test guards, so the + // not.toContain assertions below are the contract; the prefix is matched rather + // than compared exactly so the pointer can be appended. + expect(error.stderr).toContain("Upgrade failed for npm (exit code 1).") + expect(error.stderr).toContain("Details were written to") expect(error.message).toBe(error.stderr) expect(error.stderr).not.toContain("secret") expect(error.stderr).not.toContain("command output") + // altimate_change end }), ) @@ -206,10 +212,16 @@ describe("installation", () => { Effect.gen(function* () { const error = yield* Effect.flip(Installation.use.upgrade("curl", "9.9.9")) expect(error).toBeInstanceOf(Installation.UpgradeFailedError) - expect(error.stderr).toBe("Upgrade failed for curl (exit code 1).") + // altimate_change start — #1305: the message now also points at the local log, + // where the REAL stderr is written. Redaction is what this test guards, so the + // not.toContain assertions below are the contract; the prefix is matched rather + // than compared exactly so the pointer can be appended. + expect(error.stderr).toContain("Upgrade failed for curl (exit code 1).") + expect(error.stderr).toContain("Details were written to") expect(error.message).toBe(error.stderr) expect(error.stderr).not.toContain("secret") expect(error.stderr).not.toContain("script output") + // altimate_change end }), ) diff --git a/packages/opencode/test/installation/resolve-install.test.ts b/packages/opencode/test/installation/resolve-install.test.ts new file mode 100644 index 000000000..a89c80887 --- /dev/null +++ b/packages/opencode/test/installation/resolve-install.test.ts @@ -0,0 +1,82 @@ +/** + * Install resolution (#1305). + * + * `resolveInstall()` answers "which install produced THIS process", replacing a + * substring test on execPath plus a probe loop that asked each package manager + * whether it had the package at all. The second question picks arbitrarily when more + * than one install exists, which is the common case once a user has tried both the + * curl installer and npm. + * + * These cases are table-driven over fabricated paths because the real layouts cannot + * be created on a test machine. + */ +import { describe, test, expect } from "bun:test" +import { resolveInstall, type Method } from "../../src/installation" + +const NPM_PREFIXED = "/usr/local/lib/node_modules/@altimateai/altimate-code" +const PLATFORM = "node_modules/@altimateai/altimate-code-darwin-arm64/bin/altimate-code" + +describe("resolveInstall", () => { + const cases: Array<[string, string, Method]> = [ + // The npm bin/altimate shim spawns the PLATFORM package, so execPath is the nested + // platform binary rather than the wrapper — detection must match the -- suffix. + ["npm, default prefix", `${NPM_PREFIXED}/${PLATFORM}`, "npm"], + // Regression: this is the layout the old `.local/bin` rule misread as "curl", which + // made `altimate upgrade` run `curl | bash` and orphan the npm install. + [ + "npm, prefix set to ~/.local", + "/home/u/.local/lib/node_modules/@altimateai/altimate-code/node_modules/@altimateai/altimate-code-linux-x64/bin/altimate-code", + "npm", + ], + [ + "pnpm, virtual store layout", + "/home/u/.local/share/pnpm/global/5/.pnpm/@altimateai+altimate-code@0.11.2/node_modules/@altimateai/altimate-code-linux-x64/bin/altimate-code", + "pnpm", + ], + [ + "pnpm, plain global link layout", + "/home/u/.local/share/pnpm/global/5/node_modules/@altimateai/altimate-code-linux-x64/bin/altimate-code", + "pnpm", + ], + [ + "bun global", + "/home/u/.bun/install/global/node_modules/@altimateai/altimate-code-linux-x64/bin/altimate-code", + "bun", + ], + ["yarn global", "/home/u/.yarn/global/node_modules/@altimateai/altimate-code-linux-x64/bin/altimate-code", "yarn"], + // Homebrew bin entries are symlinks into Cellar; realpath lands there. Matching the + // Cellar segment (not the prefix) keeps /usr/local from colliding with npm. + ["brew, apple silicon", "/opt/homebrew/Cellar/altimate-code/0.11.2/bin/altimate", "brew"], + ["brew, intel prefix", "/usr/local/Cellar/altimate-code/0.11.2/bin/altimate", "brew"], + ["standalone install", "/home/u/.altimate/bin/altimate", "curl"], + ["standalone, pre-v0.7.1 dir", "/home/u/.opencode/bin/altimate", "curl"], + ["scoop", "C:\\Users\\u\\scoop\\apps\\altimate-code\\current\\altimate.exe", "scoop"], + ["choco", "C:\\ProgramData\\chocolatey\\lib\\altimate-code\\tools\\altimate.exe", "choco"], + // A dev build or an unrecognised location must not be attributed to a package + // manager — "unknown" degrades to notify-only rather than running someone else's + // installer over it. + ["dev build", "/tmp/build/dist/altimate", "unknown"], + ] + + for (const [name, execPath, expected] of cases) { + test(`${name} -> ${expected}`, () => { + expect(resolveInstall(execPath, {}).method).toBe(expected) + }) + } + + test("a pinned ALTIMATE_CODE_BIN_PATH is never attributed to an installer", () => { + // The shim honours this ahead of everything else, so the running binary is whatever + // the user pointed at. Auto-upgrading it would overwrite a deliberate choice. + const env = { ALTIMATE_CODE_BIN_PATH: "/somewhere/custom/altimate" } + expect(resolveInstall(`${NPM_PREFIXED}/${PLATFORM}`, env).method).toBe("unknown") + }) + + test("standalone resolution reports the directory the upgrade would write", () => { + expect(resolveInstall("/home/u/.altimate/bin/altimate", {}).root).toBe("/home/u/.altimate/bin") + }) + + test("a plain user bin directory is not a standalone install", () => { + // `.local/bin` on its own carries no information about who installed the binary. + expect(resolveInstall("/home/u/.local/bin/altimate", {}).method).toBe("unknown") + }) +}) diff --git a/packages/opencode/test/release-validation/windows-installer-930.test.ts b/packages/opencode/test/release-validation/windows-installer-930.test.ts index 1a31bda53..b5e253ed1 100644 --- a/packages/opencode/test/release-validation/windows-installer-930.test.ts +++ b/packages/opencode/test/release-validation/windows-installer-930.test.ts @@ -60,9 +60,7 @@ function setPlatform(value: string) { Object.defineProperty(process, "platform", { value, configurable: true }) } -type HttpHandler = ( - request: HttpClientRequest.HttpClientRequest, -) => Response | Effect.Effect +type HttpHandler = (request: HttpClientRequest.HttpClientRequest) => Response | Effect.Effect type SpawnResult = string | { code: number; stdout?: string; stderr?: string } type SpawnCall = { cmd: string; args: readonly string[]; env?: Record; stdin?: unknown } @@ -115,9 +113,7 @@ function upgradeWith(input: { setPlatform(input.platform) const appProcess = AppProcess.layer.pipe(Layer.provide(mockSpawner(input.spawn))) const layer = Installation.layer.pipe( - Layer.provide( - mockHttpClient(input.http ?? (() => new Response("", { status: 200, statusText: "OK" }))), - ), + Layer.provide(mockHttpClient(input.http ?? (() => new Response("", { status: 200, statusText: "OK" })))), Layer.provide(appProcess), ) return Effect.runPromise(Installation.use.upgrade("curl", input.target ?? "1.2.3").pipe(Effect.provide(layer))) @@ -341,15 +337,25 @@ describe("upgradePowershell result shape is consumed by upgrade()", () => { // detect with instanceof (matches src/cli/cmd/upgrade.ts) rather than the removed .isInstance() static. expect(err instanceof Installation.UpgradeFailedError).toBe(true) // altimate_change end - expect((err as any).stderr).toBe("Upgrade failed for curl (exit code 1).") + // altimate_change start — #1305: message keeps the sanitized prefix and now also points + // at the local log, where the real installer stderr is written. + expect((err as any).stderr).toContain("Upgrade failed for curl (exit code 1).") + expect((err as any).stderr).toContain("Details were written to") + expect((err as any).stderr).not.toContain("powershell not found") + // altimate_change end // An error telemetry event was emitted carrying the sanitized stderr. expect(tracked).toHaveLength(1) expect(tracked[0].type).toBe("upgrade_attempted") expect(tracked[0].status).toBe("error") expect(tracked[0].to_version).toBe("1.2.3") - expect(tracked[0].error).toBe("Upgrade failed for curl (exit code 1).") + // altimate_change start — #1305: telemetry now carries a stable classification code + // plus the exit status instead of the generic message. The old value was identical for + // every failure, so causes could not be told apart on a dashboard. Redaction is + // unchanged — the installer's stderr still never reaches the event. + expect(tracked[0].error).toBe("unknown: exit 1") expect(tracked[0].error).not.toContain("powershell not found") + // altimate_change end }) }) @@ -446,7 +452,9 @@ describe("install.ps1 — GITHUB_PATH emission gated on GitHub Actions (static)" describe("install.ps1 — missing altimate.exe in archive fails + cleans up (static)", () => { test("throws 'Archive did not contain' when the extracted binary is absent", () => { // if (-not (Test-Path $extracted)) { throw "Archive did not contain $BinaryName" } - expect(PS1).toMatch(/if\s*\(-not\s*\(Test-Path\s+\$extracted\)\)\s*\{\s*throw\s+"Archive did not contain \$BinaryName"/) + expect(PS1).toMatch( + /if\s*\(-not\s*\(Test-Path\s+\$extracted\)\)\s*\{\s*throw\s+"Archive did not contain \$BinaryName"/, + ) }) test("the temp dir (altimate_install_$PID) is removed in a finally block", () => { From b9a769c4e3c8e72686eb5268f3ab58e59ddfa7c6 Mon Sep 17 00:00:00 2001 From: Sarav Date: Tue, 15 Sep 2026 09:17:50 +0530 Subject: [PATCH 2/9] fix: correct .local/bin regression and revert collateral reformat (#1305) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Self-review and CI turned up three problems with the previous commit. 1. The `.local/bin` claim was wrong, and removing the branch was a regression. The commit message and PR said `.local/bin` misclassified npm installs made with `npm config set prefix ~/.local`. It does not. With that prefix, packages land in `~/.local/lib/node_modules/...` and only the shim sits in `~/.local/bin`; since execPath is the spawned platform binary, it never contains `.local/bin` for a package-manager install, so the branch could not misfire that way. Removing it deleted correct back-compat from #820 (distro-resolved standalone installs), which test/sanity/Dockerfile also relies on, and broke four tests that said so explicitly. Restored — but AFTER the node_modules match, which is what makes it safe and is the real improvement over the original ordering. Both layouts now resolve correctly, with a test asserting exactly that. 2. Running prettier over the whole file reformatted code this change never touched (`upgradeCurl`, `upgradePowershell`, `defaultLayer`), because the committed file predates the repo's printWidth of 120. That broke two source-shape tests and tripped Marker Guard, which reads reformatted upstream lines as unmarked custom code. Formatting is not CI-enforced here, so it bought nothing. Rebuilt the file from the pristine version with only the intended edits re-applied. 3. `import { Global }` pulled in a module-load side effect: core/global.ts runs a top-level `await Promise.all([...mkdir...])`, creating seven directories merely by loading the module, and dragged that into every unit test importing resolveInstall(). Replaced with a lazy import matching the existing getTelemetry() pattern. Also converted the #820 detection tests from source-text assertions to behavioural ones now that resolveInstall() is pure, and documented that `access(W_OK)` reflects the read-only attribute rather than the ACL on Windows, so the preflight degrades to a no-op there instead of falsely blocking. Co-Authored-By: Claude Opus 5 (1M context) --- packages/opencode/src/installation/index.ts | 181 +++++++++++------- .../test/install/upgrade-method.test.ts | 12 +- .../test/installation/resolve-install.test.ts | 15 +- .../release-v0.7.1-binary-adversarial.test.ts | 36 +++- 4 files changed, 156 insertions(+), 88 deletions(-) diff --git a/packages/opencode/src/installation/index.ts b/packages/opencode/src/installation/index.ts index bb0a82d6b..df3ca9c0d 100644 --- a/packages/opencode/src/installation/index.ts +++ b/packages/opencode/src/installation/index.ts @@ -9,13 +9,22 @@ import { ChildProcess } from "effect/unstable/process" import { AppProcess } from "@opencode-ai/core/process" import path from "path" import fs from "fs" -import { Global } from "@opencode-ai/core/global" import { EventV2 } from "@opencode-ai/core/event" import { makeRuntime } from "@opencode-ai/core/effect/runtime" import semver from "semver" import { InstallationChannel, InstallationVersion } from "@opencode-ai/core/installation/version" import { NpmConfig } from "@opencode-ai/core/npm-config" +// altimate_change start — lazy log-dir lookup (#1305). @opencode-ai/core/global runs a +// top-level `await Promise.all([...mkdir...])`, so a static import would create seven +// directories merely by loading this module — and would drag that side effect into every +// unit test that imports resolveInstall(). Same lazy shape as getTelemetry() below. +async function getLogDir(): Promise { + const { Global } = await import("@opencode-ai/core/global") + return Global.Path.log +} +// altimate_change end + // altimate_change start — telemetry (lazy import to avoid circular dep with Telemetry → Installation) let _telemetryCache: (typeof import("../telemetry"))["Telemetry"] | undefined async function getTelemetry() { @@ -40,13 +49,15 @@ const UPGRADE_FETCH_TIMEOUT_MS = 15_000 // altimate_change end // altimate_change start — deterministic install resolution (#1305) -// Detection used to guess two ways, and both were unsound: a substring test on -// process.execPath (`.local/bin` is a generic user bin dir, so an npm install with -// `npm config set prefix ~/.local` was classified "curl" and upgraded via -// `curl | bash`, orphaning the npm copy), and a probe loop asking each package -// manager "do you have this package?" — which answers a different question than -// "did THIS running binary come from you", so it picked arbitrarily whenever more -// than one install existed. +// Detection used to fall back to a probe loop that asked each package manager "do you +// have this package?" (`npm list -g`, `brew list`, ...) and returned the first that said +// yes. That answers a different question than "did THIS running binary come from you", +// so once a user had more than one install — e.g. a standalone binary plus an npm copy, +// which is common — the answer was effectively arbitrary and upgrades were routed at an +// install the user was not running. +// +// The directory checks that ran before the probe loop were sound and are preserved +// below; only the probe loop is replaced. // // The running binary's own path is the ground truth. The npm `bin/altimate` shim is // a Node script that spawnSync()s the PLATFORM package's binary, so inside the CLI @@ -70,10 +81,15 @@ const YARN_SEGMENT_RE = /[\\/](?:\.yarn|yarn[\\/]global)[\\/]/i const BREW_SEGMENT_RE = /[\\/]Cellar[\\/]altimate-code[\\/]/i const SCOOP_SEGMENT_RE = /[\\/]scoop[\\/]apps[\\/]/i const CHOCO_SEGMENT_RE = /[\\/]chocolatey[\\/]/i -// The standalone (curl / install.ps1 / `install --binary`) layout. `.opencode/bin` is -// the pre-v0.7.1 directory name, kept for users who have not re-installed since. -// NOTE: `.local/bin` is deliberately NOT here — see the comment above. -const STANDALONE_SEGMENT_RE = /[\\/]\.(?:altimate|opencode)[\\/]bin[\\/]/i +// The standalone (curl / install.ps1 / `install --binary`) layouts. `.opencode/bin` is +// the pre-v0.7.1 directory name and `.local/bin` a distro-resolved location; both are +// kept for back-compat (#820) and `.local/bin` is also what test/sanity/Dockerfile uses. +// +// These are checked AFTER the node_modules match above, which is what makes them safe: +// a package-manager install's execPath is the spawned platform binary deep under +// `/lib/node_modules/...`, so it can never collide with `/bin` here even +// when the prefix is `~/.local`. +const STANDALONE_SEGMENT_RE = /[\\/]\.(?:altimate|opencode)[\\/]bin[\\/]|[\\/]\.local[\\/]bin[\\/]/i export interface ResolvedInstall { readonly method: Method @@ -117,6 +133,10 @@ function realExecPath(): string { } } +/** NOTE: on Windows, `access(W_OK)` reflects the read-only ATTRIBUTE rather than the ACL, + * so a directory the user genuinely cannot write can still report writable. That makes the + * preflight a no-op there rather than a false block — we fall through to the old behaviour + * (shell out, fail, report) instead of wrongly refusing an upgrade that would have worked. */ function isWritable(dir: string): boolean { try { fs.accessSync(dir, fs.constants.W_OK) @@ -285,7 +305,7 @@ export const layer: Layer.Layer - new UpgradeFailedError({ - stderr: - `Could not download install script from ${UPGRADE_INSTALL_URL}: ${errorMessage(err)}. ` + - `Re-run the install manually: curl -fsSL ${UPGRADE_INSTALL_URL} | bash — ` + - `or download a release binary directly from https://github.com/AltimateAI/altimate-code/releases/latest`, - }), - ), - ) - const body = yield* response.text.pipe( - Effect.mapError(() => new UpgradeFailedError({ stderr: upgradeFailure("curl") })), - ) - // altimate_change end - const bodyBytes = new TextEncoder().encode(body) - const shell = yield* upgradeScriptShell() - const result = yield* appProcess - .run( - ChildProcess.make(shell, [], { - stdin: Stream.make(bodyBytes), - env: { VERSION: target }, - extendEnv: true, - }), + const upgradeCurl = Effect.fnUntraced( + function* (target: string) { + // altimate_change start — friendly fetch error + manual-recovery hint, branded install URL, bounded timeout + const response = yield* httpOk + .execute(HttpClientRequest.get(UPGRADE_INSTALL_URL)) + .pipe( + Effect.timeout(UPGRADE_FETCH_TIMEOUT_MS), + Effect.mapError( + (err) => + new UpgradeFailedError({ + stderr: + `Could not download install script from ${UPGRADE_INSTALL_URL}: ${errorMessage(err)}. ` + + `Re-run the install manually: curl -fsSL ${UPGRADE_INSTALL_URL} | bash — ` + + `or download a release binary directly from https://github.com/AltimateAI/altimate-code/releases/latest`, + }), + ), + ) + const body = yield* response.text.pipe( + Effect.mapError(() => new UpgradeFailedError({ stderr: upgradeFailure("curl") })), ) - .pipe(Effect.mapError(() => new UpgradeFailedError({ stderr: upgradeFailure("curl") }))) - return { - code: result.exitCode, - stdout: result.stdout.toString("utf8"), - stderr: result.stderr.toString("utf8"), - } - }) + // altimate_change end + const bodyBytes = new TextEncoder().encode(body) + const shell = yield* upgradeScriptShell() + const result = yield* appProcess + .run( + ChildProcess.make(shell, [], { + stdin: Stream.make(bodyBytes), + env: { VERSION: target }, + extendEnv: true, + }), + ) + .pipe(Effect.mapError(() => new UpgradeFailedError({ stderr: upgradeFailure("curl") }))) + return { + code: result.exitCode, + stdout: result.stdout.toString("utf8"), + stderr: result.stderr.toString("utf8"), + } + }, + ) // altimate_change start — Windows curl-install upgrade via PowerShell // The curl/standalone install on native Windows lives in %USERPROFILE%\.altimate\bin @@ -411,18 +448,20 @@ export const layer: Layer.Layer - new UpgradeFailedError({ - stderr: - `Could not download install script from ${UPGRADE_INSTALL_PS_URL}: ${errorMessage(err)}. ` + - `Re-run the install manually: powershell -c "irm ${UPGRADE_INSTALL_PS_URL} | iex" — ` + - `or download a release binary directly from https://github.com/AltimateAI/altimate-code/releases/latest`, - }), - ), - ) + yield* httpOk + .execute(HttpClientRequest.head(UPGRADE_INSTALL_PS_URL)) + .pipe( + Effect.timeout(UPGRADE_FETCH_TIMEOUT_MS), + Effect.mapError( + (err) => + new UpgradeFailedError({ + stderr: + `Could not download install script from ${UPGRADE_INSTALL_PS_URL}: ${errorMessage(err)}. ` + + `Re-run the install manually: powershell -c "irm ${UPGRADE_INSTALL_PS_URL} | iex" — ` + + `or download a release binary directly from https://github.com/AltimateAI/altimate-code/releases/latest`, + }), + ), + ) return yield* run( ["powershell", "-NoProfile", "-ExecutionPolicy", "Bypass", "-Command", `irm ${UPGRADE_INSTALL_PS_URL} | iex`], { env: { VERSION: target } }, @@ -523,7 +562,8 @@ export const layer: Layer.Layer getLogDir()) const base = upgradeFailure(m, upgradeResult) const stderr = [ base, classified.hint ? `Likely cause: ${classified.hint}.` : undefined, - `Details were written to ${Global.Path.log}.`, + `Details were written to ${logDir}.`, ] .filter(Boolean) .join(" ") @@ -646,9 +691,7 @@ export const layer: Layer.Layer - layer.pipe(Layer.provide(FetchHttpClient.layer), Layer.provide(AppProcess.defaultLayer)), -) +export const defaultLayer = Layer.suspend(() => layer.pipe(Layer.provide(FetchHttpClient.layer), Layer.provide(AppProcess.defaultLayer))) // altimate_change end const { runPromise } = makeRuntime(Service, defaultLayer) diff --git a/packages/opencode/test/install/upgrade-method.test.ts b/packages/opencode/test/install/upgrade-method.test.ts index a4823a9cb..702956b4c 100644 --- a/packages/opencode/test/install/upgrade-method.test.ts +++ b/packages/opencode/test/install/upgrade-method.test.ts @@ -40,11 +40,13 @@ describe("installation method detection", () => { // altimate_change end }) - test("`.local/bin` is not treated as a standalone install", () => { - // altimate_change start — #1305: `.local/bin` is a generic user bin dir. Treating it - // as curl misrouted `npm config set prefix ~/.local` installs into `curl | bash`, - // which orphaned the npm copy and left two binaries fighting over PATH. - expect(INSTALLATION_SRC).not.toMatch(/path\.join\("\.local", "bin"\)/) + test("all three standalone directories are still recognised", () => { + // altimate_change start — #1305: the three curl-install directories from #820 + // (.altimate/bin, .opencode/bin, .local/bin) must all keep resolving to "curl". + // Behavioural coverage lives in test/installation/resolve-install.test.ts; this + // asserts the source still carries all three so a refactor cannot quietly drop one. + expect(INSTALLATION_SRC).toContain("altimate|opencode") + expect(INSTALLATION_SRC).toContain(".local") // altimate_change end }) }) diff --git a/packages/opencode/test/installation/resolve-install.test.ts b/packages/opencode/test/installation/resolve-install.test.ts index a89c80887..e766b9f8b 100644 --- a/packages/opencode/test/installation/resolve-install.test.ts +++ b/packages/opencode/test/installation/resolve-install.test.ts @@ -21,8 +21,9 @@ describe("resolveInstall", () => { // The npm bin/altimate shim spawns the PLATFORM package, so execPath is the nested // platform binary rather than the wrapper — detection must match the -- suffix. ["npm, default prefix", `${NPM_PREFIXED}/${PLATFORM}`, "npm"], - // Regression: this is the layout the old `.local/bin` rule misread as "curl", which - // made `altimate upgrade` run `curl | bash` and orphan the npm install. + // The prefix here is ~/.local, so packages land in ~/.local/lib/node_modules — NOT + // ~/.local/bin, which holds only the shim. That is why keeping the `.local/bin` + // standalone branch is safe: the two can never collide on execPath. [ "npm, prefix set to ~/.local", "/home/u/.local/lib/node_modules/@altimateai/altimate-code/node_modules/@altimateai/altimate-code-linux-x64/bin/altimate-code", @@ -50,6 +51,7 @@ describe("resolveInstall", () => { ["brew, intel prefix", "/usr/local/Cellar/altimate-code/0.11.2/bin/altimate", "brew"], ["standalone install", "/home/u/.altimate/bin/altimate", "curl"], ["standalone, pre-v0.7.1 dir", "/home/u/.opencode/bin/altimate", "curl"], + ["standalone, distro-resolved ~/.local/bin", "/home/u/.local/bin/altimate", "curl"], ["scoop", "C:\\Users\\u\\scoop\\apps\\altimate-code\\current\\altimate.exe", "scoop"], ["choco", "C:\\ProgramData\\chocolatey\\lib\\altimate-code\\tools\\altimate.exe", "choco"], // A dev build or an unrecognised location must not be attributed to a package @@ -75,8 +77,11 @@ describe("resolveInstall", () => { expect(resolveInstall("/home/u/.altimate/bin/altimate", {}).root).toBe("/home/u/.altimate/bin") }) - test("a plain user bin directory is not a standalone install", () => { - // `.local/bin` on its own carries no information about who installed the binary. - expect(resolveInstall("/home/u/.local/bin/altimate", {}).method).toBe("unknown") + test("a standalone binary in ~/.local/bin is still a curl install (#820 back-compat)", () => { + // Kept deliberately: it is a distro-resolved standalone location and is what + // test/sanity/Dockerfile installs to. Safe because the node_modules match runs first — + // see the npm-under-~/.local case above, which resolves to npm rather than here. + expect(resolveInstall("/home/u/.local/bin/altimate", {}).method).toBe("curl") + expect(resolveInstall("/home/u/.local/bin/altimate", {}).root).toBe("/home/u/.local/bin") }) }) diff --git a/packages/opencode/test/skill/release-v0.7.1-binary-adversarial.test.ts b/packages/opencode/test/skill/release-v0.7.1-binary-adversarial.test.ts index 5c32174e0..f72e9e169 100644 --- a/packages/opencode/test/skill/release-v0.7.1-binary-adversarial.test.ts +++ b/packages/opencode/test/skill/release-v0.7.1-binary-adversarial.test.ts @@ -14,6 +14,8 @@ import { describe, test, expect } from "bun:test" import { readFileSync } from "fs" import path from "path" +// altimate_change — #1305: behavioural assertions for the #820 curl-path contract +import { resolveInstall } from "../../src/installation" const PKG_DIR = path.resolve(import.meta.dir, "../..") const REPO_ROOT = path.resolve(PKG_DIR, "../..") @@ -29,22 +31,38 @@ describe("v0.7.1 PR #820 — installation method() upgrade-path detection", () = // P0 review finding: `altimate upgrade` after a v0.7.1 curl install must // identify the install method as "curl" so it picks the curl-upgrade path. // Pre-fix the detector only looked at `.opencode/bin` and `.local/bin`. + // altimate_change start — #1305: detection moved from three `process.execPath.includes( + // path.join(...))` branches to resolveInstall(), which is pure in (execPath, env). The + // #820 contract is unchanged — all three directories must still resolve to "curl" — so + // these now assert the BEHAVIOUR directly instead of the shape of the source, which is + // both stronger and no longer breaks on a refactor. test("detects new curl-install path .altimate/bin", () => { - expect(installationTs).toContain(`path.join(".altimate", "bin")`) + expect(resolveInstall("/home/u/.altimate/bin/altimate", {}).method).toBe("curl") }) test("retains .opencode/bin back-compat for pre-rename installs", () => { - expect(installationTs).toContain(`path.join(".opencode", "bin")`) + expect(resolveInstall("/home/u/.opencode/bin/altimate", {}).method).toBe("curl") }) test("retains .local/bin detection (distro-resolved path)", () => { - expect(installationTs).toContain(`path.join(".local", "bin")`) + expect(resolveInstall("/home/u/.local/bin/altimate", {}).method).toBe("curl") }) test("each curl-path branch returns the string \"curl\"", () => { - // Avoid a future regression where someone adds `.altimate` but accidentally - // returns "npm" or similar — the three branches must each return "curl". - const re = /process\.execPath\.includes\(path\.join\("\.[a-zA-Z]+", "bin"\)\)\) return "curl"/g - const matches = installationTs.match(re) ?? [] - expect(matches.length).toBeGreaterThanOrEqual(3) - }) + // Guards the original regression: someone adds a directory but returns "npm". + for (const dir of [".altimate", ".opencode", ".local"]) { + expect(resolveInstall(`/home/u/${dir}/bin/altimate`, {}).method).toBe("curl") + } + }) + test("a package-manager install under the same prefix is NOT curl", () => { + // The node_modules match runs first, which is what makes keeping `.local/bin` safe: + // an npm install with `npm config set prefix ~/.local` lives under + // `~/.local/lib/node_modules/...`, never `~/.local/bin/...`. + expect( + resolveInstall( + "/home/u/.local/lib/node_modules/@altimateai/altimate-code/node_modules/@altimateai/altimate-code-linux-x64/bin/altimate-code", + {}, + ).method, + ).toBe("npm") + }) + // altimate_change end }) describe("v0.7.1 PR #820 — install script: APP rename to altimate", () => { From d0cac981deda64cf94a19bfe89c214787c1b96de Mon Sep 17 00:00:00 2001 From: Sarav Date: Tue, 15 Sep 2026 09:29:49 +0530 Subject: [PATCH 3/9] =?UTF-8?q?fix:=20address=20review=20findings=20?= =?UTF-8?q?=E2=80=94=20non-global=20layouts,=20yarn=20on=20Windows,=20bloc?= =?UTF-8?q?ked-upgrade=20telemetry=20(#1305)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Automated review of #1306 raised six findings. Each was verified against the code before acting; five were valid and are fixed here, one is declined with a reason. - **npx caches, download caches and project-local installs were attributed to a package manager.** `PKG_SEGMENT_RE` matches any `node_modules/@altimateai/altimate-code*` segment, not only global roots, so `npx`, a devDependency install, or a bun/npm cache resolved to `npm`/`bun`. `upgrade()` reads that as "run `install -g`", and for patch releases it runs automatically at startup — creating a global install the user never had. The deleted probe loop returned "unknown" for these, so this was a regression. Cache layouts are now excluded during detection, and `preflight()` additionally confirms the running binary actually lives under the manager's global root, failing open when that root cannot be determined. - **yarn classic on Windows was misclassified as npm.** Its global directory is `%LOCALAPPDATA%\Yarn\config\global`, which neither the `.yarn` nor the `yarn/global` spelling matched — so an upgrade would have run `npm install -g` over a yarn install, producing exactly the orphaned second binary this change exists to prevent. - **Preflight-blocked upgrades emitted no telemetry and no log entry**, so the flagship permission case read as "no attempt" on dashboards — strictly worse than the previous behaviour, which at least ran the command and recorded an error. Blocked attempts are now logged and tracked with their classification. - **The curl preflight checked the wrong directory.** It used the running binary's own directory, but the install script always writes to `$HOME/.altimate/bin`, so a legacy `~/.opencode/bin` install could pass preflight while a different directory was upgraded. - **The Chocolatey elevation message contradicted the classified cause** — it was returned unconditionally, so a network failure was reported as an elevation problem alongside a conflicting "Likely cause" hint. It is now used only for permission failures. - **The npm remediation told Windows users to run `sudo`**, which does not exist there; those users are now pointed at an elevated shell. - The error message now names `opencode.log` rather than the log directory, which also holds trace jsonl and heap dumps. Declined: switching `fs.accessSync` to `FileSystem.FileSystem`. It is the documented preference, but threading that service through requires widening the layer's dependency type and every downstream composition (`defaultLayer`, `node`) — well outside the scope of this fix, and raw `fs` already has precedent in sibling modules (cli/welcome.ts). Co-Authored-By: Claude Opus 5 (1M context) --- packages/opencode/src/installation/index.ts | 91 ++++++++++++++++--- .../test/installation/resolve-install.test.ts | 32 +++++++ 2 files changed, 110 insertions(+), 13 deletions(-) diff --git a/packages/opencode/src/installation/index.ts b/packages/opencode/src/installation/index.ts index df3ca9c0d..6324edf62 100644 --- a/packages/opencode/src/installation/index.ts +++ b/packages/opencode/src/installation/index.ts @@ -9,6 +9,7 @@ import { ChildProcess } from "effect/unstable/process" import { AppProcess } from "@opencode-ai/core/process" import path from "path" import fs from "fs" +import os from "os" import { EventV2 } from "@opencode-ai/core/event" import { makeRuntime } from "@opencode-ai/core/effect/runtime" import semver from "semver" @@ -21,7 +22,9 @@ import { NpmConfig } from "@opencode-ai/core/npm-config" // unit test that imports resolveInstall(). Same lazy shape as getTelemetry() below. async function getLogDir(): Promise { const { Global } = await import("@opencode-ai/core/global") - return Global.Path.log + // The file logger writes to opencode.log inside this directory; the directory itself also + // holds trace jsonl and heap dumps, so naming the file saves the user a hunt. + return path.join(Global.Path.log, "opencode.log") } // altimate_change end @@ -75,7 +78,11 @@ const PKG_SEGMENT_RE = // the wrong manager. const PNPM_SEGMENT_RE = /[\\/](?:\.pnpm|pnpm)[\\/]/i const BUN_SEGMENT_RE = /[\\/]\.bun[\\/]/i -const YARN_SEGMENT_RE = /[\\/](?:\.yarn|yarn[\\/]global)[\\/]/i +// yarn classic's global dir is `~/.yarn` / `.../yarn/global` on unix but +// `%LOCALAPPDATA%\Yarn\config\global` on Windows — match any `yarn` path segment so the +// Windows spelling is not silently attributed to npm (which would `npm install -g` over a +// yarn install and create the orphaned second binary this change exists to prevent). +const YARN_SEGMENT_RE = /[\\/]\.?yarn[\\/]/i // Homebrew bin entries are symlinks into Cellar, so realpath lands there. Match the // Cellar segment rather than the prefix: /usr/local is also a common npm prefix. const BREW_SEGMENT_RE = /[\\/]Cellar[\\/]altimate-code[\\/]/i @@ -91,6 +98,15 @@ const CHOCO_SEGMENT_RE = /[\\/]chocolatey[\\/]/i // when the prefix is `~/.local`. const STANDALONE_SEGMENT_RE = /[\\/]\.(?:altimate|opencode)[\\/]bin[\\/]|[\\/]\.local[\\/]bin[\\/]/i +// An npx cache, a package-manager download cache, or a project-local node_modules all +// contain a `node_modules/@altimateai/altimate-code*` segment but are NOT global installs. +// Attributing them to a package manager would make `upgrade()` run `npm install -g` and +// CREATE a global install the user never had — and for patch releases that happens +// automatically at startup (see cli/upgrade.ts). The old probe loop returned "unknown" +// for these, so they must keep degrading to notify-only. +const PACKAGE_MANAGERS: Method[] = ["npm", "pnpm", "bun", "yarn"] +const EPHEMERAL_SEGMENT_RE = /[\\/](?:_npx|_cacache)[\\/]|[\\/]install[\\/]cache[\\/]/i + export interface ResolvedInstall { readonly method: Method /** Directory the upgrade would mutate. Only set where we can name it without a subprocess. */ @@ -110,7 +126,7 @@ export function resolveInstall( // Never auto-upgrade a pinned path. if (env["ALTIMATE_CODE_BIN_PATH"]) return { method: "unknown" } - if (PKG_SEGMENT_RE.test(execPath)) { + if (PKG_SEGMENT_RE.test(execPath) && !EPHEMERAL_SEGMENT_RE.test(execPath)) { if (PNPM_SEGMENT_RE.test(execPath)) return { method: "pnpm" } if (BUN_SEGMENT_RE.test(execPath)) return { method: "bun" } if (YARN_SEGMENT_RE.test(execPath)) return { method: "yarn" } @@ -298,7 +314,13 @@ export const layer: Layer.Layer { - if (method === "choco") return "not running from an elevated command shell" + // altimate_change start — #1305: only claim elevation when the failure actually looks + // like a permission problem. Returning it unconditionally contradicted the classified + // "Likely cause:" hint appended by the caller (e.g. a network failure reported as an + // elevation problem). + if (method === "choco" && (!result || classifyFailure(result.stderr, result.stdout).code === "permission")) + return "not running from an elevated command shell" + // altimate_change end // altimate_change start — do not echo package-manager/install-script stderr; it can contain tokens or env if (result) return `Upgrade failed for ${method} (exit code ${result.code}).` // altimate_change end @@ -336,8 +358,11 @@ export const layer: Layer.Layer ({ code, message }) + /** Returns an error message when the upgrade cannot possibly succeed, else undefined. * * Checking first means we never shell out to a command that is going to fail on @@ -383,12 +415,28 @@ export const layer: Layer.Layer 0) { + const exec = realExecPath().toLowerCase() + const roots = dirs.map((d) => d.toLowerCase()).filter(Boolean) + if (roots.length > 0 && !roots.some((r) => exec.startsWith(r))) { + return preflightBlock("not-global", + `The running binary is not the ${m} global install (${realExecPath()}). ` + + `Upgrade it where it was installed from, or install globally with ` + + `\`${m} install -g @altimateai/altimate-code@${target}\`.`, + ) + } + } for (const dir of dirs) { if (!dir) continue // A directory that does not exist yet is not a permission problem: the package // manager creates it. Only an EXISTING, unwritable directory is a hard stop. if (!fs.existsSync(dir)) continue - if (!isWritable(dir)) return remediation(m, dir, target) + if (!isWritable(dir)) return preflightBlock("permission", remediation(m, dir, target)) } return undefined }) @@ -556,7 +604,24 @@ export const layer: Layer.Layer getTelemetry()) + T0.track({ + type: "upgrade_attempted", + timestamp: Date.now(), + session_id: T0.getContext().sessionId || "cli", + from_version: InstallationVersion, + to_version: target, + method: (["npm", "bun", "brew"].includes(m) ? m : "other") as "npm" | "bun" | "brew" | "other", + status: "error", + error: `${blocked.code}: preflight`, + }) + return yield* new UpgradeFailedError({ stderr: blocked.message }) + } // altimate_change end let upgradeResult: { code: number; stdout: string; stderr: string } | undefined switch (m) { diff --git a/packages/opencode/test/installation/resolve-install.test.ts b/packages/opencode/test/installation/resolve-install.test.ts index e766b9f8b..d0557d59a 100644 --- a/packages/opencode/test/installation/resolve-install.test.ts +++ b/packages/opencode/test/installation/resolve-install.test.ts @@ -77,6 +77,38 @@ describe("resolveInstall", () => { expect(resolveInstall("/home/u/.altimate/bin/altimate", {}).root).toBe("/home/u/.altimate/bin") }) + // Review findings on #1306 — layouts that contain a package segment but are NOT a + // global install. Attributing them to a manager would make upgrade() run `install -g` + // and CREATE a global install the user never had (automatically, for patch releases). + test("an npx cache invocation is not attributed to npm", () => { + expect( + resolveInstall( + "/home/u/.npm/_npx/a1b2c3/node_modules/@altimateai/altimate-code-linux-x64/bin/altimate-code", + {}, + ).method, + ).toBe("unknown") + }) + + test("a package-manager download cache is not attributed to a manager", () => { + expect( + resolveInstall( + "/home/u/.bun/install/cache/@altimateai/altimate-code-linux-x64/bin/altimate-code", + {}, + ).method, + ).toBe("unknown") + }) + + test("yarn classic on Windows is yarn, not npm", () => { + // %LOCALAPPDATA%\Yarn\config\global — the unix `.yarn` / `yarn/global` spellings do + // not cover it, and falling through to npm would `npm install -g` over a yarn install. + expect( + resolveInstall( + "C:\\Users\\u\\AppData\\Local\\Yarn\\config\\global\\node_modules\\@altimateai\\altimate-code-win32-x64\\bin\\altimate-code.exe", + {}, + ).method, + ).toBe("yarn") + }) + test("a standalone binary in ~/.local/bin is still a curl install (#820 back-compat)", () => { // Kept deliberately: it is a distro-resolved standalone location and is what // test/sanity/Dockerfile installs to. Safe because the node_modules match runs first — From 1ef59166a25e9aa66fabf9e141ee97c1e91e0570 Mon Sep 17 00:00:00 2001 From: Sarav Date: Tue, 15 Sep 2026 10:10:51 +0530 Subject: [PATCH 4/9] fix: establish ownership before acting; stop leaking diagnostics; correct package identities (#1305) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three-model consensus review plus cubic/CodeRabbit found nine issues in the previous round, five of them introduced by this branch. Each was verified against the code first. **Bun global upgrades were refused outright.** `bun pm bin -g` reports the SHIM directory (~/.bun/bin) while packages live in a sibling tree (~/.bun/install/global/node_modules). The ownership check treated the shim dir as the package root, so the real executable was never "inside" it. `globalLayout()` now returns `packageRoot` and `writable` separately — ownership is decided against the package tree, permissions against what the upgrade writes. **Ownership is now established before any consumer receives an actionable identity.** A path match is a hypothesis: a project-local node_modules is shaped exactly like a global one. `Installation.method()` confirms it with the manager and downgrades to `unknown` when the running binary is not in that manager's global tree. This matters because `cli/cmd/uninstall.ts` acts on the answer destructively and never runs the upgrade preflight. It also stops us mutating the wrong tree when a different `npm` is first on PATH — its `npm root -g` will not contain our executable, so we refuse rather than upgrade someone else's install. **Containment was unsound in both directions.** The lowercased `startsWith` matched `/prefix/lib/node_modules-other` against `/prefix/lib/node_modules`, resolved symlinks on only one side, and mis-compared on case-sensitive filesystems. Replaced with a separator-aware `path.relative` check. Its own test then caught a further bug: resolving only paths that exist compares /var against /private/var, so `realpathOr` now resolves the deepest existing ancestor and re-appends the remainder. **Diagnostics are redacted before they reach any sink.** The previous round logged package-manager stdout/stderr verbatim, justified by the log file staying local. That was wrong: `Logging.loggers()` adds a stderr logger under OPENCODE_PRINT_LOGS=1, and `Otlp.loggers()` ships records to a remote collector when OTEL_EXPORTER_OTLP_ENDPOINT is set — neither redacts, and npm error output routinely carries registry `_authToken` values. The message also no longer promises a log artifact that an ERROR log level would discard. **scoop/choco no longer resolve to an actionable method.** `latest()`/`upgrade()` still query and install the upstream `opencode` package, so an Altimate install resolving to those methods would pull in a different package. The old probe loop self-limited by requiring `scoop list opencode` to match; path matching has no such guard. Notify-only until those commands carry Altimate identities. **`uninstall` targeted upstream packages.** It ran `npm uninstall -g opencode-ai` and `brew uninstall opencode`, able to remove an unrelated upstream install while leaving Altimate in place. Pre-existing, but widened by this branch. **`yarn` is rejected where it was unhandled.** `Installation.upgrade()` has no `yarn` case; `cli/upgrade.ts` already routed it to notify, but `cli/cmd/upgrade.ts` and the HTTP upgrade route guarded only `unknown` and would have surfaced an opaque failure. Also: narrowed the pnpm/yarn patterns to real layouts instead of any `pnpm`/`yarn` path segment; excluded `dlx` caches alongside npx; renamed `ResolvedInstall.root` to `binDir` with an accurate description of what it holds. **Tests.** A previous guard asserted `INSTALLATION_SRC.toContain(".local")` against the whole file, which cannot detect the regression it claims to prevent — `.local` appears in three nearby comments, so deleting the regex alternation left it green. It now asserts against the regex line, verified by simulating the removal and watching it fail. Added ownership/containment coverage for the bun layout, prefix-sibling rejection, symlinked parents, and non-existent paths. Co-Authored-By: Claude Opus 5 (1M context) --- packages/opencode/src/cli/cmd/uninstall.ts | 33 +-- packages/opencode/src/cli/cmd/upgrade.ts | 6 +- packages/opencode/src/installation/index.ts | 235 +++++++++++++----- packages/opencode/src/server/routes/global.ts | 8 +- .../test/install/upgrade-method.test.ts | 16 +- .../test/installation/ownership.test.ts | 76 ++++++ .../test/installation/resolve-install.test.ts | 11 +- 7 files changed, 297 insertions(+), 88 deletions(-) create mode 100644 packages/opencode/test/installation/ownership.test.ts diff --git a/packages/opencode/src/cli/cmd/uninstall.ts b/packages/opencode/src/cli/cmd/uninstall.ts index 935967cbe..2d7378039 100644 --- a/packages/opencode/src/cli/cmd/uninstall.ts +++ b/packages/opencode/src/cli/cmd/uninstall.ts @@ -130,15 +130,18 @@ async function showRemovalSummary(targets: RemovalTargets, method: Installation. } if (method !== "curl" && method !== "unknown") { + // altimate_change start — #1305: these targeted upstream's `opencode-ai` / `opencode`, + // so an uninstall could remove an unrelated upstream package while leaving Altimate + // installed. scoop/choco are omitted: Installation.method() no longer returns them + // (their commands still reference upstream identities), so they are unreachable here. const cmds: Record = { - npm: "npm uninstall -g opencode-ai", - pnpm: "pnpm uninstall -g opencode-ai", - bun: "bun remove -g opencode-ai", - yarn: "yarn global remove opencode-ai", - brew: "brew uninstall opencode", - choco: "choco uninstall opencode", - scoop: "scoop uninstall opencode", + npm: "npm uninstall -g @altimateai/altimate-code", + pnpm: "pnpm uninstall -g @altimateai/altimate-code", + bun: "bun remove -g @altimateai/altimate-code", + yarn: "yarn global remove @altimateai/altimate-code", + brew: "brew uninstall altimate-code", } + // altimate_change end prompts.log.info(` ✓ Package: ${cmds[method] || method}`) } } @@ -181,20 +184,20 @@ async function executeUninstall(method: Installation.Method, targets: RemovalTar } if (method !== "curl" && method !== "unknown") { + // altimate_change start — #1305: Altimate package identities, not upstream's. const cmds: Record = { - npm: ["npm", "uninstall", "-g", "opencode-ai"], - pnpm: ["pnpm", "uninstall", "-g", "opencode-ai"], - bun: ["bun", "remove", "-g", "opencode-ai"], - yarn: ["yarn", "global", "remove", "opencode-ai"], - brew: ["brew", "uninstall", "opencode"], - choco: ["choco", "uninstall", "opencode"], - scoop: ["scoop", "uninstall", "opencode"], + npm: ["npm", "uninstall", "-g", "@altimateai/altimate-code"], + pnpm: ["pnpm", "uninstall", "-g", "@altimateai/altimate-code"], + bun: ["bun", "remove", "-g", "@altimateai/altimate-code"], + yarn: ["yarn", "global", "remove", "@altimateai/altimate-code"], + brew: ["brew", "uninstall", "altimate-code"], } + // altimate_change end const cmd = cmds[method] if (cmd) { spinner.start(`Running ${cmd.join(" ")}...`) - const result = await Process.run(method === "choco" ? ["choco", "uninstall", "opencode", "-y", "-r"] : cmd, { + const result = await Process.run(cmd, { nothrow: true, }) if (result.code !== 0) { diff --git a/packages/opencode/src/cli/cmd/upgrade.ts b/packages/opencode/src/cli/cmd/upgrade.ts index 4b935b6fa..e1a12da26 100644 --- a/packages/opencode/src/cli/cmd/upgrade.ts +++ b/packages/opencode/src/cli/cmd/upgrade.ts @@ -46,7 +46,11 @@ export const UpgradeCommand = { // altimate_change end const detectedMethod = await Installation.method() const method = (args.method as Installation.Method) ?? detectedMethod - if (method === "unknown") { + // altimate_change start — #1305: Installation.upgrade()'s switch has no `yarn` case, so + // yarn reaches `default` and dies with "Unknown installation method: yarn". cli/upgrade.ts + // already routes yarn to notify for the same reason; this is the explicit-command path. + if (method === "unknown" || method === "yarn") { + // altimate_change end // altimate_change start — branding prompts.log.error(`altimate is installed to ${process.execPath} and may be managed by a package manager`) // altimate_change end diff --git a/packages/opencode/src/installation/index.ts b/packages/opencode/src/installation/index.ts index 6324edf62..edee16a2d 100644 --- a/packages/opencode/src/installation/index.ts +++ b/packages/opencode/src/installation/index.ts @@ -20,7 +20,10 @@ import { NpmConfig } from "@opencode-ai/core/npm-config" // top-level `await Promise.all([...mkdir...])`, so a static import would create seven // directories merely by loading this module — and would drag that side effect into every // unit test that imports resolveInstall(). Same lazy shape as getTelemetry() below. -async function getLogDir(): Promise { +async function getLogFile(): Promise { + // Our record is logged at WARN, which an ERROR minimum level drops (see + // Logging.minimumLogLevel) — do not promise an artifact that was never written. + if (process.env["OPENCODE_LOG_LEVEL"]?.toUpperCase() === "ERROR") return undefined const { Global } = await import("@opencode-ai/core/global") // The file logger writes to opencode.log inside this directory; the directory itself also // holds trace jsonl and heap dumps, so naming the file saves the user a hunt. @@ -76,13 +79,14 @@ const PKG_SEGMENT_RE = // plain `pnpm/global/` link path (no `.pnpm` segment), so match both spellings — // otherwise the plain layout falls through to the npm default and routes upgrades at // the wrong manager. -const PNPM_SEGMENT_RE = /[\\/](?:\.pnpm|pnpm)[\\/]/i +const PNPM_SEGMENT_RE = /[\\/](?:\.pnpm|pnpm[\\/]global)[\\/]/i const BUN_SEGMENT_RE = /[\\/]\.bun[\\/]/i -// yarn classic's global dir is `~/.yarn` / `.../yarn/global` on unix but -// `%LOCALAPPDATA%\Yarn\config\global` on Windows — match any `yarn` path segment so the -// Windows spelling is not silently attributed to npm (which would `npm install -g` over a -// yarn install and create the orphaned second binary this change exists to prevent). -const YARN_SEGMENT_RE = /[\\/]\.?yarn[\\/]/i +// yarn classic's global dir is `~/.yarn` / `.../yarn/global` on unix and +// `%LOCALAPPDATA%\Yarn\config\global` on Windows; berry uses `.yarn`. Enumerate those +// layouts rather than matching any `yarn` path segment — a bare segment match let an +// unrelated ancestor directory named `yarn` decide the manager, which is the same +// path-is-identity mistake this change exists to remove. +const YARN_SEGMENT_RE = /[\\/](?:\.yarn|yarn[\\/](?:global|config[\\/]global|berry))[\\/]/i // Homebrew bin entries are symlinks into Cellar, so realpath lands there. Match the // Cellar segment rather than the prefix: /usr/local is also a common npm prefix. const BREW_SEGMENT_RE = /[\\/]Cellar[\\/]altimate-code[\\/]/i @@ -105,18 +109,25 @@ const STANDALONE_SEGMENT_RE = /[\\/]\.(?:altimate|opencode)[\\/]bin[\\/]|[\\/]\. // automatically at startup (see cli/upgrade.ts). The old probe loop returned "unknown" // for these, so they must keep degrading to notify-only. const PACKAGE_MANAGERS: Method[] = ["npm", "pnpm", "bun", "yarn"] -const EPHEMERAL_SEGMENT_RE = /[\\/](?:_npx|_cacache)[\\/]|[\\/]install[\\/]cache[\\/]/i +const EPHEMERAL_SEGMENT_RE = /[\\/](?:_npx|_cacache|dlx|\.npx)[\\/]|[\\/]install[\\/]cache[\\/]|[\\/]dlx-[^\\/]+[\\/]/i export interface ResolvedInstall { readonly method: Method - /** Directory the upgrade would mutate. Only set where we can name it without a subprocess. */ - readonly root?: string + /** Directory the running binary sits in, for standalone layouts only. + * + * NOT the upgrade target: the install script always writes $HOME/.altimate/bin, so for a + * legacy ~/.opencode/bin or ~/.local/bin install these differ — which is why globalLayout() + * names the real target rather than reading this. */ + readonly binDir?: string } -/** Resolve the install that produced THIS process. +/** Resolve a CANDIDATE install identity for THIS process from its path. * - * Pure in (execPath, env) so it can be unit-tested against fabricated layouts - * without spawning real installs. */ + * Pure in (execPath, env) so it can be unit-tested against fabricated layouts without + * spawning real installs — which is also its limit: a path cannot prove global ownership. + * A project-local node_modules is shaped exactly like a global one, so a package-manager + * answer here is a hypothesis. `Installation.method()` confirms it with the manager before + * any consumer receives an actionable identity. */ export function resolveInstall( execPath: string = realExecPath(), env: NodeJS.ProcessEnv = process.env, @@ -133,9 +144,14 @@ export function resolveInstall( return { method: "npm" } } if (BREW_SEGMENT_RE.test(execPath)) return { method: "brew" } - if (SCOOP_SEGMENT_RE.test(execPath)) return { method: "scoop" } - if (CHOCO_SEGMENT_RE.test(execPath)) return { method: "choco" } - if (STANDALONE_SEGMENT_RE.test(execPath)) return { method: "curl", root: path.dirname(execPath) } + // scoop / choco are deliberately NOT returned here. `latest()` and `upgrade()` still query + // and install the upstream `opencode` package (see the scoop/choco cases below), so + // resolving an Altimate install to those methods would install a DIFFERENT, upstream + // package alongside it. The old probe loop self-limited because it required + // `scoop list opencode` to match; path matching has no such guard. Returning "unknown" + // degrades to notify-only until those commands use Altimate package identities. + if (SCOOP_SEGMENT_RE.test(execPath) || CHOCO_SEGMENT_RE.test(execPath)) return { method: "unknown" } + if (STANDALONE_SEGMENT_RE.test(execPath)) return { method: "curl", binDir: path.dirname(execPath) } return { method: "unknown" } } @@ -149,6 +165,55 @@ function realExecPath(): string { } } +/** bun's global PACKAGE root, derived from its shim directory. + * + * `bun pm bin -g` reports ~/.bun/bin, but globally installed packages live in a sibling + * tree at ~/.bun/install/global/node_modules. Treating the shim dir as the package root + * made every bun global install fail the ownership check as "not-global". */ +export function bunGlobalRoot(bin: string): string { + return bin ? path.join(path.dirname(bin), "install", "global", "node_modules") : "" +} + +/** Resolve symlinks, tolerating paths that do not exist yet. + * + * Resolving only the paths that exist is not enough: on macOS a tmp/home path realpaths + * from /var to /private/var, so comparing a resolved parent against an UNresolved child + * reports "not inside" for a path that plainly is. Resolve the deepest existing ancestor + * and re-append the remainder so both sides land in the same namespace. */ +function realpathOr(p: string): string { + try { + return fs.realpathSync(p) + } catch { + // fall through to the ancestor walk + } + const rest: string[] = [] + let cur = p + for (;;) { + const parent = path.dirname(cur) + if (parent === cur) return p + rest.unshift(path.basename(cur)) + cur = parent + try { + return path.join(fs.realpathSync(cur), ...rest) + } catch { + // keep walking up + } + } +} + +/** Separator-aware, symlink-resolved containment. + * + * Replaces a lowercased `startsWith`, which was wrong three ways: it matched + * `/prefix/lib/node_modules-other` against `/prefix/lib/node_modules`, it resolved symlinks + * on only one side (so a symlinked prefix — /var vs /private/var, nvm, asdf — falsely failed), + * and lowercasing produced false matches on case-sensitive filesystems. `path.relative` + * handles separators and platform case rules for us. */ +export function isInside(child: string, parent: string): boolean { + if (!parent || !child) return false + const rel = path.relative(realpathOr(parent), realpathOr(child)) + return rel === "" || (!rel.startsWith("..") && !path.isAbsolute(rel)) +} + /** NOTE: on Windows, `access(W_OK)` reflects the read-only ATTRIBUTE rather than the ACL, * so a directory the user genuinely cannot write can still report writable. That makes the * preflight a no-op there rather than a false block — we fall through to the old behaviour @@ -162,6 +227,27 @@ function isWritable(dir: string): boolean { } } +/** Mask credential-shaped substrings before diagnostics reach ANY log sink. + * + * The earlier version logged package-manager stdout/stderr verbatim on the reasoning that + * the log file stays on the machine. That is false: `Logging.loggers()` adds a stderr + * logger when OPENCODE_PRINT_LOGS=1, and `Otlp.loggers()` ships log records to a remote + * collector whenever OTEL_EXPORTER_OTLP_ENDPOINT is set — neither redacts. npm/pnpm/yarn + * error output routinely carries registry `_authToken` values and credentialed URLs. + * + * Conservative by design: over-masking a diagnostic is cheap, leaking a token is not. */ +function redactSecrets(input: string): string { + if (!input) return input + return input + .replace(/(bearer\s+)\S+/gi, "$1[REDACTED]") + .replace( + /((?:auth[-_]?token|authorization|api[-_]?key|access[-_]?token|password|passwd|secret|token)\s*[:=]\s*)(["']?)[^\s"',}]+/gi, + "$1$2[REDACTED]", + ) + .replace(/(https?:\/\/)[^\s/@]+:[^\s/@]+@/gi, "$1[REDACTED]@") + .replace(/\b[0-9a-f]{32,}\b/gi, "[REDACTED]") +} + /** Classify a failed upgrade into a stable code plus a message safe to show. * * Deliberately does NOT echo the package manager's stderr — it can carry tokens and @@ -329,47 +415,72 @@ export const layer: Layer.Layer/node_modules and the shims at itself, so the Unix + // `npm root -g` is the portable package dir: on Windows packages live at + // /node_modules and the shims at itself, so the unix // /lib/node_modules is wrong there. `npm bin -g` was REMOVED in npm 9 // ("Unknown command: bin"), so derive the bin dir from the prefix instead. const root = (yield* text(["npm", "root", "-g"])).trim() const prefix = (yield* text(["npm", "prefix", "-g"])).trim() const bin = prefix ? (process.platform === "win32" ? prefix : path.join(prefix, "bin")) : "" - return [root, bin].filter(Boolean) + return { packageRoot: root, writable: [root, bin].filter(Boolean) } } case "pnpm": { - // Both: a global install writes the store root AND the shim dir; checking only - // one lets the other fail with EACCES after we have already shelled out. const root = (yield* text(["pnpm", "root", "-g"])).trim() const bin = (yield* text(["pnpm", "bin", "-g"])).trim() - return [root, bin].filter(Boolean) + return { packageRoot: root, writable: [root, bin].filter(Boolean) } } case "bun": { const bin = (yield* text(["bun", "pm", "bin", "-g"])).trim() - return [bin].filter(Boolean) + return { packageRoot: bunGlobalRoot(bin), writable: [bunGlobalRoot(bin), bin].filter(Boolean) } } case "yarn": { + // `yarn global dir` is the folder holding package.json + node_modules, not the + // packages themselves. const dir = (yield* text(["yarn", "global", "dir"])).trim() + const root = dir ? path.join(dir, "node_modules") : "" const bin = (yield* text(["yarn", "global", "bin"])).trim() - return [dir, bin].filter(Boolean) - } - case "curl": { - // The install script always writes to $HOME/.altimate/bin regardless of where the - // running binary sits (`install`, INSTALL_DIR), so a legacy ~/.opencode/bin install - // must have the ACTUAL target checked — not its own directory, which the upgrade - // never touches. - return [path.join(os.homedir(), ".altimate", "bin")] + return { packageRoot: root, writable: [root, bin].filter(Boolean) } } - // brew / scoop / choco own their own elevation and policy — do not second-guess them. + case "curl": + // The install script always writes $HOME/.altimate/bin regardless of where the + // running binary sits (`install`, INSTALL_DIR), so a legacy ~/.opencode/bin or + // ~/.local/bin install must have the ACTUAL target checked. + return { packageRoot: "", writable: [path.join(os.homedir(), ".altimate", "bin")] } + // brew owns its own elevation and policy — do not second-guess it. default: - return [] as string[] + return empty } }) + /** Confirm the running binary really belongs to `m`'s global tree. + * + * Returns "unverifiable" when the manager cannot answer (not installed, command failed) + * so callers can decide — we never block on a missing answer. + * + * This is also what stops us mutating the WRONG tree: if a different `npm` is first on + * PATH, `npm root -g` describes that npm, the running binary is not inside it, and we + * refuse instead of upgrading someone else's global install. */ + const ownsRunningBinary = Effect.fnUntraced(function* (m: Method) { + if (!PACKAGE_MANAGERS.includes(m)) return "unverifiable" as const + const layout = yield* globalLayout(m) + if (!layout.packageRoot) return "unverifiable" as const + return isInside(realExecPath(), layout.packageRoot) ? ("owned" as const) : ("foreign" as const) + }) + const remediation = (m: Method, dir: string, target: string) => { const pkg = `@altimateai/altimate-code@${target}` switch (m) { @@ -414,24 +525,20 @@ export const layer: Layer.Layer 0) { - const exec = realExecPath().toLowerCase() - const roots = dirs.map((d) => d.toLowerCase()).filter(Boolean) - if (roots.length > 0 && !roots.some((r) => exec.startsWith(r))) { - return preflightBlock("not-global", - `The running binary is not the ${m} global install (${realExecPath()}). ` + - `Upgrade it where it was installed from, or install globally with ` + - `\`${m} install -g @altimateai/altimate-code@${target}\`.`, - ) - } + // Ownership first: a project-local node_modules, or a global tree belonging to a + // DIFFERENT manager binary that happens to be first on PATH, must never be mutated. + // "unverifiable" fails open — we do not block on a missing answer. + const ownership = yield* ownsRunningBinary(m) + if (ownership === "foreign") { + return preflightBlock( + "not-global", + `The running binary is not part of the ${m} global installation ` + + `(${realExecPath()}). Upgrade it where it was installed from, or install it ` + + `globally with \`${m} install -g @altimateai/altimate-code@${target}\`.`, + ) } - for (const dir of dirs) { + const layout = yield* globalLayout(m) + for (const dir of layout.writable) { if (!dir) continue // A directory that does not exist yet is not a permission problem: the package // manager creates it. Only an EXISTING, unwritable directory is a hard stop. @@ -526,11 +633,19 @@ export const layer: Layer.Layer getLogDir()) + const logFile = yield* Effect.promise(() => getLogFile()) const base = upgradeFailure(m, upgradeResult) const stderr = [ base, classified.hint ? `Likely cause: ${classified.hint}.` : undefined, - `Details were written to ${logDir}.`, + logFile ? `Details were written to ${logFile}.` : undefined, ] .filter(Boolean) .join(" ") diff --git a/packages/opencode/src/server/routes/global.ts b/packages/opencode/src/server/routes/global.ts index 78c6557d5..80f5bebc7 100644 --- a/packages/opencode/src/server/routes/global.ts +++ b/packages/opencode/src/server/routes/global.ts @@ -309,9 +309,13 @@ export const GlobalRoutes = lazy(() => ), async (c) => { const method = await Installation.method() - if (method === "unknown") { - return c.json({ success: false, error: "Unknown installation method" }, 400) + // altimate_change start — #1305: `yarn` has no case in Installation.upgrade()'s switch, + // so it would reach `default` and surface as an opaque failure. Reject it up front the + // same way `unknown` is rejected. + if (method === "unknown" || method === "yarn") { + return c.json({ success: false, error: `Unsupported installation method: ${method}` }, 400) } + // altimate_change end // altimate_change start — upstream_fix: branch/dev builds have no published release, so an // implicit Installation.latest() builds a non-existent npm dist-tag URL (channel = git branch // name) and 404s — and latest() is Effect.orDie, so this handler throws → opaque 500. Return a diff --git a/packages/opencode/test/install/upgrade-method.test.ts b/packages/opencode/test/install/upgrade-method.test.ts index 702956b4c..f351f400b 100644 --- a/packages/opencode/test/install/upgrade-method.test.ts +++ b/packages/opencode/test/install/upgrade-method.test.ts @@ -41,12 +41,16 @@ describe("installation method detection", () => { }) test("all three standalone directories are still recognised", () => { - // altimate_change start — #1305: the three curl-install directories from #820 - // (.altimate/bin, .opencode/bin, .local/bin) must all keep resolving to "curl". - // Behavioural coverage lives in test/installation/resolve-install.test.ts; this - // asserts the source still carries all three so a refactor cannot quietly drop one. - expect(INSTALLATION_SRC).toContain("altimate|opencode") - expect(INSTALLATION_SRC).toContain(".local") + // altimate_change start — #1305. An earlier version of this test asserted + // INSTALLATION_SRC.toContain(".local") against the WHOLE FILE, which cannot detect the + // regression it claims to guard: `.local` appears in three nearby comments, so deleting + // the alternation from STANDALONE_SEGMENT_RE left it green. Assert against the regex + // LINE itself, and let resolve-install.test.ts carry the behavioural coverage. + const line = INSTALLATION_SRC.split("\n").find((l) => l.startsWith("const STANDALONE_SEGMENT_RE")) + expect(line).toBeDefined() + expect(line).toContain("altimate") + expect(line).toContain("opencode") + expect(line).toContain(".local") // altimate_change end }) }) diff --git a/packages/opencode/test/installation/ownership.test.ts b/packages/opencode/test/installation/ownership.test.ts new file mode 100644 index 000000000..95d2cc5b1 --- /dev/null +++ b/packages/opencode/test/installation/ownership.test.ts @@ -0,0 +1,76 @@ +/** + * Ownership + containment (#1305 review round 2). + * + * `resolveInstall()` answers from the path alone, which cannot prove that the running + * binary belongs to a manager's GLOBAL tree. These cover the two pieces that decide it. + */ +import { describe, test, expect } from "bun:test" +import fs from "fs" +import os from "os" +import path from "path" +import { isInside, bunGlobalRoot } from "../../src/installation" + +describe("bunGlobalRoot", () => { + test("derives the package tree from the shim directory", () => { + // `bun pm bin -g` reports the SHIM dir; packages live in a sibling tree. Conflating the + // two rejected every global bun install as "not-global". + expect(bunGlobalRoot("/home/u/.bun/bin")).toBe("/home/u/.bun/install/global/node_modules") + }) + + test("a bun global binary is inside the derived root", () => { + const root = bunGlobalRoot("/home/u/.bun/bin") + const exec = "/home/u/.bun/install/global/node_modules/@altimateai/altimate-code/bin/altimate-code" + // The regression: the shim dir does NOT contain the executable, the package root does. + expect(exec.startsWith("/home/u/.bun/bin")).toBe(false) + expect(exec.startsWith(root)).toBe(true) + }) + + test("returns empty when bun reports nothing", () => { + expect(bunGlobalRoot("")).toBe("") + }) +}) + +describe("isInside", () => { + const tmp = fs.mkdtempSync(path.join(os.tmpdir(), "ownership-")) + const parent = path.join(tmp, "node_modules") + const sibling = path.join(tmp, "node_modules-other") + fs.mkdirSync(parent, { recursive: true }) + fs.mkdirSync(sibling, { recursive: true }) + + test("a child directory is inside", () => { + expect(isInside(path.join(parent, "@altimateai", "altimate-code"), parent)).toBe(true) + }) + + test("the directory itself counts as inside", () => { + expect(isInside(parent, parent)).toBe(true) + }) + + test("a sibling sharing a name prefix is NOT inside", () => { + // The previous lowercased startsWith() matched `/x/node_modules-other` against + // `/x/node_modules`, which let an unrelated tree pass the ownership check. + expect(isInside(path.join(sibling, "pkg"), parent)).toBe(false) + }) + + test("an unrelated path is not inside", () => { + expect(isInside("/somewhere/else/bin/altimate", parent)).toBe(false) + }) + + test("symlinked parents resolve before comparison", () => { + // A symlinked prefix (/var vs /private/var on macOS, nvm, asdf) previously produced a + // false "not-global" refusal because only the executable side was realpath-resolved. + const link = path.join(tmp, "link-to-node_modules") + try { + fs.symlinkSync(parent, link) + } catch { + return // symlinks unavailable (e.g. unprivileged Windows) — nothing to assert + } + expect(isInside(path.join(link, "pkg"), parent)).toBe(true) + expect(isInside(path.join(parent, "pkg"), link)).toBe(true) + }) + + test("an empty parent is never a container", () => { + // globalLayout() returns "" when the manager cannot answer; that must not read as + // containment (which would silently approve any path). + expect(isInside("/anything", "")).toBe(false) + }) +}) diff --git a/packages/opencode/test/installation/resolve-install.test.ts b/packages/opencode/test/installation/resolve-install.test.ts index d0557d59a..e5acac289 100644 --- a/packages/opencode/test/installation/resolve-install.test.ts +++ b/packages/opencode/test/installation/resolve-install.test.ts @@ -52,8 +52,11 @@ describe("resolveInstall", () => { ["standalone install", "/home/u/.altimate/bin/altimate", "curl"], ["standalone, pre-v0.7.1 dir", "/home/u/.opencode/bin/altimate", "curl"], ["standalone, distro-resolved ~/.local/bin", "/home/u/.local/bin/altimate", "curl"], - ["scoop", "C:\\Users\\u\\scoop\\apps\\altimate-code\\current\\altimate.exe", "scoop"], - ["choco", "C:\\ProgramData\\chocolatey\\lib\\altimate-code\\tools\\altimate.exe", "choco"], + // scoop/choco deliberately resolve to "unknown": upgrade()/uninstall still reference the + // upstream `opencode` package, so an actionable answer here would install or remove a + // DIFFERENT package. Notify-only until those commands carry Altimate identities. + ["scoop", "C:\\Users\\u\\scoop\\apps\\altimate-code\\current\\altimate.exe", "unknown"], + ["choco", "C:\\ProgramData\\chocolatey\\lib\\altimate-code\\tools\\altimate.exe", "unknown"], // A dev build or an unrecognised location must not be attributed to a package // manager — "unknown" degrades to notify-only rather than running someone else's // installer over it. @@ -74,7 +77,7 @@ describe("resolveInstall", () => { }) test("standalone resolution reports the directory the upgrade would write", () => { - expect(resolveInstall("/home/u/.altimate/bin/altimate", {}).root).toBe("/home/u/.altimate/bin") + expect(resolveInstall("/home/u/.altimate/bin/altimate", {}).binDir).toBe("/home/u/.altimate/bin") }) // Review findings on #1306 — layouts that contain a package segment but are NOT a @@ -114,6 +117,6 @@ describe("resolveInstall", () => { // test/sanity/Dockerfile installs to. Safe because the node_modules match runs first — // see the npm-under-~/.local case above, which resolves to npm rather than here. expect(resolveInstall("/home/u/.local/bin/altimate", {}).method).toBe("curl") - expect(resolveInstall("/home/u/.local/bin/altimate", {}).root).toBe("/home/u/.local/bin") + expect(resolveInstall("/home/u/.local/bin/altimate", {}).binDir).toBe("/home/u/.local/bin") }) }) From f228cb2f70f865b897c28e266e46a049ce489af8 Mon Sep 17 00:00:00 2001 From: Sarav Date: Tue, 15 Sep 2026 10:13:15 +0530 Subject: [PATCH 5/9] fix: mark the uninstall choco-branch removal (#1305) Marker Guard flagged the `Process.run(cmd)` change as unmarked custom code in an upstream-shared file. The choco special-case it replaced is unreachable now that `Installation.method()` no longer returns choco. Co-Authored-By: Claude Opus 5 (1M context) --- packages/opencode/src/cli/cmd/uninstall.ts | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/packages/opencode/src/cli/cmd/uninstall.ts b/packages/opencode/src/cli/cmd/uninstall.ts index 2d7378039..10ba38529 100644 --- a/packages/opencode/src/cli/cmd/uninstall.ts +++ b/packages/opencode/src/cli/cmd/uninstall.ts @@ -197,9 +197,13 @@ async function executeUninstall(method: Installation.Method, targets: RemovalTar const cmd = cmds[method] if (cmd) { spinner.start(`Running ${cmd.join(" ")}...`) + // altimate_change start — #1305: the choco special-case here passed a hardcoded + // `["choco","uninstall","opencode",...]`; choco is no longer a reachable method (see + // the command map above), so the branch is gone and `cmd` is used directly. const result = await Process.run(cmd, { nothrow: true, }) + // altimate_change end if (result.code !== 0) { spinner.stop(`Package manager uninstall failed: exit code ${result.code}`, 1) const text = `${result.stdout.toString("utf8")}\n${result.stderr.toString("utf8")}` From 6ee55b6db0dfde92dba2e1562d7d5bceb92bde22 Mon Sep 17 00:00:00 2001 From: Sarav Date: Tue, 15 Sep 2026 11:08:39 +0530 Subject: [PATCH 6/9] fix: detect the unscoped npm wrapper; redact the success path too (#1305) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses the three review comments from @sahrizvi, which I had missed — I had only been reading the automated reviews. **CRITICAL — the primary documented install path was reported as `unknown`.** `publish.ts` ships an unscoped `altimate-code` package alongside the scoped one, and that unscoped package is what `README.md:30` and `docs/docs/getting-started.md:27` tell users to install. `PKG_SEGMENT_RE` required an `@altimateai` segment, so it did not match. It is worse than a missing alternative spelling, because of which file actually executes: `postinstall.mjs` hard-links the resolved platform binary to `/bin/.altimate-code`, and both shims run that cached file BEFORE walking to the nested platform package. A hardlink has no symlink for realpath to follow, so from the first upgrade onward execPath is the wrapper's own path with the platform suffix gone entirely — `.../node_modules/ altimate-code/bin/.altimate-code`. Detection returned `unknown`, update checks stopped offering upgrades, and `altimate upgrade` failed with "Unknown installation method" — the exact opaque failure this branch exists to remove. The scope prefix is now optional and fixtures cover the shapes that actually run: unscoped and scoped cached hardlinks, the unscoped nested platform package, and an unscoped wrapper under a pnpm global root. **MAJOR — the success path logged raw subprocess output.** The previous commit redacted the failure branch but left `Effect.logInfo("upgraded", { stdout, stderr })` untouched. Both fan out to stderr under OPENCODE_PRINT_LOGS=1 and to an OTLP collector when one is configured. "The existing pattern already does this" is not a reason for either path to keep doing it, so the success path is redacted too. **MAJOR — upgrades could report success without changing the running binary.** The trailing `text([process.execPath, "--version"])` discarded its output, so an upgrade that wrote to a different prefix than the running executable passed silently. The result is now compared against the target and a mismatch is logged with the execPath and a hint. It does not fail the operation: the package manager genuinely succeeded, and branch/dev builds legitimately report a different version string. Not done from that comment: passing an explicit `--prefix` to the install command. The ownership check added earlier already prevents mutating a foreign tree — it refuses rather than writing to the wrong prefix — and pinning `--prefix` changes install semantics enough to want its own change. Co-Authored-By: Claude Opus 5 (1M context) --- packages/opencode/src/installation/index.ts | 52 ++++++++++++++----- .../test/installation/resolve-install.test.ts | 35 +++++++++++++ 2 files changed, 75 insertions(+), 12 deletions(-) diff --git a/packages/opencode/src/installation/index.ts b/packages/opencode/src/installation/index.ts index edee16a2d..28f91965a 100644 --- a/packages/opencode/src/installation/index.ts +++ b/packages/opencode/src/installation/index.ts @@ -65,16 +65,23 @@ const UPGRADE_FETCH_TIMEOUT_MS = 15_000 // The directory checks that ran before the probe loop were sound and are preserved // below; only the probe loop is replaced. // -// The running binary's own path is the ground truth. The npm `bin/altimate` shim is -// a Node script that spawnSync()s the PLATFORM package's binary, so inside the CLI -// process.execPath is: -// /lib/node_modules/@altimateai/altimate-code/node_modules/ -// @altimateai/altimate-code-darwin-arm64/bin/altimate-code -// i.e. it always lands under node_modules for every package-manager install. Match -// the optional `--` suffix explicitly rather than relying on the -// wrapper name happening to be a prefix of the platform package name. +// The running binary's own path is the ground truth, but it takes THREE shapes and the +// scope prefix is optional — `publish.ts` ships an unscoped `altimate-code` wrapper +// alongside the scoped one, and that unscoped package is what README.md:30 and +// docs/docs/getting-started.md:27 tell users to install: +// +// /lib/node_modules/altimate-code/bin/.altimate-code (unscoped wrapper, +// /lib/node_modules/@altimateai/altimate-code/bin/.altimate-code cached hardlink) +// /lib/node_modules/.../@altimateai/altimate-code-darwin-arm64/bin/altimate-code +// +// The first two are what actually run: postinstall.mjs hard-links the resolved platform +// binary to `/bin/.altimate-code` and both shims execute that cached file BEFORE +// walking to the nested platform package. A hardlink has no symlink for realpath to follow, +// so execPath keeps the wrapper's path and loses the platform suffix entirely. Requiring the +// `@altimateai` segment therefore reported "unknown" for the primary documented install +// path, silently disabling auto-upgrade for most real users. const PKG_SEGMENT_RE = - /[\\/]node_modules[\\/]@altimateai[\\/]altimate-code(?:-[a-z0-9]+-[a-z0-9]+(?:-[a-z0-9]+)?)?(?:[\\/]|$)/i + /[\\/]node_modules[\\/](?:@altimateai[\\/])?altimate-code(?:-[a-z0-9]+-[a-z0-9]+(?:-[a-z0-9]+)?)?(?:[\\/]|$)/i // pnpm global installs may expose the package via the `.pnpm` virtual store OR via a // plain `pnpm/global/` link path (no `.pnpm` segment), so match both spellings — // otherwise the plain layout falls through to the npm default and routes upgrades at @@ -844,12 +851,17 @@ export const layer: Layer.Layer getTelemetry()) T2.track({ @@ -862,7 +874,23 @@ export const layer: Layer.Layer v.trim().replace(/^v/, "") + if (after && normalize(after) !== normalize(target)) { + yield* Effect.logWarning("upgrade did not change the running binary", { + method: m, + target, + running: after, + execPath: process.execPath, + hint: "the package manager wrote somewhere other than the running executable's location", + }) + } + // altimate_change end }), } diff --git a/packages/opencode/test/installation/resolve-install.test.ts b/packages/opencode/test/installation/resolve-install.test.ts index e5acac289..77cd56874 100644 --- a/packages/opencode/test/installation/resolve-install.test.ts +++ b/packages/opencode/test/installation/resolve-install.test.ts @@ -80,6 +80,41 @@ describe("resolveInstall", () => { expect(resolveInstall("/home/u/.altimate/bin/altimate", {}).binDir).toBe("/home/u/.altimate/bin") }) + // sahrizvi review — the shapes that actually run in production. postinstall.mjs hard-links + // the platform binary into `/bin/.altimate-code` and both shims execute that cached + // file first, so after the first run execPath is the WRAPPER's path with no platform suffix. + // `publish.ts` also ships an unscoped `altimate-code` package, which is what README.md:30 + // and the getting-started docs tell users to install — so the scope prefix is optional. + test("unscoped wrapper, cached hardlink (the documented npm install) -> npm", () => { + expect( + resolveInstall("/usr/local/lib/node_modules/altimate-code/bin/.altimate-code", {}).method, + ).toBe("npm") + }) + + test("scoped wrapper, cached hardlink -> npm", () => { + expect( + resolveInstall("/usr/local/lib/node_modules/@altimateai/altimate-code/bin/.altimate-code", {}).method, + ).toBe("npm") + }) + + test("unscoped wrapper, nested platform package -> npm", () => { + expect( + resolveInstall( + "/usr/local/lib/node_modules/altimate-code/node_modules/@altimateai/altimate-code-darwin-arm64/bin/altimate-code", + {}, + ).method, + ).toBe("npm") + }) + + test("unscoped wrapper under a pnpm global root -> pnpm", () => { + expect( + resolveInstall( + "/home/u/.local/share/pnpm/global/5/node_modules/altimate-code/bin/.altimate-code", + {}, + ).method, + ).toBe("pnpm") + }) + // Review findings on #1306 — layouts that contain a package segment but are NOT a // global install. Attributing them to a manager would make upgrade() run `install -g` // and CREATE a global install the user never had (automatically, for patch releases). From 2708fc8f73fa5fda219d333b07f0b4a54d3ab4c4 Mon Sep 17 00:00:00 2001 From: Sarav Date: Tue, 15 Sep 2026 12:40:00 +0530 Subject: [PATCH 7/9] fix: resolve install ownership from the manager; refuse destructive actions when unresolved (#1305) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three-model review (round 3) returned blocker-grade findings that all traced to the same root cause: identity was being inferred from the path string. A path cannot distinguish a top-level global install from a transitive dependency, and it cannot say which of our two published wrappers owns a platform package. Ownership is now a filesystem fact obtained from the package manager. `ownerOf(root, execPath)` asks which of `@altimateai/altimate-code` / `altimate-code` actually contains the running binary, and covers both real shapes: (a) the binary inside the wrapper — postinstall hard-links it to `/bin/.altimate-code` and the shims run that first; (b) the binary as one of our platform packages stored BESIDE the wrapper — pnpm's isolated store, hoisting, every Windows install (postinstall exits early there), and any install run with `--ignore-scripts`. (b) is bounded to that manager's own tree so a project-local binary cannot borrow a global wrapper's identity, and it refuses when BOTH wrappers are installed rather than guessing. `Installation.method()` returns a package-manager identity only when ownership is confirmed; otherwise `unknown`. That made `unknown` much more common, which exposed the consumer that had never been re-examined: **`uninstall` now refuses before removing anything when ownership is unresolved.** It was deleting data, config, cache and state unconditionally while skipping both the binary and the package removal — so an unverifiable install lost everything the user cared about and stayed installed, silently. It now stops and prints per-manager removal instructions. **The CLI upgrade dead end is gone.** "Install anyways?" passed `unknown` straight to `Installation.upgrade()`, which refuses it, so both answers ended in `UpgradeFailedError`. Replaced with actionable instructions. `UNSUPPORTED_UPGRADE_METHODS` is shared so the CLI and both HTTP routes reject the same set — the v2 handler had drifted to `unknown` only. `upgrade()` refuses yarn/scoop/choco at the choke point and the scoop/choco branches are deleted; they installed upstream's `opencode` package, not ours. Diagnostics are redacted before reaching any sink — the logger fans out to stderr under OPENCODE_PRINT_LOGS and to an OTLP collector when one is configured, so "it stays local" was never true. Masks now cover `Basic` blobs, quoted JSON keys and bare URL userinfo, and the post-upgrade `running:` field is redacted like every other subprocess-derived value. KNOWN OUTSTANDING — deliberately not fixed here, tracked for a follow-up: * `owningPackageOrScoped()` still falls back to the scoped name when a second manager query disagrees with the one `method()` already made. An unscoped install could then be upgraded under the scoped name, installing a duplicate. The fix is to resolve one identity and thread it through rather than re-deriving it per call site. * `resolveInstall()` picks a single candidate manager from path shape and only that one is queried, so a custom bun/pnpm directory without the expected segment resolves to npm, finds no owner, and degrades to `unknown`. Path shape should order which managers to ask, not decide the answer. * The standalone upgrade path still writes `$HOME/.altimate/bin` regardless of where the running binary lives. * Coverage is unit-level; there is no test across detection → upgrade → uninstall. Co-Authored-By: Claude Opus 5 (1M context) --- packages/opencode/src/cli/cmd/uninstall.ts | 49 +++- packages/opencode/src/cli/cmd/upgrade.ts | 39 ++- packages/opencode/src/installation/index.ts | 245 +++++++++++++----- packages/opencode/src/server/routes/global.ts | 7 +- .../instance/httpapi/handlers/global.ts | 8 +- .../test/install/upgrade-method.test.ts | 13 +- .../test/installation/ownership.test.ts | 164 +++++++++++- .../test/installation/resolve-install.test.ts | 5 - 8 files changed, 412 insertions(+), 118 deletions(-) diff --git a/packages/opencode/src/cli/cmd/uninstall.ts b/packages/opencode/src/cli/cmd/uninstall.ts index 10ba38529..bc1c71217 100644 --- a/packages/opencode/src/cli/cmd/uninstall.ts +++ b/packages/opencode/src/cli/cmd/uninstall.ts @@ -62,9 +62,34 @@ export const UninstallCommand = { const method = await Installation.method() prompts.log.info(`Installation method: ${method}`) + // altimate_change start — #1305: refuse BEFORE removing anything when we cannot tell what + // installed this binary. + // + // `unknown` means detection could not confirm an owner. The removal targets below always + // include data, config, cache and state, while the binary and the package-manager entry + // are only removed for a known method — so proceeding here wiped everything the user + // cares about and left the installation running, with no indication that had happened. + // Data loss with nothing uninstalled is strictly worse than declining. + if (method === "unknown") { + prompts.log.error(`Cannot determine how altimate was installed (running from ${process.execPath}).`) + prompts.log.info("Uninstalling now would delete your data and config while leaving the program installed.") + prompts.log.info("Remove it with the tool you installed it with, then re-run to clean up data:") + prompts.log.info(" npm/pnpm/bun/yarn: uninstall -g @altimateai/altimate-code (or altimate-code)") + prompts.log.info(" Homebrew: brew uninstall altimate-code") + prompts.log.info(" install script: rm the binary from ~/.altimate/bin") + prompts.outro("Nothing was removed") + return + } + // altimate_change end + const targets = await collectRemovalTargets(args, method) - await showRemovalSummary(targets, method) + // altimate_change start — #1305: the package the MANAGER confirms owns this binary. + // publish.ts ships both a scoped and an unscoped wrapper; removing the wrong one removes + // nothing while uninstall goes on to delete config and cache. + const pkg = (await Installation.packageName()) ?? "@altimateai/altimate-code" + // altimate_change end + await showRemovalSummary(targets, method, pkg) if (!args.force && !args.dryRun) { const confirm = await prompts.confirm({ @@ -83,7 +108,7 @@ export const UninstallCommand = { return } - await executeUninstall(method, targets) + await executeUninstall(method, targets, pkg) prompts.outro("Done") }, @@ -103,7 +128,7 @@ async function collectRemovalTargets(args: UninstallArgs, method: Installation.M return { directories, shellConfig, binary } } -async function showRemovalSummary(targets: RemovalTargets, method: Installation.Method) { +async function showRemovalSummary(targets: RemovalTargets, method: Installation.Method, pkg: string) { prompts.log.message("The following will be removed:") for (const dir of targets.directories) { @@ -135,10 +160,10 @@ async function showRemovalSummary(targets: RemovalTargets, method: Installation. // installed. scoop/choco are omitted: Installation.method() no longer returns them // (their commands still reference upstream identities), so they are unreachable here. const cmds: Record = { - npm: "npm uninstall -g @altimateai/altimate-code", - pnpm: "pnpm uninstall -g @altimateai/altimate-code", - bun: "bun remove -g @altimateai/altimate-code", - yarn: "yarn global remove @altimateai/altimate-code", + npm: `npm uninstall -g ${pkg}`, + pnpm: `pnpm uninstall -g ${pkg}`, + bun: `bun remove -g ${pkg}`, + yarn: `yarn global remove ${pkg}`, brew: "brew uninstall altimate-code", } // altimate_change end @@ -146,7 +171,7 @@ async function showRemovalSummary(targets: RemovalTargets, method: Installation. } } -async function executeUninstall(method: Installation.Method, targets: RemovalTargets) { +async function executeUninstall(method: Installation.Method, targets: RemovalTargets, pkg: string) { const spinner = prompts.spinner() const errors: string[] = [] @@ -186,10 +211,10 @@ async function executeUninstall(method: Installation.Method, targets: RemovalTar if (method !== "curl" && method !== "unknown") { // altimate_change start — #1305: Altimate package identities, not upstream's. const cmds: Record = { - npm: ["npm", "uninstall", "-g", "@altimateai/altimate-code"], - pnpm: ["pnpm", "uninstall", "-g", "@altimateai/altimate-code"], - bun: ["bun", "remove", "-g", "@altimateai/altimate-code"], - yarn: ["yarn", "global", "remove", "@altimateai/altimate-code"], + npm: ["npm", "uninstall", "-g", pkg], + pnpm: ["pnpm", "uninstall", "-g", pkg], + bun: ["bun", "remove", "-g", pkg], + yarn: ["yarn", "global", "remove", pkg], brew: ["brew", "uninstall", "altimate-code"], } // altimate_change end diff --git a/packages/opencode/src/cli/cmd/upgrade.ts b/packages/opencode/src/cli/cmd/upgrade.ts index e1a12da26..3de80d361 100644 --- a/packages/opencode/src/cli/cmd/upgrade.ts +++ b/packages/opencode/src/cli/cmd/upgrade.ts @@ -46,27 +46,26 @@ export const UpgradeCommand = { // altimate_change end const detectedMethod = await Installation.method() const method = (args.method as Installation.Method) ?? detectedMethod - // altimate_change start — #1305: Installation.upgrade()'s switch has no `yarn` case, so - // yarn reaches `default` and dies with "Unknown installation method: yarn". cli/upgrade.ts - // already routes yarn to notify for the same reason; this is the explicit-command path. - if (method === "unknown" || method === "yarn") { - // altimate_change end - // altimate_change start — branding - prompts.log.error(`altimate is installed to ${process.execPath} and may be managed by a package manager`) - // altimate_change end - const install = await prompts.select({ - message: "Install anyways?", - options: [ - { label: "Yes", value: true }, - { label: "No", value: false }, - ], - initialValue: false, - }) - if (!install) { - prompts.outro("Done") - return - } + // altimate_change start — #1305: stop instead of offering a choice that cannot work. + // `Installation.upgrade()` refuses every method in UNSUPPORTED_UPGRADE_METHODS, so the + // old "Install anyways?" prompt ended in `UpgradeFailedError: Unknown installation + // method` whichever way the user answered — and detection now returns `unknown` for + // anything it cannot verify, which made that dead end much more common. + if (Installation.UNSUPPORTED_UPGRADE_METHODS.includes(method)) { + prompts.log.error( + method === "unknown" + ? `Cannot determine how altimate was installed (running from ${process.execPath}).` + : `Upgrading a ${method} installation is not supported.`, + ) + prompts.log.info("Upgrade with the tool you installed it with:") + prompts.log.info(" npm/pnpm/bun: install -g @altimateai/altimate-code@latest (or altimate-code)") + prompts.log.info(" Homebrew: brew upgrade altimate-code") + prompts.log.info(" install script: curl -fsSL https://www.altimate.sh/install | bash") + prompts.log.info("Or force a specific manager with --method .") + prompts.outro("Done") + return } + // altimate_change end prompts.log.info("Using method: " + method) const target = args.target ? args.target.replace(/^v/, "") : await Installation.latest() diff --git a/packages/opencode/src/installation/index.ts b/packages/opencode/src/installation/index.ts index 28f91965a..df265e379 100644 --- a/packages/opencode/src/installation/index.ts +++ b/packages/opencode/src/installation/index.ts @@ -103,14 +103,15 @@ const CHOCO_SEGMENT_RE = /[\\/]chocolatey[\\/]/i // the pre-v0.7.1 directory name and `.local/bin` a distro-resolved location; both are // kept for back-compat (#820) and `.local/bin` is also what test/sanity/Dockerfile uses. // -// These are checked AFTER the node_modules match above, which is what makes them safe: -// a package-manager install's execPath is the spawned platform binary deep under -// `/lib/node_modules/...`, so it can never collide with `/bin` here even -// when the prefix is `~/.local`. +// These are checked AFTER the node_modules match above, which is what makes them safe: a +// package-manager install's execPath always sits under `/lib/node_modules/...` +// (whichever of the three shapes above it takes), so it can never collide with +// `/bin` here even when the prefix is `~/.local`. const STANDALONE_SEGMENT_RE = /[\\/]\.(?:altimate|opencode)[\\/]bin[\\/]|[\\/]\.local[\\/]bin[\\/]/i // An npx cache, a package-manager download cache, or a project-local node_modules all -// contain a `node_modules/@altimateai/altimate-code*` segment but are NOT global installs. +// contain a matching `node_modules/[@altimateai/]altimate-code*` segment but are NOT global +// installs. // Attributing them to a package manager would make `upgrade()` run `npm install -g` and // CREATE a global install the user never had — and for patch releases that happens // automatically at startup (see cli/upgrade.ts). The old probe loop returned "unknown" @@ -120,21 +121,59 @@ const EPHEMERAL_SEGMENT_RE = /[\\/](?:_npx|_cacache|dlx|\.npx)[\\/]|[\\/]install export interface ResolvedInstall { readonly method: Method - /** Directory the running binary sits in, for standalone layouts only. - * - * NOT the upgrade target: the install script always writes $HOME/.altimate/bin, so for a - * legacy ~/.opencode/bin or ~/.local/bin install these differ — which is why globalLayout() - * names the real target rather than reading this. */ - readonly binDir?: string +} + +/** The two npm packages we publish. `publish.ts` ships a scoped wrapper and an unscoped one, + * and the docs tell users to install the unscoped one. Platform binaries are ALWAYS scoped + * (`@altimateai/altimate-code--`), so the running binary's own path cannot tell you + * which wrapper owns it — hence ownerOf() below asks the filesystem instead of guessing. */ +const CANDIDATE_PACKAGES = ["@altimateai/altimate-code", "altimate-code"] as const + +/** Our per-platform packages. Always scoped, regardless of which wrapper pulled them in. */ +const PLATFORM_PKG_RE = + /[\\/]node_modules[\\/]@altimateai[\\/]altimate-code-[a-z0-9]+-[a-z0-9]+(?:-baseline)?(?:[\\/]|$)/i + +/** Which of our packages, installed at top level in `root`, owns `execPath`? + * + * Deliberately does NOT parse `execPath` for the package name. A path cannot distinguish a + * top-level global install from a transitive dependency, and it cannot say which wrapper + * owns a platform package — platform packages are always scoped whichever wrapper pulled + * them in, so reading the scope off the running binary named the wrong package. + * + * Two ways a binary belongs to a wrapper: + * + * (a) It lives inside the wrapper directory. This is the common case: postinstall.mjs + * hard-links the platform binary to `/bin/.altimate-code` and the shims run + * that first. + * (b) It IS one of our platform packages, stored beside the wrapper rather than under it. + * pnpm's isolated store puts `.pnpm/@altimateai+altimate-code--@V/...` as a + * SIBLING of the wrapper's own store entry, and hoisting does the same. This is not an + * edge case: postinstall skips the cached binary on Windows entirely, and any install + * run with `--ignore-scripts` takes this route on every platform. + * + * (b) is bounded to the manager's own tree so a project-local binary cannot borrow a global + * wrapper's identity, and it refuses to guess when BOTH wrappers are installed. */ +export function ownerOf(root: string, execPath: string): string | undefined { + if (!root || !execPath) return undefined + const present = CANDIDATE_PACKAGES.filter((name) => fs.existsSync(path.join(root, ...name.split("/")))) + for (const name of present) { + if (isInside(execPath, path.join(root, ...name.split("/")))) return name + } + if (!PLATFORM_PKG_RE.test(execPath)) return undefined + // Must be this manager's tree — `dirname(root)` covers stores kept beside `node_modules`. + if (!isInside(execPath, root) && !isInside(execPath, path.dirname(root))) return undefined + // Exactly one of our wrappers installed: it is the only thing that could have pulled this + // platform package in. Both installed means we cannot say which, and guessing would + // upgrade or remove the wrong one. + return present.length === 1 ? present[0] : undefined } /** Resolve a CANDIDATE install identity for THIS process from its path. * * Pure in (execPath, env) so it can be unit-tested against fabricated layouts without - * spawning real installs — which is also its limit: a path cannot prove global ownership. - * A project-local node_modules is shaped exactly like a global one, so a package-manager - * answer here is a hypothesis. `Installation.method()` confirms it with the manager before - * any consumer receives an actionable identity. */ + * spawning real installs — which is also its limit: a path cannot prove global ownership, + * nor say which package owns the binary. It picks which manager to ASK; `ownerOf()` answers + * whether that manager actually owns us, and under which package name. */ export function resolveInstall( execPath: string = realExecPath(), env: NodeJS.ProcessEnv = process.env, @@ -152,17 +191,18 @@ export function resolveInstall( } if (BREW_SEGMENT_RE.test(execPath)) return { method: "brew" } // scoop / choco are deliberately NOT returned here. `latest()` and `upgrade()` still query - // and install the upstream `opencode` package (see the scoop/choco cases below), so - // resolving an Altimate install to those methods would install a DIFFERENT, upstream - // package alongside it. The old probe loop self-limited because it required - // `scoop list opencode` to match; path matching has no such guard. Returning "unknown" - // degrades to notify-only until those commands use Altimate package identities. + // and install the upstream `opencode` package, so resolving an Altimate install to those + // methods would pull in a DIFFERENT, upstream package. The old probe loop self-limited + // because it required `scoop list opencode` to match; path matching has no such guard. if (SCOOP_SEGMENT_RE.test(execPath) || CHOCO_SEGMENT_RE.test(execPath)) return { method: "unknown" } - if (STANDALONE_SEGMENT_RE.test(execPath)) return { method: "curl", binDir: path.dirname(execPath) } + if (STANDALONE_SEGMENT_RE.test(execPath)) return { method: "curl" } return { method: "unknown" } } -/** realpath so a symlinked bin entry (npm, brew) resolves to the file it points at. +/** realpath so a symlinked bin entry (brew's Cellar link, a shimmed standalone install) + * resolves to the file it points at. Note npm's cached binary is a HARDLINK, which has + * nothing to resolve — the path stays as-is, which is why detection matches the wrapper + * package rather than relying on reaching the platform package. * Falls back to the raw path when the file is gone or unreadable. */ function realExecPath(): string { try { @@ -243,23 +283,42 @@ function isWritable(dir: string): boolean { * error output routinely carries registry `_authToken` values and credentialed URLs. * * Conservative by design: over-masking a diagnostic is cheap, leaking a token is not. */ -function redactSecrets(input: string): string { +export function redactSecrets(input: string): string { if (!input) return input - return input - .replace(/(bearer\s+)\S+/gi, "$1[REDACTED]") - .replace( - /((?:auth[-_]?token|authorization|api[-_]?key|access[-_]?token|password|passwd|secret|token)\s*[:=]\s*)(["']?)[^\s"',}]+/gi, - "$1$2[REDACTED]", - ) - .replace(/(https?:\/\/)[^\s/@]+:[^\s/@]+@/gi, "$1[REDACTED]@") - .replace(/\b[0-9a-f]{32,}\b/gi, "[REDACTED]") + return ( + input + // `//registry.npmjs.org/:_authToken=…` — the exact shape npm prints in ERESOLVE/E401 + // output and writes to .npmrc, which a plain `token=` pattern misses because of the + // leading underscore and the registry prefix. + .replace(/(_auth(?:Token)?|_password)\s*=\s*\S+/gi, "$1=[REDACTED]") + // Whole authorization values, not just the Bearer scheme: `Basic dXNlcjpwYXNz` leaked + // the encoded credential when only the scheme word was masked. + .replace(/((?:authorization|proxy-authorization)\s*[:=]\s*)(["']?)\S+.*$/gim, "$1$2[REDACTED]") + .replace(/\b((?:bearer|basic|token)\s+)\S+/gi, "$1[REDACTED]") + // Handles both `token=value` and JSON's `"token":"value"` — the quoted key form slipped + // past a pattern that expected the key to be bare. + .replace( + /(["']?)(auth[-_]?token|authorization|api[-_]?key|access[-_]?token|password|passwd|secret|token)\1(\s*[:=]\s*)(["']?)[^\s"',}]+/gi, + "$1$2$1$3$4[REDACTED]", + ) + // Credentials embedded in a registry or git remote URL. Userinfo WITHOUT a colon is a + // token too (`https://@registry/...`), and the previous pattern required the + // `user:pass` form so it let those through. + .replace(/(https?:\/\/)[^\s/@]+@/gi, "$1[REDACTED]@") + // provider-prefixed tokens travel in git/registry errors and are not key=value shaped + .replace(/\b(gh[pousr]_|github_pat_|glpat-|npm_|sk-|xox[baprs]-)[A-Za-z0-9_-]{8,}/g, "$1[REDACTED]") + // long opaque blobs: hex digests and base64-ish secrets + .replace(/\b[0-9a-f]{32,}\b/gi, "[REDACTED]") + .replace(/\b[A-Za-z0-9+/]{40,}={0,2}\b/g, "[REDACTED]") + ) } /** Classify a failed upgrade into a stable code plus a message safe to show. * - * Deliberately does NOT echo the package manager's stderr — it can carry tokens and - * environment. The classification is derived from it, the raw text is only logged - * locally (see the logWarning in upgrade()). */ + * Deliberately does NOT echo the package manager's stderr into the user-facing message — it + * can carry tokens and environment. The classification is derived from it; the raw text is + * logged only after redactSecrets() (see the logWarning/logInfo in upgrade()), because the + * logger fans out to stderr and to OTLP and is NOT local-only. */ function classifyFailure(stderr: string, stdout: string): { code: string; hint?: string } { const t = `${stderr}\n${stdout}` if (/EACCES|EPERM|permission denied/i.test(t)) @@ -276,6 +335,12 @@ function classifyFailure(stderr: string, stdout: string): { code: string; hint?: export type Method = "curl" | "npm" | "yarn" | "pnpm" | "bun" | "brew" | "scoop" | "choco" | "unknown" +// altimate_change start — #1305: methods upgrade() refuses. Exported so every consumer +// rejects the same set instead of each maintaining its own list (they drifted: the v2 HTTP +// handler checked only "unknown" and 500'd on the rest). +export const UNSUPPORTED_UPGRADE_METHODS: Method[] = ["unknown", "yarn", "scoop", "choco"] +// altimate_change end + export type ReleaseType = "patch" | "minor" | "major" export const Event = { @@ -351,6 +416,8 @@ export interface Interface { readonly method: () => Effect.Effect readonly latest: (method?: Method) => Effect.Effect readonly upgrade: (method: Method, target: string) => Effect.Effect + // altimate_change — #1305: verified owning package, or undefined when not ours + readonly packageName: () => Effect.Effect } export class Service extends Context.Service()("@opencode/Installation") {} @@ -473,23 +540,35 @@ export const layer: Layer.Layer { - const pkg = `@altimateai/altimate-code@${target}` + + /** The package to install when upgrading. Uses the verified owner so an unscoped install + * is upgraded with the unscoped name — installing the other one would leave a duplicate + * and a stale original. Falls back to the scoped name only when a caller forced a method + * explicitly and no owner could be confirmed. */ + const owningPackageOrScoped = Effect.fnUntraced(function* (m: Method) { + return (yield* owningPackage(m)) ?? "@altimateai/altimate-code" + }) + + const remediation = (m: Method, dir: string, target: string, owner: string) => { + // The name the manager confirmed owns this install — telling a user to reinstall the + // other one would leave them with a duplicate and a stale original. + const pkg = `${owner}@${target}` switch (m) { case "npm": // Windows has no sudo — tell those users to use an elevated shell instead. @@ -532,25 +611,21 @@ export const layer: Layer.Layer) => runPromise((s) => s.latest(...args)) export const method = () => runPromise((s) => s.method()) +// altimate_change start — #1305: the package the manager confirms owns the running binary. +// `uninstall` needs it for the same reason `upgrade` does: removing the wrong one of our two +// published wrappers silently removes nothing while the real install stays. +export const packageName = () => runPromise((s) => s.packageName()) +// altimate_change end export const upgrade = (...args: Parameters) => runPromise((s) => s.upgrade(...args)) // altimate_change start — thunk LayerNode deps defers facade refs past circular module-init diff --git a/packages/opencode/src/server/routes/global.ts b/packages/opencode/src/server/routes/global.ts index 80f5bebc7..a8ccbac31 100644 --- a/packages/opencode/src/server/routes/global.ts +++ b/packages/opencode/src/server/routes/global.ts @@ -309,10 +309,9 @@ export const GlobalRoutes = lazy(() => ), async (c) => { const method = await Installation.method() - // altimate_change start — #1305: `yarn` has no case in Installation.upgrade()'s switch, - // so it would reach `default` and surface as an opaque failure. Reject it up front the - // same way `unknown` is rejected. - if (method === "unknown" || method === "yarn") { + // altimate_change start — #1305: Installation.upgrade() refuses these, which would + // surface as an opaque 500. Reject up front with a 400, like `unknown`. + if (Installation.UNSUPPORTED_UPGRADE_METHODS.includes(method)) { return c.json({ success: false, error: `Unsupported installation method: ${method}` }, 400) } // altimate_change end diff --git a/packages/opencode/src/server/routes/instance/httpapi/handlers/global.ts b/packages/opencode/src/server/routes/instance/httpapi/handlers/global.ts index 97def2499..f3c66d001 100644 --- a/packages/opencode/src/server/routes/instance/httpapi/handlers/global.ts +++ b/packages/opencode/src/server/routes/instance/httpapi/handlers/global.ts @@ -96,12 +96,16 @@ export const globalHandlers = HttpApiBuilder.group(RootHttpApi, "global", (handl const upgrade = Effect.fn("GlobalHttpApi.upgrade")(function* (ctx: { payload: typeof GlobalUpgradeInput.Type }) { const method = yield* installation.method() - if (method === "unknown") { + // altimate_change start — #1305: this handler only rejected "unknown", so a yarn/scoop/ + // choco install reached Installation.upgrade(), which refuses them — surfacing as an + // unhandled error rather than a 400. Kept in step with the Hono route. + if (Installation.UNSUPPORTED_UPGRADE_METHODS.includes(method)) { return { status: 400, - body: { success: false as const, error: "Unknown installation method" }, + body: { success: false as const, error: `Unsupported installation method: ${method}` }, } } + // altimate_change end // NOTE: the branch/dev-build channel guard that cli/cmd/upgrade.ts and the Hono /global upgrade // route carry is intentionally NOT applied here. This v2 HttpApi tree is not mounted by the // shipped server (cli/cmd/serve.ts loads the Hono server/server.ts), and the guard's isLocal() diff --git a/packages/opencode/test/install/upgrade-method.test.ts b/packages/opencode/test/install/upgrade-method.test.ts index f351f400b..cd5f4e7e5 100644 --- a/packages/opencode/test/install/upgrade-method.test.ts +++ b/packages/opencode/test/install/upgrade-method.test.ts @@ -91,8 +91,17 @@ describe("brew latest() version resolution", () => { }) describe("upgrade execution", () => { - test("npm upgrade uses scoped package name", () => { - expect(INSTALLATION_SRC).toContain("@altimateai/altimate-code@${target}") + test("npm upgrade installs an Altimate package, never upstream's", () => { + // altimate_change start — #1305: the literal scoped name was replaced by upgradePackage(), + // which returns whichever Altimate package OWNS the running install (publish.ts ships a + // scoped and an unscoped one; upgrading with the wrong name installs a second copy). + // The brand contract is unchanged: both candidates are ours, never `opencode-ai`. + expect(INSTALLATION_SRC).toContain("${yield* owningPackageOrScoped(m)}@${target}") + const helper = INSTALLATION_SRC.split("\n").find((l) => l.includes("owningPackage(m)) ??")) + expect(helper).toBeDefined() + expect(helper).toContain("@altimateai/altimate-code") + expect(helper).not.toContain("opencode-ai") + // altimate_change end }) test("brew upgrade taps AltimateAI/tap", () => { diff --git a/packages/opencode/test/installation/ownership.test.ts b/packages/opencode/test/installation/ownership.test.ts index 95d2cc5b1..46380c888 100644 --- a/packages/opencode/test/installation/ownership.test.ts +++ b/packages/opencode/test/installation/ownership.test.ts @@ -8,7 +8,7 @@ import { describe, test, expect } from "bun:test" import fs from "fs" import os from "os" import path from "path" -import { isInside, bunGlobalRoot } from "../../src/installation" +import { isInside, bunGlobalRoot, ownerOf, redactSecrets } from "../../src/installation" describe("bunGlobalRoot", () => { test("derives the package tree from the shim directory", () => { @@ -74,3 +74,165 @@ describe("isInside", () => { expect(isInside("/anything", "")).toBe(false) }) }) + +describe("ownerOf", () => { + // Real directories, because the whole point is that ownership is a filesystem fact rather + // than something inferable from the path string. + const root = fs.mkdtempSync(path.join(os.tmpdir(), "owner-")) + const mk = (p: string) => { + const full = path.join(root, p) + fs.mkdirSync(path.dirname(full), { recursive: true }) + fs.writeFileSync(full, "") + return full + } + + test("finds the unscoped wrapper", () => { + const exec = mk("altimate-code/bin/.altimate-code") + expect(ownerOf(root, exec)).toBe("altimate-code") + }) + + test("finds the scoped wrapper", () => { + const exec = mk("@altimateai/altimate-code/bin/.altimate-code") + expect(ownerOf(root, exec)).toBe("@altimateai/altimate-code") + }) + + test("a platform binary nested in the unscoped wrapper reports the UNSCOPED name", () => { + // The platform package is always scoped, so reading the scope off the running binary's + // path named the wrong wrapper — upgrading then installed a duplicate and left the + // user's install stale. Containment in the top-level directory gets it right. + const exec = mk("altimate-code/node_modules/@altimateai/altimate-code-darwin-arm64/bin/altimate-code") + expect(ownerOf(root, exec)).toBe("altimate-code") + }) + + test("a transitive dependency of another global CLI is NOT ours", () => { + // Inside the manager's global tree, but not a global install of ours. Containment in the + // global root alone accepted this and would have run `install -g` for a package the user + // never installed. + const exec = mk("another-cli/node_modules/altimate-code/bin/.altimate-code") + expect(ownerOf(root, exec)).toBeUndefined() + }) + + test("a binary outside the global root is NOT ours", () => { + expect(ownerOf(root, "/somewhere/else/altimate")).toBeUndefined() + }) + + test("pnpm isolated store: platform binary is a SIBLING of the wrapper, not inside it", () => { + // The shape that actually runs on Windows (postinstall skips the cached binary) and + // anywhere `--ignore-scripts` was used. An earlier version of this test put the binary + // inside the wrapper's own store directory, which is not how pnpm lays it out — that + // masked the failure and let a broken containment check look correct. + const store = path.join(root, "pnpm-case", "node_modules") + const wrapper = path.join(store, ".pnpm", "altimate-code@1.0.0", "node_modules", "altimate-code") + const platform = path.join( + store, + ".pnpm", + "@altimateai+altimate-code-linux-x64@1.0.0", + "node_modules", + "@altimateai", + "altimate-code-linux-x64", + "bin", + ) + fs.mkdirSync(wrapper, { recursive: true }) + fs.mkdirSync(platform, { recursive: true }) + const exec = path.join(platform, "altimate-code") + fs.writeFileSync(exec, "") + try { + fs.symlinkSync(wrapper, path.join(store, "altimate-code")) + } catch { + return // symlinks unavailable + } + // Neither wrapper directory contains the binary, but exactly one of our wrappers is + // installed in this tree, so it is unambiguously the owner. + expect(isInside(exec, wrapper)).toBe(false) + expect(ownerOf(store, exec)).toBe("altimate-code") + }) + + test("a platform binary is ambiguous when BOTH wrappers are installed", () => { + // Guessing here would upgrade or uninstall the wrong wrapper. + const store = path.join(root, "ambiguous", "node_modules") + fs.mkdirSync(path.join(store, "altimate-code"), { recursive: true }) + fs.mkdirSync(path.join(store, "@altimateai", "altimate-code"), { recursive: true }) + const platform = path.join(store, ".pnpm", "p@1", "node_modules", "@altimateai", "altimate-code-linux-x64", "bin") + fs.mkdirSync(platform, { recursive: true }) + const exec = path.join(platform, "altimate-code") + fs.writeFileSync(exec, "") + expect(ownerOf(store, exec)).toBeUndefined() + }) + + test("a platform binary outside the manager's tree does not borrow its identity", () => { + // A project-local platform package must not be attributed to a global wrapper. + const store = path.join(root, "bounded", "node_modules") + fs.mkdirSync(path.join(store, "altimate-code"), { recursive: true }) + const elsewhere = path.join(root, "someproject", "node_modules", "@altimateai", "altimate-code-linux-x64", "bin") + fs.mkdirSync(elsewhere, { recursive: true }) + const exec = path.join(elsewhere, "altimate-code") + fs.writeFileSync(exec, "") + expect(ownerOf(store, exec)).toBeUndefined() + }) + + test("an unknown root yields no owner", () => { + // globalLayout() returns "" when the manager cannot answer. That must not authorise + // anything — the previous version treated it as permission to act. + expect(ownerOf("", "/anything")).toBeUndefined() + }) + + test("resolves through a symlinked top-level entry (pnpm-style virtual store)", () => { + // pnpm links top-level names into a virtual store whose location differs between + // layouts; resolving the link means we never have to enumerate where the store lives. + const store = path.join(root, ".store", "altimate-code@1", "node_modules", "altimate-code") + fs.mkdirSync(path.join(store, "bin"), { recursive: true }) + const exec = path.join(store, "bin", ".altimate-code") + fs.writeFileSync(exec, "") + const link = path.join(root, "linked-root") + fs.mkdirSync(link, { recursive: true }) + try { + fs.symlinkSync(store, path.join(link, "altimate-code")) + } catch { + return // symlinks unavailable + } + expect(ownerOf(link, exec)).toBe("altimate-code") + }) +}) + +describe("redactSecrets", () => { + // Diagnostics reach stderr (OPENCODE_PRINT_LOGS) and a remote OTLP collector, so these are + // the shapes real npm/pnpm/yarn failures actually print. + const cases: Array<[string, string]> = [ + ["npm registry auth", "//registry.npmjs.org/:_authToken=abc123def456ghi"], + ["bearer header", "Authorization: Bearer abcdef123456"], + ["credentialed url", "https://user:hunter2@registry.example.com/pkg"], + ["github token", "remote: fatal ghp_ABCDEFGHIJKLMNOPQRSTUVWXYZ012345"], + ["key=value token", 'token="s3cr3t-value-here"'], + ["hex digest", "sha512-" + "a".repeat(40)], + // Shapes round 3 found still exposed. + ["basic auth blob", "Authorization: Basic dXNlcjpwYXNzd29yZDEyMw=="], + ["quoted json key", '{"token":"short-secret"}'], + ["bare url userinfo", "https://short-secret@registry.example/pkg"], + ] + for (const [name, input] of cases) { + test(`masks ${name}`, () => { + const out = redactSecrets(input) + expect(out).toContain("[REDACTED]") + for (const secret of [ + "abc123def456ghi", + "abcdef123456", + "hunter2", + "ABCDEFGHIJKLMNOPQRSTUVWXYZ012345", + "s3cr3t-value-here", + "dXNlcjpwYXNzd29yZDEyMw==", + "short-secret", + ]) { + if (input.includes(secret)) expect(out).not.toContain(secret) + } + }) + } + + test("leaves ordinary diagnostics readable", () => { + const msg = "npm ERR! code EACCES\nnpm ERR! syscall mkdir\nnpm ERR! path /usr/local/lib" + expect(redactSecrets(msg)).toBe(msg) + }) + + test("is a no-op on empty input", () => { + expect(redactSecrets("")).toBe("") + }) +}) diff --git a/packages/opencode/test/installation/resolve-install.test.ts b/packages/opencode/test/installation/resolve-install.test.ts index 77cd56874..46a1ee888 100644 --- a/packages/opencode/test/installation/resolve-install.test.ts +++ b/packages/opencode/test/installation/resolve-install.test.ts @@ -76,10 +76,6 @@ describe("resolveInstall", () => { expect(resolveInstall(`${NPM_PREFIXED}/${PLATFORM}`, env).method).toBe("unknown") }) - test("standalone resolution reports the directory the upgrade would write", () => { - expect(resolveInstall("/home/u/.altimate/bin/altimate", {}).binDir).toBe("/home/u/.altimate/bin") - }) - // sahrizvi review — the shapes that actually run in production. postinstall.mjs hard-links // the platform binary into `/bin/.altimate-code` and both shims execute that cached // file first, so after the first run execPath is the WRAPPER's path with no platform suffix. @@ -152,6 +148,5 @@ describe("resolveInstall", () => { // test/sanity/Dockerfile installs to. Safe because the node_modules match runs first — // see the npm-under-~/.local case above, which resolves to npm rather than here. expect(resolveInstall("/home/u/.local/bin/altimate", {}).method).toBe("curl") - expect(resolveInstall("/home/u/.local/bin/altimate", {}).binDir).toBe("/home/u/.local/bin") }) }) From 40779c55368e0bb0ef9b9dc54b070ed457575c89 Mon Sep 17 00:00:00 2001 From: Sarav Date: Tue, 15 Sep 2026 12:40:27 +0530 Subject: [PATCH 8/9] fix: mark the two remaining altimate_change additions (#1305) Marker Guard flagged the uninstall summary call and the packageName interface member as unmarked custom code in upstream-shared files. Co-Authored-By: Claude Opus 5 (1M context) --- packages/opencode/src/cli/cmd/uninstall.ts | 11 ++++++++++- packages/opencode/src/installation/index.ts | 9 ++++++--- 2 files changed, 16 insertions(+), 4 deletions(-) diff --git a/packages/opencode/src/cli/cmd/uninstall.ts b/packages/opencode/src/cli/cmd/uninstall.ts index bc1c71217..27b2da565 100644 --- a/packages/opencode/src/cli/cmd/uninstall.ts +++ b/packages/opencode/src/cli/cmd/uninstall.ts @@ -88,8 +88,8 @@ export const UninstallCommand = { // publish.ts ships both a scoped and an unscoped wrapper; removing the wrong one removes // nothing while uninstall goes on to delete config and cache. const pkg = (await Installation.packageName()) ?? "@altimateai/altimate-code" - // altimate_change end await showRemovalSummary(targets, method, pkg) + // altimate_change end if (!args.force && !args.dryRun) { const confirm = await prompts.confirm({ @@ -108,7 +108,10 @@ export const UninstallCommand = { return } + // altimate_change start — #1305: pass the verified package name through so removal + // targets the wrapper the user actually installed. await executeUninstall(method, targets, pkg) + // altimate_change end prompts.outro("Done") }, @@ -128,7 +131,10 @@ async function collectRemovalTargets(args: UninstallArgs, method: Installation.M return { directories, shellConfig, binary } } +// altimate_change start — #1305: takes the verified package name so the summary prints the +// command that will actually run. async function showRemovalSummary(targets: RemovalTargets, method: Installation.Method, pkg: string) { + // altimate_change end prompts.log.message("The following will be removed:") for (const dir of targets.directories) { @@ -171,7 +177,10 @@ async function showRemovalSummary(targets: RemovalTargets, method: Installation. } } +// altimate_change start — #1305: takes the verified package name so removal targets the +// wrapper the user actually installed. async function executeUninstall(method: Installation.Method, targets: RemovalTargets, pkg: string) { + // altimate_change end const spinner = prompts.spinner() const errors: string[] = [] diff --git a/packages/opencode/src/installation/index.ts b/packages/opencode/src/installation/index.ts index df265e379..36a97c16e 100644 --- a/packages/opencode/src/installation/index.ts +++ b/packages/opencode/src/installation/index.ts @@ -416,8 +416,9 @@ export interface Interface { readonly method: () => Effect.Effect readonly latest: (method?: Method) => Effect.Effect readonly upgrade: (method: Method, target: string) => Effect.Effect - // altimate_change — #1305: verified owning package, or undefined when not ours + // altimate_change start — #1305: verified owning package, or undefined when not ours readonly packageName: () => Effect.Effect + // altimate_change end } export class Service extends Context.Service()("@opencode/Installation") {} @@ -732,14 +733,16 @@ export const layer: Layer.Layer Date: Tue, 15 Sep 2026 19:58:19 +0530 Subject: [PATCH 9/9] fix: one resolved identity; report the upgrade that actually happened (#1305) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses the CHANGES_REQUESTED review on 40779c55. Its three MAJOR findings are not independent — they are two invariants this change had not finished — so they are fixed together rather than patched individually. **One identity, resolved once.** Ownership was verified in `method()`, then independently re-probed in `preflight()`, again when building the install command, and once more in `uninstall`. Each was a separate manager query that could fail or race independent of the one before it, and `owningPackageOrScoped()` turned a later failure into a silent substitution of the scoped package name. For a confirmed UNSCOPED install that meant upgrade installed a second, scoped copy while the real one stayed stale — reporting success — and uninstall deleted the user's data, then removed a package that was never installed. `identity()` now resolves once and is memoised for the process; nothing it reads can change while the binary is running. `packageFor()` uses the verified owner, and when a caller forces `--method`, or ownership is unconfirmed, it logs that it is assuming the scoped name rather than substituting quietly. That also answers the third finding: an upgrade previously spawned ~7 manager subprocesses because every call site re-derived the answer. It is now a single query (two spawns for npm), while keeping verification before any mutating action. **Report what actually happened.** Telemetry recorded `status: "success"` and the CLI printed "Upgrade complete" BEFORE checking whether the running binary had changed; a mismatch only wrote a log warning. The check also used `text()`, which swallows a failed spawn and returns "", and an empty string was read as "nothing to verify" — so an upgrade that left the binary unrunnable reported success. Verification now runs first, via `run()` so the exit status is visible, and distinguishes three outcomes: a non-zero exit (the binary cannot start) and a contradicting version both fail the upgrade and record an error; exit 0 with no output is reported as unverifiable rather than being claimed either way. Review minors, all fixed: * uninstall no longer tells the user to "re-run" a command whose binary they have just removed; it names the data/config/cache/state directories to delete by hand. * per-manager removal and upgrade syntax — `bun remove -g` and `yarn global remove` are not ` uninstall -g`. * Windows recovery text no longer prints POSIX-only paths and `curl … | bash`; it uses `%USERPROFILE%\.altimate\bin` and the PowerShell installer. * `--method` no longer offers `choco`/`scoop`, which `Installation.upgrade()` always refuses. Help snapshot updated. * the ambiguous trailing "(or altimate-code)" is now an explicit sentence about the scoped vs unscoped package name. * `bunGlobalRoot()` falls back to BUN_INSTALL when a configured `install.globalBinDir` breaks the derivation, and returns "" when bun reports nothing. * stale comment claiming the failure log "stays local" — the claim `redactSecrets()`'s own docblock calls false — removed. * `ownership.test.ts` no longer leaks a temp directory per run; short-token redaction shapes pinned. Known limitation, deliberate: only the manager the path points at is queried. Probing every manager would resolve a custom layout whose directory carries no recognisable segment, but it multiplies the subprocess count this change exists to reduce, to rescue a case that already degrades safely to notify-only. Co-Authored-By: Claude Opus 5 (1M context) --- packages/opencode/src/cli/cmd/uninstall.ts | 20 +- packages/opencode/src/cli/cmd/upgrade.ts | 21 +- packages/opencode/src/installation/index.ts | 219 +++++++++++++----- .../__snapshots__/help-snapshots.test.ts.snap | 2 +- .../test/install/upgrade-method.test.ts | 4 +- .../test/installation/ownership.test.ts | 13 +- .../windows-installer-930.test.ts | 6 + 7 files changed, 211 insertions(+), 74 deletions(-) diff --git a/packages/opencode/src/cli/cmd/uninstall.ts b/packages/opencode/src/cli/cmd/uninstall.ts index 27b2da565..3b3949036 100644 --- a/packages/opencode/src/cli/cmd/uninstall.ts +++ b/packages/opencode/src/cli/cmd/uninstall.ts @@ -71,12 +71,24 @@ export const UninstallCommand = { // cares about and left the installation running, with no indication that had happened. // Data loss with nothing uninstalled is strictly worse than declining. if (method === "unknown") { + const win = process.platform === "win32" + const standalone = win ? "%USERPROFILE%\\.altimate\\bin" : "~/.altimate/bin" prompts.log.error(`Cannot determine how altimate was installed (running from ${process.execPath}).`) prompts.log.info("Uninstalling now would delete your data and config while leaving the program installed.") - prompts.log.info("Remove it with the tool you installed it with, then re-run to clean up data:") - prompts.log.info(" npm/pnpm/bun/yarn: uninstall -g @altimateai/altimate-code (or altimate-code)") - prompts.log.info(" Homebrew: brew uninstall altimate-code") - prompts.log.info(" install script: rm the binary from ~/.altimate/bin") + prompts.log.info("Remove the program with whichever tool installed it — each has its own syntax:") + prompts.log.info(" npm: npm uninstall -g altimate-code") + prompts.log.info(" pnpm: pnpm uninstall -g altimate-code") + prompts.log.info(" bun: bun remove -g altimate-code") + prompts.log.info(" yarn: yarn global remove altimate-code") + prompts.log.info(" Homebrew: brew uninstall altimate-code") + prompts.log.info(` installer: delete the binary from ${standalone}`) + prompts.log.info("If you installed the scoped package, use @altimateai/altimate-code as the name instead.") + // Do not tell the user to "re-run" this command: once the package is gone, so is the + // binary that would run it. Name the directories so data can be cleaned up by hand. + prompts.log.info("Then delete these directories to remove data, config, cache and state:") + for (const dir of [Global.Path.data, Global.Path.config, Global.Path.cache, Global.Path.state]) { + prompts.log.info(` ${dir}`) + } prompts.outro("Nothing was removed") return } diff --git a/packages/opencode/src/cli/cmd/upgrade.ts b/packages/opencode/src/cli/cmd/upgrade.ts index 3de80d361..f6d6c1c7e 100644 --- a/packages/opencode/src/cli/cmd/upgrade.ts +++ b/packages/opencode/src/cli/cmd/upgrade.ts @@ -23,7 +23,11 @@ export const UpgradeCommand = { alias: "m", describe: "installation method to use", type: "string", - choices: ["curl", "npm", "pnpm", "bun", "brew", "choco", "scoop"], + // altimate_change start — #1305: keep in step with UNSUPPORTED_UPGRADE_METHODS. + // choco/scoop were offered here but Installation.upgrade() always refuses them, so + // selecting either could only fail. + choices: ["curl", "npm", "pnpm", "bun", "brew"], + // altimate_change end }) }, handler: async (args: { target?: string; method?: string }) => { @@ -57,10 +61,17 @@ export const UpgradeCommand = { ? `Cannot determine how altimate was installed (running from ${process.execPath}).` : `Upgrading a ${method} installation is not supported.`, ) - prompts.log.info("Upgrade with the tool you installed it with:") - prompts.log.info(" npm/pnpm/bun: install -g @altimateai/altimate-code@latest (or altimate-code)") - prompts.log.info(" Homebrew: brew upgrade altimate-code") - prompts.log.info(" install script: curl -fsSL https://www.altimate.sh/install | bash") + prompts.log.info("Upgrade with whichever tool installed it:") + prompts.log.info(" npm: npm install -g altimate-code@latest") + prompts.log.info(" pnpm: pnpm install -g altimate-code@latest") + prompts.log.info(" bun: bun install -g altimate-code@latest") + prompts.log.info(" Homebrew: brew upgrade altimate-code") + prompts.log.info( + process.platform === "win32" + ? " installer: irm https://www.altimate.sh/install.ps1 | iex" + : " installer: curl -fsSL https://www.altimate.sh/install | bash", + ) + prompts.log.info("If you installed the scoped package, use @altimateai/altimate-code as the name instead.") prompts.log.info("Or force a specific manager with --method .") prompts.outro("Done") return diff --git a/packages/opencode/src/installation/index.ts b/packages/opencode/src/installation/index.ts index 36a97c16e..3ca06c816 100644 --- a/packages/opencode/src/installation/index.ts +++ b/packages/opencode/src/installation/index.ts @@ -217,8 +217,21 @@ function realExecPath(): string { * `bun pm bin -g` reports ~/.bun/bin, but globally installed packages live in a sibling * tree at ~/.bun/install/global/node_modules. Treating the shim dir as the package root * made every bun global install fail the ownership check as "not-global". */ -export function bunGlobalRoot(bin: string): string { - return bin ? path.join(path.dirname(bin), "install", "global", "node_modules") : "" +export function bunGlobalRoot(bin: string, env: NodeJS.ProcessEnv = process.env): string { + // BUN_INSTALL points at the install root directly and survives a configured + // `install.globalBinDir`, which otherwise breaks the derivation from the bin directory. + // No bin directory means bun told us nothing — do not invent a root from the environment. + if (!bin) return "" + const derived = path.join(path.dirname(bin), "install", "global", "node_modules") + if (fs.existsSync(derived)) return derived + // A configured `install.globalBinDir` breaks the derivation; BUN_INSTALL still points at + // the install root. + const fromEnv = env["BUN_INSTALL"] ? path.join(env["BUN_INSTALL"], "install", "global", "node_modules") : "" + if (fromEnv && fs.existsSync(fromEnv)) return fromEnv + // Neither exists: prefer the derived path so the caller still has something to check, and + // ownership simply finds no owner. A configured `install.globalDir` that matches neither + // shape degrades to notify-only rather than acting on a guess. + return derived || fromEnv } /** Resolve symlinks, tolerating paths that do not exist yet. @@ -339,6 +352,16 @@ export type Method = "curl" | "npm" | "yarn" | "pnpm" | "bun" | "brew" | "scoop" // rejects the same set instead of each maintaining its own list (they drifted: the v2 HTTP // handler checked only "unknown" and 500'd on the rest). export const UNSUPPORTED_UPGRADE_METHODS: Method[] = ["unknown", "yarn", "scoop", "choco"] + +/** One resolved answer about this install, shared by every consumer. */ +interface ResolvedIdentity { + readonly method: Method + /** Verified owning package — present only when a manager confirmed it. */ + readonly packageName?: string + readonly packageRoot: string + /** Directories an upgrade would write, for the permission preflight. */ + readonly writable: string[] +} // altimate_change end export type ReleaseType = "patch" | "minor" | "major" @@ -541,31 +564,60 @@ export const layer: Layer.Layer { // The name the manager confirmed owns this install — telling a user to reinstall the // other one would leave them with a duplicate and a stale original. @@ -612,7 +664,10 @@ export const layer: Layer.Layer (exit code N)." with nothing written anywhere. - // The log file is local and already carries this content on success, so logging - // it here is consistency, not new exposure — the user-facing message and the - // telemetry payload both stay redacted. + // Everything subprocess-derived goes through redactSecrets() first: the logger + // fans out to stderr under OPENCODE_PRINT_LOGS and to an OTLP collector when one + // is configured, so it is NOT local-only. const classified = classifyFailure(upgradeResult?.stderr ?? "", upgradeResult?.stdout ?? "") yield* Effect.logWarning("upgrade failed", { method: m, @@ -948,19 +1002,81 @@ export const layer: Layer.Layer v.trim().replace(/^v/, "") + const after = verify.stdout.trim() + // Three outcomes, not two. A NON-ZERO exit means the binary cannot run — that is the + // hole the old `text()` call hid, because it swallowed the failure and returned "". + // A clear, different version means the upgrade landed somewhere else. Exit 0 with no + // output is neither: we cannot verify, so we say so rather than failing a good + // upgrade or claiming one we did not confirm. + const unrunnable = verify.code !== 0 + const contradicted = after !== "" && normalize(after) !== normalize(target) + const T2 = yield* Effect.promise(() => getTelemetry()) + if (after === "" && !unrunnable) { + yield* Effect.logWarning("could not verify the upgraded binary", { + method: m, + target, + execPath: process.execPath, + hint: "the binary ran but reported no version; the upgrade itself reported success", + }) + } + if (unrunnable || contradicted) { + yield* Effect.logWarning("upgrade did not change the running binary", { + method: m, + target, + code: verify.code, + running: redactSecrets(after), + execPath: process.execPath, + hint: unrunnable + ? "the running executable could not be started after the upgrade" + : "the package manager reported success but wrote somewhere other than the running executable", + }) + T2.track({ + type: "upgrade_attempted", + timestamp: Date.now(), + session_id: T2.getContext().sessionId || "cli", + from_version: InstallationVersion, + to_version: target, + method: telemetryMethod, + status: "error", + error: `unverified: exit ${verify.code}`, + }) + const logFile = yield* Effect.promise(() => getLogFile()) + return yield* new UpgradeFailedError({ + stderr: [ + unrunnable + ? `${m} reported success, but ${process.execPath} could not be started afterwards (exit ${verify.code}).` + : `${m} reported success, but ${process.execPath} still reports ${after} rather than ${target}.`, + "The upgrade was most likely written to a different location than the binary you are running.", + logFile ? `Details were written to ${logFile}.` : undefined, + ] + .filter(Boolean) + .join(" "), + }) + } yield* Effect.logInfo("upgraded", { method: m, target, stdout: redactSecrets(upgradeResult.stdout), stderr: redactSecrets(upgradeResult.stderr), }) - // altimate_change end - // altimate_change start — telemetry for upgrade success - const T2 = yield* Effect.promise(() => getTelemetry()) T2.track({ type: "upgrade_attempted", timestamp: Date.now(), @@ -971,25 +1087,6 @@ export const layer: Layer.Layer v.trim().replace(/^v/, "") - if (after && normalize(after) !== normalize(target)) { - yield* Effect.logWarning("upgrade did not change the running binary", { - method: m, - target, - // Subprocess output — redacted like every other logged field. This one was added - // in an earlier round of this change and missed the redactor. - running: redactSecrets(after), - execPath: process.execPath, - hint: "the package manager wrote somewhere other than the running executable's location", - }) - } - // altimate_change end }), } diff --git a/packages/opencode/test/cli/help/__snapshots__/help-snapshots.test.ts.snap b/packages/opencode/test/cli/help/__snapshots__/help-snapshots.test.ts.snap index 1d20dbf51..ea905cad2 100644 --- a/packages/opencode/test/cli/help/__snapshots__/help-snapshots.test.ts.snap +++ b/packages/opencode/test/cli/help/__snapshots__/help-snapshots.test.ts.snap @@ -237,7 +237,7 @@ Options: engine serve the types it provides; 'local' keeps every connection on the local drivers [string] [choices: "workspace", "local"] -m, --method installation method to use - [string] [choices: "curl", "npm", "pnpm", "bun", "brew", "choco", "scoop"]" + [string] [choices: "curl", "npm", "pnpm", "bun", "brew"]" `; exports[`opencode CLI help-text snapshots every documented command emits stable help text: opencode uninstall --help 1`] = ` diff --git a/packages/opencode/test/install/upgrade-method.test.ts b/packages/opencode/test/install/upgrade-method.test.ts index cd5f4e7e5..ab53404ac 100644 --- a/packages/opencode/test/install/upgrade-method.test.ts +++ b/packages/opencode/test/install/upgrade-method.test.ts @@ -96,8 +96,8 @@ describe("upgrade execution", () => { // which returns whichever Altimate package OWNS the running install (publish.ts ships a // scoped and an unscoped one; upgrading with the wrong name installs a second copy). // The brand contract is unchanged: both candidates are ours, never `opencode-ai`. - expect(INSTALLATION_SRC).toContain("${yield* owningPackageOrScoped(m)}@${target}") - const helper = INSTALLATION_SRC.split("\n").find((l) => l.includes("owningPackage(m)) ??")) + expect(INSTALLATION_SRC).toContain("${yield* packageFor(m)}@${target}") + const helper = INSTALLATION_SRC.split("\n").find((l) => l.includes('assuming: "@altimateai/altimate-code"')) expect(helper).toBeDefined() expect(helper).toContain("@altimateai/altimate-code") expect(helper).not.toContain("opencode-ai") diff --git a/packages/opencode/test/installation/ownership.test.ts b/packages/opencode/test/installation/ownership.test.ts index 46380c888..f1e0c2e52 100644 --- a/packages/opencode/test/installation/ownership.test.ts +++ b/packages/opencode/test/installation/ownership.test.ts @@ -4,7 +4,7 @@ * `resolveInstall()` answers from the path alone, which cannot prove that the running * binary belongs to a manager's GLOBAL tree. These cover the two pieces that decide it. */ -import { describe, test, expect } from "bun:test" +import { describe, test, expect, afterAll } from "bun:test" import fs from "fs" import os from "os" import path from "path" @@ -32,6 +32,9 @@ describe("bunGlobalRoot", () => { describe("isInside", () => { const tmp = fs.mkdtempSync(path.join(os.tmpdir(), "ownership-")) + // altimate_change — #1305: these ran on every invocation and never cleaned up, leaving a + // directory behind in the OS temp dir each time. + afterAll(() => fs.rmSync(tmp, { recursive: true, force: true })) const parent = path.join(tmp, "node_modules") const sibling = path.join(tmp, "node_modules-other") fs.mkdirSync(parent, { recursive: true }) @@ -79,6 +82,7 @@ describe("ownerOf", () => { // Real directories, because the whole point is that ownership is a filesystem fact rather // than something inferable from the path string. const root = fs.mkdtempSync(path.join(os.tmpdir(), "owner-")) + afterAll(() => fs.rmSync(root, { recursive: true, force: true })) const mk = (p: string) => { const full = path.join(root, p) fs.mkdirSync(path.dirname(full), { recursive: true }) @@ -208,6 +212,11 @@ describe("redactSecrets", () => { ["basic auth blob", "Authorization: Basic dXNlcjpwYXNzd29yZDEyMw=="], ["quoted json key", '{"token":"short-secret"}'], ["bare url userinfo", "https://short-secret@registry.example/pkg"], + // Short unlabelled tokens are the known gap: the catch-all patterns need 32+ hex or + // 40+ base64 chars, so a short secret only gets masked when it carries a key or a + // recognisable prefix. This pins the shapes that DO work. + ["short token with key", "npm_config_authToken=abc123"], + ["short prefixed token", "npm_abcd1234efgh"], ] for (const [name, input] of cases) { test(`masks ${name}`, () => { @@ -221,6 +230,8 @@ describe("redactSecrets", () => { "s3cr3t-value-here", "dXNlcjpwYXNzd29yZDEyMw==", "short-secret", + "abc123", + "abcd1234efgh", ]) { if (input.includes(secret)) expect(out).not.toContain(secret) } diff --git a/packages/opencode/test/release-validation/windows-installer-930.test.ts b/packages/opencode/test/release-validation/windows-installer-930.test.ts index b5e253ed1..a60c981d5 100644 --- a/packages/opencode/test/release-validation/windows-installer-930.test.ts +++ b/packages/opencode/test/release-validation/windows-installer-930.test.ts @@ -138,6 +138,10 @@ describe("upgrade('curl', target) — platform dispatch", () => { }, spawn: (call) => { spawnCalls.push(call) + // altimate_change — #1305: upgrade() now verifies the running binary reports the + // target version before claiming success, so the mock has to answer that probe. + // These tests cover platform dispatch, not verification. + if (call.args?.includes("--version")) return { code: 0, stdout: "1.2.3", stderr: "" } return { code: 0, stdout: "ok", stderr: "" } }, }) @@ -175,6 +179,8 @@ describe("upgrade('curl', target) — platform dispatch", () => { spawn: (call) => { spawnCalls.push(call) if (call.cmd === "bash" && call.args[0] === "--version") return "GNU bash" + // altimate_change — #1305: answer upgrade()'s post-upgrade version probe. + if (call.args?.includes("--version")) return { code: 0, stdout: "1.2.3", stderr: "" } return { code: 0, stdout: "done", stderr: "" } }, })