Skip to content

Commit dbb7e60

Browse files
committed
fix(update): run Windows shims through the interpreter and verify natively
1 parent 60adaf0 commit dbb7e60

10 files changed

Lines changed: 240 additions & 200 deletions

File tree

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
---
2+
"@pythoughts/pythinker-code": patch
3+
---
4+
5+
Fix `pythinker doctor` crashing on native installs, and report the last recorded update outcome.
Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
---
2+
"@pythoughts/pythinker-code": patch
3+
---
4+
5+
Stop reporting an update as installed when the executable did not change; the version is checked after the installer finishes and a mismatch is recorded as a failure with the reason.
Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
---
2+
"@pythoughts/pythinker-code": patch
3+
---
4+
5+
Show download progress under the prompt while a Windows update installs, instead of nothing until it finishes.
Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
---
2+
"@pythoughts/pythinker-code": patch
3+
---
4+
5+
Fix automatic updates on Windows for npm, pnpm, and yarn installs, which failed to start at all.

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

Lines changed: 35 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -86,18 +86,32 @@ function bunCommand(platform: NodeJS.Platform): string {
8686
}
8787

8888
/**
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.
89+
* Node ≥18.20/20.12 refuses to spawn a `.cmd`/`.bat` file directly
90+
* (CVE-2024-27980) and fails with `EINVAL` — which is every npm-family update
91+
* on Windows: `npm.cmd`, `pnpm.cmd`, `yarn.cmd`. The command interpreter runs
92+
* them instead. It is spelled out as argv rather than `shell: true` so the
93+
* exact command line is visible here (and asserted in tests) instead of being
94+
* assembled by Node's string joining.
9495
*/
95-
export function needsShell(cmd: string, platform: NodeJS.Platform): boolean {
96+
function viaCommandInterpreter(command: SpawnCommand): SpawnCommand {
97+
return {
98+
...command,
99+
cmd: process.env['ComSpec'] ?? 'cmd.exe',
100+
args: ['/d', '/s', '/c', command.cmd, ...command.args],
101+
};
102+
}
103+
104+
/** True for the Windows package-manager shims that cannot be spawned directly. */
105+
export function isWindowsShim(cmd: string, platform: NodeJS.Platform): boolean {
96106
if (platform !== 'win32') return false;
97107
const lower = cmd.toLowerCase();
98108
return lower.endsWith('.cmd') || lower.endsWith('.bat');
99109
}
100110

111+
function spawnable(command: SpawnCommand, platform: NodeJS.Platform): SpawnCommand {
112+
return isWindowsShim(command.cmd, platform) ? viaCommandInterpreter(command) : command;
113+
}
114+
101115
export function installCommandFor(
102116
source: InstallSource,
103117
version: string,
@@ -162,11 +176,20 @@ export function spawnForSource(
162176
): SpawnCommand {
163177
switch (source) {
164178
case 'npm-global':
165-
return { cmd: withCmdSuffix('npm', platform), args: ['install', '-g', `${NPM_PACKAGE_NAME}@${version}`] };
179+
return spawnable(
180+
{ cmd: withCmdSuffix('npm', platform), args: ['install', '-g', `${NPM_PACKAGE_NAME}@${version}`] },
181+
platform,
182+
);
166183
case 'pnpm-global':
167-
return { cmd: withCmdSuffix('pnpm', platform), args: ['add', '-g', `${NPM_PACKAGE_NAME}@${version}`] };
184+
return spawnable(
185+
{ cmd: withCmdSuffix('pnpm', platform), args: ['add', '-g', `${NPM_PACKAGE_NAME}@${version}`] },
186+
platform,
187+
);
168188
case 'yarn-global':
169-
return { cmd: withCmdSuffix('yarn', platform), args: ['global', 'add', `${NPM_PACKAGE_NAME}@${version}`] };
189+
return spawnable(
190+
{ cmd: withCmdSuffix('yarn', platform), args: ['global', 'add', `${NPM_PACKAGE_NAME}@${version}`] },
191+
platform,
192+
);
170193
case 'bun-global':
171194
return { cmd: bunCommand(platform), args: ['add', '-g', `${NPM_PACKAGE_NAME}@${version}`] };
172195
case 'homebrew':
@@ -565,7 +588,6 @@ export async function installUpdate(
565588
await new Promise<void>((resolve, reject) => {
566589
const child = spawn(cmd, [...args], {
567590
stdio: 'inherit',
568-
shell: needsShell(cmd, platform),
569591
env: env === undefined ? undefined : { ...process.env, ...env },
570592
});
571593
child.once('error', reject);
@@ -928,6 +950,9 @@ async function startBackgroundInstall(
928950
logUpdateInfo(logger, 'background update install succeeded', {
929951
targetVersion: target.version,
930952
source,
953+
// Present when the install was recorded without proof, so a report
954+
// of "it says updated but it did not" is answerable from the log.
955+
unverified: verification.ok ? verification.unverified : undefined,
931956
});
932957
return;
933958
}
@@ -952,7 +977,6 @@ async function startBackgroundInstall(
952977
// A detached child gets its own console window on Windows regardless
953978
// of stdio; stdio: 'ignore' alone does not suppress it.
954979
windowsHide: platform === 'win32',
955-
shell: needsShell(cmd, platform),
956980
// stdout stays discarded (install progress is noise); stderr is piped so
957981
// the installer's machine-readable progress lines can be recorded and a
958982
// failure still keeps the installer's own error text.

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

Lines changed: 18 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -81,22 +81,25 @@ function execFileText(
8181
args: readonly string[],
8282
platform: NodeJS.Platform = process.platform,
8383
): Promise<string> {
84+
// `npm.cmd` cannot be spawned directly on Node ≥18.20/20.12
85+
// (CVE-2024-27980): it fails with EINVAL, and every npm-family Windows
86+
// install then classifies as `unsupported` and never auto-updates.
87+
const viaInterpreter = platform === 'win32' && command.toLowerCase().endsWith('.cmd');
88+
const spawnCommand = viaInterpreter ? process.env['ComSpec'] ?? 'cmd.exe' : command;
89+
const spawnArgs = viaInterpreter ? ['/d', '/s', '/c', command, ...args] : [...args];
8490
return new Promise((resolveOutput, reject) => {
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) => {
94-
if (error) {
95-
reject(error);
96-
return;
97-
}
98-
resolveOutput(stdout);
99-
});
91+
execFile(
92+
spawnCommand,
93+
spawnArgs,
94+
{ encoding: 'utf-8', windowsHide: true },
95+
(error, stdout) => {
96+
if (error) {
97+
reject(error);
98+
return;
99+
}
100+
resolveOutput(stdout);
101+
},
102+
);
100103
});
101104
}
102105

apps/pythinker-code/src/cli/update/verify-install.ts

Lines changed: 38 additions & 73 deletions
Original file line numberDiff line numberDiff line change
@@ -7,48 +7,43 @@
77
* disk stayed on the old version, so the footer advertised
88
* "restart to apply" forever and the recorded outcome was a lie.
99
*
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:
10+
* Only a `native` install is verified, and only against the artifact the
11+
* installer replaces — the packaged binary at `process.execPath`, probed with
12+
* `--version` (Commander prints and exits before any preflight runs). The
13+
* npm family is deliberately left unverified: a global reinstall rewrites the
14+
* very directory this process was loaded from, so a read there proves nothing
15+
* about the next launch and a wrong answer would park a healthy version.
1316
*
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.
17+
* It fails **open**: a probe that times out, cannot run, or prints no version
18+
* reports `ok` with an `unverified` note for the caller to log. A slow
19+
* antivirus scan must never turn a good install into a recorded failure. Only
20+
* a version read successfully *and* disagreeing with the target is a mismatch.
2421
*/
2522

2623
import { execFile } from 'node:child_process';
27-
import { readFile } from 'node:fs/promises';
28-
2924
import { valid } from 'semver';
3025

31-
import { findHostPackageJsonPath } from '#/cli/version';
32-
26+
import { formatErrorMessage } from './format-error';
3327
import type { InstallSource } from './types';
3428

3529
/** Bound on the `--version` probe: a native binary starts in well under this. */
3630
const VERSION_PROBE_TIMEOUT_MS = 20_000;
3731

3832
export type InstallVerification =
39-
| { readonly ok: true }
33+
/** Installed as expected, or not checkable — `unverified` says which. */
34+
| { readonly ok: true; readonly unverified?: string }
4035
| { readonly ok: false; readonly reason: string };
4136

4237
export interface VerifyInstalledVersionDeps {
43-
/** Path of the packaged binary to probe (native sources only). */
38+
/** Path of the packaged binary to probe (native installs only). */
4439
readonly execPath: string;
4540
/** Runs `<exe> --version` and resolves its stdout. */
4641
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>;
4942
}
5043

51-
const OK: InstallVerification = { ok: true };
44+
function unverified(note: string): InstallVerification {
45+
return { ok: true, unverified: note };
46+
}
5247

5348
/**
5449
* Extract the first `x.y.z` from a `--version` output. Commander prints the
@@ -90,13 +85,6 @@ async function defaultProbeExecutableVersion(execPath: string): Promise<string>
9085
});
9186
}
9287

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-
10088
/**
10189
* Verify that `expectedVersion` is what an install of `source` actually left
10290
* behind. See the module comment for the fail-open rule.
@@ -106,51 +94,28 @@ export async function verifyInstalledVersion(
10694
expectedVersion: string,
10795
overrides: Partial<VerifyInstalledVersionDeps> = {},
10896
): Promise<InstallVerification> {
109-
if (valid(expectedVersion) === null) return OK;
97+
if (source !== 'native') return unverified(`not verified for ${source} installs`);
98+
if (valid(expectedVersion) === null) {
99+
return unverified(`not a version to verify against: ${expectedVersion}`);
100+
}
110101

111-
const deps: VerifyInstalledVersionDeps = {
112-
execPath: overrides.execPath ?? process.execPath,
113-
probeExecutableVersion: overrides.probeExecutableVersion ?? defaultProbeExecutableVersion,
114-
readPackageVersion: overrides.readPackageVersion ?? defaultReadPackageVersion,
115-
};
102+
const execPath = overrides.execPath ?? process.execPath;
103+
const probe = overrides.probeExecutableVersion ?? defaultProbeExecutableVersion;
116104

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;
105+
let output: string;
106+
try {
107+
output = await probe(execPath);
108+
} catch (error) {
109+
return unverified(`${execPath} could not be run: ${formatErrorMessage(error)}`);
155110
}
111+
112+
const found = parseVersionOutput(output);
113+
if (found === null) return unverified(`${execPath} printed no version`);
114+
if (sameVersion(found, expectedVersion)) return { ok: true };
115+
return {
116+
ok: false,
117+
reason:
118+
`the installer reported success but ${execPath} still reports ` +
119+
`${found} (expected ${expectedVersion})`,
120+
};
156121
}

0 commit comments

Comments
 (0)