Skip to content

Commit 60adaf0

Browse files
committed
fix(update): verify installs and stop crashing doctor on native builds
1 parent 82951c6 commit 60adaf0

10 files changed

Lines changed: 611 additions & 34 deletions

File tree

apps/pythinker-code/src/cli/sub/doctor.ts

Lines changed: 14 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,7 @@ import {
2424
} from '#/cli/update/preflight';
2525
import { detectInstallSource } from '#/cli/update/source';
2626
import type { UpdateInstallFailure } from '#/cli/update/types';
27-
import { getHostPackageRoot, getVersion } from '#/cli/version';
27+
import { findHostPackageRoot, getVersion } from '#/cli/version';
2828
import { getUpdateInstallLogFile } from '#/utils/paths';
2929

3030
interface WritableLike {
@@ -50,7 +50,8 @@ export interface DoctorDeps {
5050
export interface DoctorRuntimeInfo {
5151
readonly version: string;
5252
readonly installSource: string;
53-
readonly packageRoot: string;
53+
/** Absent on a native binary: a packaged install has no `package.json`. */
54+
readonly packageRoot?: string;
5455
readonly executable: string;
5556
readonly installations?: readonly string[];
5657
readonly ripgrep?: RgResolution;
@@ -62,6 +63,7 @@ export interface DoctorRuntimeInfo {
6263
readonly pendingVersion?: string;
6364
readonly pendingRequestedBy?: 'automatic' | 'manual';
6465
readonly activeOperation?: string;
66+
readonly lastSuccess?: string;
6567
readonly lastFailure?: string;
6668
readonly logPath?: string;
6769
};
@@ -191,7 +193,7 @@ function resolveDeps(deps: Partial<DoctorDeps> | DoctorDeps | undefined): Resolv
191193
return {
192194
version: getVersion(),
193195
installSource,
194-
packageRoot: getHostPackageRoot(),
196+
packageRoot: findHostPackageRoot() ?? undefined,
195197
executable: process.execPath,
196198
installations,
197199
ripgrep,
@@ -206,6 +208,11 @@ function resolveDeps(deps: Partial<DoctorDeps> | DoctorDeps | undefined): Resolv
206208
installState.active === null
207209
? undefined
208210
: `${installState.active.operation ?? 'install'} ${installState.active.version}`,
211+
lastSuccess:
212+
installState.lastSuccess === null
213+
? undefined
214+
: `${installState.lastSuccess.version} (installed ` +
215+
`${installState.lastSuccess.installedAt})`,
209216
lastFailure:
210217
installState.lastFailure === null
211218
? undefined
@@ -387,7 +394,7 @@ function formatRuntimeInfo(info: DoctorRuntimeInfo | undefined): string[] {
387394
'Runtime',
388395
` Version: ${info.version}`,
389396
` Install source: ${info.installSource}`,
390-
` Package root: ${info.packageRoot}`,
397+
...(info.packageRoot === undefined ? [] : [` Package root: ${info.packageRoot}`]),
391398
` Executable: ${info.executable}`,
392399
...(installations.length > 1
393400
? [
@@ -414,6 +421,9 @@ function formatRuntimeInfo(info: DoctorRuntimeInfo | undefined): string[] {
414421
...(info.update.activeOperation === undefined
415422
? []
416423
: [` Update operation: ${info.update.activeOperation}`]),
424+
...(info.update.lastSuccess === undefined
425+
? []
426+
: [` Last update success: ${info.update.lastSuccess}`]),
417427
...(info.update.lastFailure === undefined
418428
? []
419429
: [` Last update failure: ${info.update.lastFailure}`]),

apps/pythinker-code/src/cli/update/preflight.ts

Lines changed: 35 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -56,9 +56,13 @@ import {
5656
type UpdateRequestOrigin,
5757
type UpdateTarget,
5858
} from './types';
59+
import { verifyInstalledVersion, type InstallVerification } from './verify-install';
5960

6061
export type { UpdatePreflightResult } from './types';
6162

63+
/** Reused for the paths that never reach verification (a failed install). */
64+
const OK_VERIFICATION: InstallVerification = { ok: true };
65+
6266
export interface RunUpdatePreflightOptions {
6367
readonly stdout?: { write(chunk: string): boolean };
6468
readonly stderr?: { write(chunk: string): boolean };
@@ -81,6 +85,19 @@ function bunCommand(platform: NodeJS.Platform): string {
8185
return platform === 'win32' ? 'bun.exe' : 'bun';
8286
}
8387

88+
/**
89+
* Node ≥18.20/20.12 refuses to spawn a `.cmd`/`.bat` file without a shell
90+
* (CVE-2024-27980) and fails with `EINVAL`, which is every npm-family update
91+
* on Windows: `npm.cmd`, `pnpm.cmd`, `yarn.cmd`. Only the package manager
92+
* wrappers need it — the arguments are a fixed flag list plus
93+
* `<package>@<semver>`, so nothing here reaches the shell as data.
94+
*/
95+
export function needsShell(cmd: string, platform: NodeJS.Platform): boolean {
96+
if (platform !== 'win32') return false;
97+
const lower = cmd.toLowerCase();
98+
return lower.endsWith('.cmd') || lower.endsWith('.bat');
99+
}
100+
84101
export function installCommandFor(
85102
source: InstallSource,
86103
version: string,
@@ -548,6 +565,7 @@ export async function installUpdate(
548565
await new Promise<void>((resolve, reject) => {
549566
const child = spawn(cmd, [...args], {
550567
stdio: 'inherit',
568+
shell: needsShell(cmd, platform),
551569
env: env === undefined ? undefined : { ...process.env, ...env },
552570
});
553571
child.once('error', reject);
@@ -560,6 +578,11 @@ export async function installUpdate(
560578
reject(new Error(`${cmd} exited with ${detail}`));
561579
});
562580
});
581+
// Exit code 0 is the installer's opinion; this is the fact. Rejecting here
582+
// routes a silent no-op install into the same failure reporting a crashed
583+
// installer gets, instead of printing "Updated …" over an unchanged binary.
584+
const verification = await verifyInstalledVersion(source, version);
585+
if (!verification.ok) throw new Error(verification.reason);
563586
}
564587

565588
/** Keep the tail only: installers can be chatty, and the state file is small. */
@@ -861,11 +884,19 @@ async function startBackgroundInstall(
861884
// `settled` already stops new progress writes; drain the ones in flight so
862885
// none of them renames over the outcome below.
863886
await progressWrites;
887+
// An installer that exits 0 without replacing the binary must not be
888+
// recorded as a success: the footer would advertise "restart to apply"
889+
// for a version that never runs, on every launch, forever.
890+
const verification = succeeded
891+
? await verifyInstalledVersion(source, target.version)
892+
: OK_VERIFICATION;
893+
const installed = succeeded && verification.ok;
894+
const outcomeReason = verification.ok ? reason : verification.reason;
864895
const attempts = failureAttemptsFor(startedState, target, 'install') + 1;
865896
const stderrTail = readStderrTail();
866-
const message = stderrTail === undefined ? reason : `${reason}: ${stderrTail}`;
897+
const message = stderrTail === undefined ? outcomeReason : `${outcomeReason}: ${stderrTail}`;
867898

868-
const nextState: UpdateInstallState = succeeded
899+
const nextState: UpdateInstallState = installed
869900
? {
870901
...startedState,
871902
active: null,
@@ -889,7 +920,7 @@ async function startBackgroundInstall(
889920
};
890921
try {
891922
await writeUpdateInstallState(nextState).catch(() => {});
892-
if (succeeded) {
923+
if (installed) {
893924
trackUpdateEvent(track, 'update_background_install_succeeded', {
894925
target_version: target.version,
895926
source,
@@ -921,6 +952,7 @@ async function startBackgroundInstall(
921952
// A detached child gets its own console window on Windows regardless
922953
// of stdio; stdio: 'ignore' alone does not suppress it.
923954
windowsHide: platform === 'win32',
955+
shell: needsShell(cmd, platform),
924956
// stdout stays discarded (install progress is noise); stderr is piped so
925957
// the installer's machine-readable progress lines can be recorded and a
926958
// failure still keeps the installer's own error text.

apps/pythinker-code/src/cli/update/source.ts

Lines changed: 24 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -76,9 +76,21 @@ function npmCommand(platform: NodeJS.Platform): string {
7676
return platform === 'win32' ? 'npm.cmd' : 'npm';
7777
}
7878

79-
function execFileText(command: string, args: readonly string[]): Promise<string> {
79+
function execFileText(
80+
command: string,
81+
args: readonly string[],
82+
platform: NodeJS.Platform = process.platform,
83+
): Promise<string> {
8084
return new Promise((resolveOutput, reject) => {
81-
execFile(command, [...args], { encoding: 'utf-8' }, (error, stdout) => {
85+
// `npm.cmd` cannot be spawned without a shell on Node ≥18.20/20.12
86+
// (CVE-2024-27980); without this the npm prefix lookup fails with EINVAL
87+
// and every npm-family Windows install classifies as `unsupported`.
88+
const options = {
89+
encoding: 'utf-8',
90+
shell: platform === 'win32' && command.toLowerCase().endsWith('.cmd'),
91+
windowsHide: true,
92+
} as const;
93+
execFile(command, [...args], options, (error, stdout) => {
8294
if (error) {
8395
reject(error);
8496
return;
@@ -140,14 +152,22 @@ export async function detectInstallSource(
140152
getPackageRoot: deps.getPackageRoot ?? getHostPackageRoot,
141153
getGlobalPrefix:
142154
deps.getGlobalPrefix ??
143-
(() => execFileText(npmCommand(platform), ['prefix', '-g']).then((text) => text.trim())),
155+
(() =>
156+
execFileText(npmCommand(platform), ['prefix', '-g'], platform).then((text) => text.trim())),
144157
detectNative: deps.detectNative ?? detectNativeInstall,
145158
platform,
146159
};
147160

148161
if (resolved.detectNative()) return 'native';
149162

150-
const packageRoot = resolved.getPackageRoot();
163+
// A layout with no reachable `package.json` cannot be classified, and this
164+
// runs on every launch — it reports "unsupported" rather than throwing.
165+
let packageRoot: string;
166+
try {
167+
packageRoot = resolved.getPackageRoot();
168+
} catch {
169+
return 'unsupported';
170+
}
151171
const heuristic = classifyByPathHeuristic(packageRoot);
152172
if (heuristic !== null) return heuristic;
153173

Lines changed: 156 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,156 @@
1+
/**
2+
* Post-install verification.
3+
*
4+
* An installer exit code of 0 only says the installer believed it finished.
5+
* It does not say the bytes that will run next launch are the target version:
6+
* a Windows report had `install.ps1` exit 0 repeatedly while the executable on
7+
* disk stayed on the old version, so the footer advertised
8+
* "restart to apply" forever and the recorded outcome was a lie.
9+
*
10+
* This module answers the only question that matters after an install — does
11+
* the thing that runs next report the version we installed? — and it answers
12+
* it from the same artifact the source updates:
13+
*
14+
* - native: the packaged binary at `process.execPath`, probed with
15+
* `--version` (Commander prints and exits before any preflight runs).
16+
* - npm/pnpm/yarn/bun: the host `package.json`, re-read from disk.
17+
* - homebrew: nothing — its update lands through the prepare-on-restart
18+
* lifecycle, not through this install path.
19+
*
20+
* It fails **open**: an unreadable package, a probe that times out or a
21+
* version string it cannot parse all report `ok`. A slow antivirus scan must
22+
* never turn a good install into a recorded failure. Only a version it read
23+
* successfully *and* that disagrees with the target is reported as a mismatch.
24+
*/
25+
26+
import { execFile } from 'node:child_process';
27+
import { readFile } from 'node:fs/promises';
28+
29+
import { valid } from 'semver';
30+
31+
import { findHostPackageJsonPath } from '#/cli/version';
32+
33+
import type { InstallSource } from './types';
34+
35+
/** Bound on the `--version` probe: a native binary starts in well under this. */
36+
const VERSION_PROBE_TIMEOUT_MS = 20_000;
37+
38+
export type InstallVerification =
39+
| { readonly ok: true }
40+
| { readonly ok: false; readonly reason: string };
41+
42+
export interface VerifyInstalledVersionDeps {
43+
/** Path of the packaged binary to probe (native sources only). */
44+
readonly execPath: string;
45+
/** Runs `<exe> --version` and resolves its stdout. */
46+
readonly probeExecutableVersion: (execPath: string) => Promise<string>;
47+
/** Reads the installed host `package.json`, or null when there is none. */
48+
readonly readPackageVersion: () => Promise<string | null>;
49+
}
50+
51+
const OK: InstallVerification = { ok: true };
52+
53+
/**
54+
* Extract the first `x.y.z` from a `--version` output. Commander prints the
55+
* bare version, but a wrapper is free to add a banner around it.
56+
*/
57+
export function parseVersionOutput(output: string): string | null {
58+
// No leading `\b`: a `v` prefix is a word character, so `v1.2.3` would not
59+
// match. A digit or dot before the first number still disqualifies it.
60+
const match = /(?<![\d.])\d+\.\d+\.\d+(?:-[0-9A-Za-z.-]+)?/u.exec(output);
61+
return match?.[0] ?? null;
62+
}
63+
64+
function sameVersion(found: string, expected: string): boolean {
65+
const normalize = (value: string): string => value.replace(/^v/u, '').trim();
66+
return normalize(found) === normalize(expected);
67+
}
68+
69+
async function defaultProbeExecutableVersion(execPath: string): Promise<string> {
70+
return new Promise<string>((resolve, reject) => {
71+
execFile(
72+
execPath,
73+
['--version'],
74+
{
75+
timeout: VERSION_PROBE_TIMEOUT_MS,
76+
windowsHide: true,
77+
encoding: 'utf-8',
78+
// The probe must not check for updates, install anything, or touch the
79+
// install state this verification is about to write.
80+
env: { ...process.env, PYTHINKER_CODE_NO_AUTO_UPDATE: '1' },
81+
},
82+
(error, stdout) => {
83+
if (error) {
84+
reject(error);
85+
return;
86+
}
87+
resolve(stdout);
88+
},
89+
);
90+
});
91+
}
92+
93+
async function defaultReadPackageVersion(): Promise<string | null> {
94+
const path = findHostPackageJsonPath();
95+
if (path === null) return null;
96+
const parsed = JSON.parse(await readFile(path, 'utf-8')) as { version?: unknown };
97+
return typeof parsed.version === 'string' ? parsed.version : null;
98+
}
99+
100+
/**
101+
* Verify that `expectedVersion` is what an install of `source` actually left
102+
* behind. See the module comment for the fail-open rule.
103+
*/
104+
export async function verifyInstalledVersion(
105+
source: InstallSource,
106+
expectedVersion: string,
107+
overrides: Partial<VerifyInstalledVersionDeps> = {},
108+
): Promise<InstallVerification> {
109+
if (valid(expectedVersion) === null) return OK;
110+
111+
const deps: VerifyInstalledVersionDeps = {
112+
execPath: overrides.execPath ?? process.execPath,
113+
probeExecutableVersion: overrides.probeExecutableVersion ?? defaultProbeExecutableVersion,
114+
readPackageVersion: overrides.readPackageVersion ?? defaultReadPackageVersion,
115+
};
116+
117+
switch (source) {
118+
case 'native': {
119+
let output: string;
120+
try {
121+
output = await deps.probeExecutableVersion(deps.execPath);
122+
} catch {
123+
return OK;
124+
}
125+
const found = parseVersionOutput(output);
126+
if (found === null || sameVersion(found, expectedVersion)) return OK;
127+
return {
128+
ok: false,
129+
reason:
130+
`the installer reported success but ${deps.execPath} still reports ` +
131+
`${found} (expected ${expectedVersion})`,
132+
};
133+
}
134+
case 'npm-global':
135+
case 'pnpm-global':
136+
case 'yarn-global':
137+
case 'bun-global': {
138+
let found: string | null;
139+
try {
140+
found = await deps.readPackageVersion();
141+
} catch {
142+
return OK;
143+
}
144+
if (found === null || sameVersion(found, expectedVersion)) return OK;
145+
return {
146+
ok: false,
147+
reason:
148+
`the installer reported success but the installed package is still ` +
149+
`${found} (expected ${expectedVersion})`,
150+
};
151+
}
152+
case 'homebrew':
153+
case 'unsupported':
154+
return OK;
155+
}
156+
}

0 commit comments

Comments
 (0)