Skip to content

Commit 6ee55b6

Browse files
saravmajesticclaude
saravmajestic
and
claude
committed
fix: detect the unscoped npm wrapper; redact the success path too (#1305)
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 `<wrapper>/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) <[email protected]>
1 parent f228cb2 commit 6ee55b6

2 files changed

Lines changed: 75 additions & 12 deletions

File tree

‎packages/opencode/src/installation/index.ts‎

Lines changed: 40 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -65,16 +65,23 @@ const UPGRADE_FETCH_TIMEOUT_MS = 15_000
6565
// The directory checks that ran before the probe loop were sound and are preserved
6666
// below; only the probe loop is replaced.
6767
//
68-
// The running binary's own path is the ground truth. The npm `bin/altimate` shim is
69-
// a Node script that spawnSync()s the PLATFORM package's binary, so inside the CLI
70-
// process.execPath is:
71-
// <prefix>/lib/node_modules/@altimateai/altimate-code/node_modules/
72-
// @altimateai/altimate-code-darwin-arm64/bin/altimate-code
73-
// i.e. it always lands under node_modules for every package-manager install. Match
74-
// the optional `-<platform>-<arch>` suffix explicitly rather than relying on the
75-
// wrapper name happening to be a prefix of the platform package name.
68+
// The running binary's own path is the ground truth, but it takes THREE shapes and the
69+
// scope prefix is optional — `publish.ts` ships an unscoped `altimate-code` wrapper
70+
// alongside the scoped one, and that unscoped package is what README.md:30 and
71+
// docs/docs/getting-started.md:27 tell users to install:
72+
//
73+
// <prefix>/lib/node_modules/altimate-code/bin/.altimate-code (unscoped wrapper,
74+
// <prefix>/lib/node_modules/@altimateai/altimate-code/bin/.altimate-code cached hardlink)
75+
// <prefix>/lib/node_modules/.../@altimateai/altimate-code-darwin-arm64/bin/altimate-code
76+
//
77+
// The first two are what actually run: postinstall.mjs hard-links the resolved platform
78+
// binary to `<wrapper>/bin/.altimate-code` and both shims execute that cached file BEFORE
79+
// walking to the nested platform package. A hardlink has no symlink for realpath to follow,
80+
// so execPath keeps the wrapper's path and loses the platform suffix entirely. Requiring the
81+
// `@altimateai` segment therefore reported "unknown" for the primary documented install
82+
// path, silently disabling auto-upgrade for most real users.
7683
const PKG_SEGMENT_RE =
77-
/[\\/]node_modules[\\/]@altimateai[\\/]altimate-code(?:-[a-z0-9]+-[a-z0-9]+(?:-[a-z0-9]+)?)?(?:[\\/]|$)/i
84+
/[\\/]node_modules[\\/](?:@altimateai[\\/])?altimate-code(?:-[a-z0-9]+-[a-z0-9]+(?:-[a-z0-9]+)?)?(?:[\\/]|$)/i
7885
// pnpm global installs may expose the package via the `.pnpm` virtual store OR via a
7986
// plain `pnpm/global/<v>` link path (no `.pnpm` segment), so match both spellings —
8087
// otherwise the plain layout falls through to the npm default and routes upgrades at
@@ -844,12 +851,17 @@ export const layer: Layer.Layer<Service, never, HttpClient.HttpClient | AppProce
844851
// altimate_change end
845852
}
846853
// altimate_change end
854+
// altimate_change start — #1305: this path has always logged raw subprocess output,
855+
// which fans out to stderr (OPENCODE_PRINT_LOGS=1) and to an OTLP collector when one
856+
// is configured. Redact here too — "the existing pattern already does this" is not a
857+
// reason for either path to keep doing it.
847858
yield* Effect.logInfo("upgraded", {
848859
method: m,
849860
target,
850-
stdout: upgradeResult.stdout,
851-
stderr: upgradeResult.stderr,
861+
stdout: redactSecrets(upgradeResult.stdout),
862+
stderr: redactSecrets(upgradeResult.stderr),
852863
})
864+
// altimate_change end
853865
// altimate_change start — telemetry for upgrade success
854866
const T2 = yield* Effect.promise(() => getTelemetry())
855867
T2.track({
@@ -862,7 +874,23 @@ export const layer: Layer.Layer<Service, never, HttpClient.HttpClient | AppProce
862874
status: "success",
863875
})
864876
// altimate_change end
865-
yield* text([process.execPath, "--version"])
877+
// altimate_change start — #1305: this call previously discarded its output, so an
878+
// upgrade that wrote to a DIFFERENT location than the running binary reported success
879+
// while the executable on disk was unchanged. We cannot fail the operation on this
880+
// (the package manager did succeed, and a version string can legitimately differ for
881+
// dev/branch builds), but it must not pass silently.
882+
const after = (yield* text([process.execPath, "--version"])).trim()
883+
const normalize = (v: string) => v.trim().replace(/^v/, "")
884+
if (after && normalize(after) !== normalize(target)) {
885+
yield* Effect.logWarning("upgrade did not change the running binary", {
886+
method: m,
887+
target,
888+
running: after,
889+
execPath: process.execPath,
890+
hint: "the package manager wrote somewhere other than the running executable's location",
891+
})
892+
}
893+
// altimate_change end
866894
}),
867895
}
868896

‎packages/opencode/test/installation/resolve-install.test.ts‎

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -80,6 +80,41 @@ describe("resolveInstall", () => {
8080
expect(resolveInstall("/home/u/.altimate/bin/altimate", {}).binDir).toBe("/home/u/.altimate/bin")
8181
})
8282

83+
// sahrizvi review — the shapes that actually run in production. postinstall.mjs hard-links
84+
// the platform binary into `<wrapper>/bin/.altimate-code` and both shims execute that cached
85+
// file first, so after the first run execPath is the WRAPPER's path with no platform suffix.
86+
// `publish.ts` also ships an unscoped `altimate-code` package, which is what README.md:30
87+
// and the getting-started docs tell users to install — so the scope prefix is optional.
88+
test("unscoped wrapper, cached hardlink (the documented npm install) -> npm", () => {
89+
expect(
90+
resolveInstall("/usr/local/lib/node_modules/altimate-code/bin/.altimate-code", {}).method,
91+
).toBe("npm")
92+
})
93+
94+
test("scoped wrapper, cached hardlink -> npm", () => {
95+
expect(
96+
resolveInstall("/usr/local/lib/node_modules/@altimateai/altimate-code/bin/.altimate-code", {}).method,
97+
).toBe("npm")
98+
})
99+
100+
test("unscoped wrapper, nested platform package -> npm", () => {
101+
expect(
102+
resolveInstall(
103+
"/usr/local/lib/node_modules/altimate-code/node_modules/@altimateai/altimate-code-darwin-arm64/bin/altimate-code",
104+
{},
105+
).method,
106+
).toBe("npm")
107+
})
108+
109+
test("unscoped wrapper under a pnpm global root -> pnpm", () => {
110+
expect(
111+
resolveInstall(
112+
"/home/u/.local/share/pnpm/global/5/node_modules/altimate-code/bin/.altimate-code",
113+
{},
114+
).method,
115+
).toBe("pnpm")
116+
})
117+
83118
// Review findings on #1306 — layouts that contain a package segment but are NOT a
84119
// global install. Attributing them to a manager would make upgrade() run `install -g`
85120
// and CREATE a global install the user never had (automatically, for patch releases).

0 commit comments

Comments
 (0)