diff --git a/packages/opencode/src/cli/cmd/uninstall.ts b/packages/opencode/src/cli/cmd/uninstall.ts index 935967cbe..3b3949036 100644 --- a/packages/opencode/src/cli/cmd/uninstall.ts +++ b/packages/opencode/src/cli/cmd/uninstall.ts @@ -62,9 +62,46 @@ 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") { + 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 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 + } + // 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" + await showRemovalSummary(targets, method, pkg) + // altimate_change end if (!args.force && !args.dryRun) { const confirm = await prompts.confirm({ @@ -83,7 +120,10 @@ export const UninstallCommand = { return } - await executeUninstall(method, targets) + // 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") }, @@ -103,7 +143,10 @@ async function collectRemovalTargets(args: UninstallArgs, method: Installation.M return { directories, shellConfig, binary } } -async function showRemovalSummary(targets: RemovalTargets, method: Installation.Method) { +// 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) { @@ -130,20 +173,26 @@ 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 ${pkg}`, + pnpm: `pnpm uninstall -g ${pkg}`, + bun: `bun remove -g ${pkg}`, + yarn: `yarn global remove ${pkg}`, + brew: "brew uninstall altimate-code", } + // altimate_change end prompts.log.info(` ✓ Package: ${cmds[method] || method}`) } } -async function executeUninstall(method: Installation.Method, targets: RemovalTargets) { +// 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[] = [] @@ -181,22 +230,26 @@ 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", pkg], + pnpm: ["pnpm", "uninstall", "-g", pkg], + bun: ["bun", "remove", "-g", pkg], + yarn: ["yarn", "global", "remove", pkg], + 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, { + // 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")}` diff --git a/packages/opencode/src/cli/cmd/upgrade.ts b/packages/opencode/src/cli/cmd/upgrade.ts index 4b935b6fa..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 }) => { @@ -46,23 +50,33 @@ 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 — 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 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 } + // 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 0fb8ab275..3ca06c816 100644 --- a/packages/opencode/src/installation/index.ts +++ b/packages/opencode/src/installation/index.ts @@ -8,12 +8,29 @@ 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 os from "os" 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 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. + return path.join(Global.Path.log, "opencode.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() { @@ -37,8 +54,316 @@ 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 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, 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 +// 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[\\/]global)[\\/]/i +const BUN_SEGMENT_RE = /[\\/]\.bun[\\/]/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 +const SCOOP_SEGMENT_RE = /[\\/]scoop[\\/]apps[\\/]/i +const CHOCO_SEGMENT_RE = /[\\/]chocolatey[\\/]/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 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 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" +// for these, so they must keep degrading to notify-only. +const PACKAGE_MANAGERS: Method[] = ["npm", "pnpm", "bun", "yarn"] +const EPHEMERAL_SEGMENT_RE = /[\\/](?:_npx|_cacache|dlx|\.npx)[\\/]|[\\/]install[\\/]cache[\\/]|[\\/]dlx-[^\\/]+[\\/]/i + +export interface ResolvedInstall { + readonly method: Method +} + +/** 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, + * 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, +): 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) && !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" } + return { method: "npm" } + } + 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, 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" } + return { method: "unknown" } +} + +/** 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 { + return fs.realpathSync(process.execPath) + } catch { + return process.execPath + } +} + +/** 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, 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. + * + * 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 + * (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) + return true + } catch { + return false + } +} + +/** 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. */ +export function redactSecrets(input: string): string { + if (!input) return input + 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 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)) + 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" +// 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"] + +/** 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" export const Event = { @@ -114,6 +439,9 @@ export interface Interface { readonly method: () => Effect.Effect readonly latest: (method?: Method) => Effect.Effect readonly upgrade: (method: Method, target: string) => Effect.Effect + // 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") {} @@ -170,13 +498,195 @@ 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 return `Upgrade failed for ${method}.` } +// altimate_change start — writability preflight (#1305) + /** Directories a global install of `m` would mutate. Empty = nothing cheap to check. */ + /** Where a global install of `m` actually lives. + * + * `packageRoot` is the directory that CONTAINS the installed package — ownership is + * decided against this and NEVER against the shim/bin directory. They are different + * directories for bun (`bun pm bin -g` is ~/.bun/bin; packages live under + * ~/.bun/install/global/node_modules) and for yarn, and conflating them rejected every + * global bun install as "not-global". + * + * `writable` is the set of directories the upgrade actually writes, checked for + * permission. A global install writes both the package tree and the shim dir. */ + const globalLayout = Effect.fnUntraced(function* (m: Method) { + const empty = { packageRoot: "", writable: [] as string[] } + switch (m) { + case "npm": { + // `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 { packageRoot: root, writable: [root, bin].filter(Boolean) } + } + case "pnpm": { + const root = (yield* text(["pnpm", "root", "-g"])).trim() + const bin = (yield* text(["pnpm", "bin", "-g"])).trim() + return { packageRoot: root, writable: [root, bin].filter(Boolean) } + } + case "bun": { + const bin = (yield* text(["bun", "pm", "bin", "-g"])).trim() + 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 { packageRoot: root, writable: [root, bin].filter(Boolean) } + } + 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 empty + } + }) + + /** The install that produced this process, resolved ONCE and reused. + * + * Previously this was re-derived at every call site — `method()`, then again in + * `preflight()`, then again when building the install command, then once more in + * `uninstall`. Each was a separate manager query that could fail or race independently + * of the one before it, and a later failure silently substituted the scoped package + * name: an unscoped install would be "upgraded" by installing a second, scoped copy + * while the real one stayed stale — with the command still reporting success. Uninstall + * had the mirror bug: delete the user's data, then remove a package that was never + * installed. + * + * One resolution, memoised for the life of the process, removes that class of bug and + * takes an upgrade from ~7 manager subprocesses down to a single query (two spawns for + * npm). Nothing it reads can change during a run — the manager that owns the running + * binary cannot change while that binary is executing. + * + * Only the manager the path points at is asked. Probing every manager would also find + * the owner of a custom layout whose directory carries no recognisable segment, but it + * multiplies the subprocess count this change exists to reduce in order to rescue a case + * that already degrades safely to notify-only. Known limitation, recorded deliberately. */ + let cached: ResolvedIdentity | undefined + const identity = Effect.fnUntraced(function* () { + if (cached) return cached + const candidate = resolveInstall().method + const layout = yield* globalLayout(candidate) + if (!PACKAGE_MANAGERS.includes(candidate)) { + cached = { method: candidate, packageRoot: layout.packageRoot, writable: layout.writable } + return cached + } + const owner = ownerOf(layout.packageRoot, realExecPath()) + cached = owner + ? { method: candidate, packageName: owner, packageRoot: layout.packageRoot, writable: layout.writable } + : { method: "unknown" as Method, packageRoot: "", writable: [] } + return cached + }) + + /** The package to install or remove for `m`. + * + * When `m` is the resolved method we use the VERIFIED owner. An explicit `--method` + * override, or an install whose ownership could not be confirmed, cannot tell us which + * of our two published wrappers is present — so we say so in the log rather than quietly + * picking one and reporting success. */ + const packageFor = Effect.fnUntraced(function* (m: Method) { + const id = yield* identity() + if (m === id.method && id.packageName) return id.packageName + yield* Effect.logWarning("package name not verified — assuming the scoped wrapper", { + requested: m, + resolved: id.method, + assuming: "@altimateai/altimate-code", + }) + return "@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. + return process.platform === "win32" + ? `Cannot write to the npm global prefix (${dir}). ` + + `Re-run \`npm install -g ${pkg}\` from an elevated (Administrator) shell, ` + + `or switch to a user-owned prefix with \`npm config set prefix\`.` + : `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}).` + } + } + + /** A refusal carrying the classification code so the blocked attempt is still recorded + * as that KIND of failure rather than vanishing from telemetry as "no attempt". */ + const preflightBlock = (code: string, message: string) => ({ 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 + * 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 id = yield* identity() + // Reuse the resolution we already made when it describes this method; an explicit + // override still needs its own lookup. + const layout = m === id.method ? { writable: id.writable } : yield* globalLayout(m) + // Ownership is NOT re-checked here. `Installation.method()` already refuses to return + // a package-manager identity unless the manager confirms it owns the running binary, so + // every automatic path is covered before it gets this far. A caller that passes an + // explicit `--method` is overriding detection deliberately; refusing it here would only + // stop the user doing what they asked for, and would double the manager queries. + 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. + if (!fs.existsSync(dir)) continue + if (!isWritable(dir)) { + const owner = yield* packageFor(m) + return preflightBlock("permission", remediation(m, dir, target, owner)) + } + } + return undefined + }) + // altimate_change end + const upgradeScriptShell = Effect.fnUntraced(function* () { const bashVersion = yield* text(["bash", "--version"]) if (bashVersion) return "bash" @@ -260,53 +770,33 @@ 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 - } + // altimate_change start — resolve from the running binary instead of guessing (#1305). + // Replaces a substring test on execPath plus a loop that spawned up to seven package + // managers ("npm list -g", "brew list", ...) on the startup update-check path. + const candidate = resolveInstall().method + // A path match is a CANDIDATE, not proof of ownership: a project-local node_modules + // is shaped exactly like a global one. Confirm it here rather than at the upgrade + // boundary, because `cli/cmd/uninstall.ts` acts on this answer DESTRUCTIVELY and + // never runs the upgrade preflight. Costs one manager query (two spawns for npm: + // `root -g` and `prefix -g`) versus up to seven probes before, and only when the path + // already looks like a package manager. + if (PACKAGE_MANAGERS.includes(candidate)) { + // No owning package found — a project-local install, a transitive dependency of + // another global CLI, a manager we cannot query, or a foreign tree. Degrade to + // notify-only rather than acting on a guess. + return (yield* identity()).method } - - return "unknown" as Method + return candidate }), + // altimate_change end + // altimate_change start — #1305: the package the manager confirms owns this binary, + // so upgrade and uninstall name the wrapper the user actually installed. + packageName: Effect.fn("Installation.packageName")(function* () { + const candidate = resolveInstall().method + if (!PACKAGE_MANAGERS.includes(candidate)) return undefined + return (yield* identity()).packageName + }), + // altimate_change end latest: Effect.fn("Installation.latest")(function* (installMethod?: Method) { const detectedMethod = installMethod || (yield* result.method()) @@ -376,6 +866,27 @@ 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) { case "curl": @@ -386,17 +897,17 @@ export const layer: Layer.Layer (exit code N)." with nothing written anywhere. + // 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, + target, + code: upgradeResult?.code, + reason: classified.code, + stdout: redactSecrets(upgradeResult?.stdout ?? ""), + stderr: redactSecrets(upgradeResult?.stderr ?? ""), + }) + const logFile = yield* Effect.promise(() => getLogFile()) + const base = upgradeFailure(m, upgradeResult) + const stderr = [ + base, + classified.hint ? `Likely cause: ${classified.hint}.` : undefined, + logFile ? `Details were written to ${logFile}.` : undefined, + ] + .filter(Boolean) + .join(" ") const T = yield* Effect.promise(() => getTelemetry()) T.track({ type: "upgrade_attempted", @@ -449,19 +994,89 @@ 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: upgradeResult.stdout, - stderr: upgradeResult.stderr, + stdout: redactSecrets(upgradeResult.stdout), + stderr: redactSecrets(upgradeResult.stderr), }) - // altimate_change start — telemetry for upgrade success - const T2 = yield* Effect.promise(() => getTelemetry()) T2.track({ type: "upgrade_attempted", timestamp: Date.now(), @@ -472,7 +1087,6 @@ 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 78c6557d5..a8ccbac31 100644 --- a/packages/opencode/src/server/routes/global.ts +++ b/packages/opencode/src/server/routes/global.ts @@ -309,9 +309,12 @@ 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: 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 // 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/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/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/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 653ee77e4..ab53404ac 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,30 @@ 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("all three standalone directories are still recognised", () => { + // 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 }) }) @@ -73,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* 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") + // altimate_change end }) test("brew upgrade taps AltimateAI/tap", () => { 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/ownership.test.ts b/packages/opencode/test/installation/ownership.test.ts new file mode 100644 index 000000000..f1e0c2e52 --- /dev/null +++ b/packages/opencode/test/installation/ownership.test.ts @@ -0,0 +1,249 @@ +/** + * 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, afterAll } from "bun:test" +import fs from "fs" +import os from "os" +import path from "path" +import { isInside, bunGlobalRoot, ownerOf, redactSecrets } 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-")) + // 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 }) + 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) + }) +}) + +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 }) + 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"], + // 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}`, () => { + const out = redactSecrets(input) + expect(out).toContain("[REDACTED]") + for (const secret of [ + "abc123def456ghi", + "abcdef123456", + "hunter2", + "ABCDEFGHIJKLMNOPQRSTUVWXYZ012345", + "s3cr3t-value-here", + "dXNlcjpwYXNzd29yZDEyMw==", + "short-secret", + "abc123", + "abcd1234efgh", + ]) { + 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 new file mode 100644 index 000000000..46a1ee888 --- /dev/null +++ b/packages/opencode/test/installation/resolve-install.test.ts @@ -0,0 +1,152 @@ +/** + * 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"], + // 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", + "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"], + ["standalone, distro-resolved ~/.local/bin", "/home/u/.local/bin/altimate", "curl"], + // 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. + ["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") + }) + + // 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). + 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 — + // see the npm-under-~/.local case above, which resolves to npm rather than here. + expect(resolveInstall("/home/u/.local/bin/altimate", {}).method).toBe("curl") + }) +}) 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..a60c981d5 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))) @@ -142,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: "" } }, }) @@ -179,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: "" } }, }) @@ -341,15 +343,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 +458,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", () => { 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", () => {