From 60adaf0ee37469c3c250088462d9db14761e5dfe Mon Sep 17 00:00:00 2001 From: elkaix Date: Sat, 8 Aug 2026 18:34:00 -0400 Subject: [PATCH 1/5] fix(update): verify installs and stop crashing doctor on native builds --- apps/pythinker-code/src/cli/sub/doctor.ts | 18 +- .../src/cli/update/preflight.ts | 38 ++++- apps/pythinker-code/src/cli/update/source.ts | 28 +++- .../src/cli/update/verify-install.ts | 156 ++++++++++++++++++ apps/pythinker-code/src/cli/version.ts | 26 ++- apps/pythinker-code/test/cli/doctor.test.ts | 51 ++++++ .../test/cli/update/preflight.test.ts | 139 +++++++++++++--- .../test/cli/update/source.test.ts | 16 ++ .../test/cli/update/verify-install.test.ts | 140 ++++++++++++++++ apps/pythinker-web/public/install.ps1 | 33 ++++ 10 files changed, 611 insertions(+), 34 deletions(-) create mode 100644 apps/pythinker-code/src/cli/update/verify-install.ts create mode 100644 apps/pythinker-code/test/cli/update/verify-install.test.ts diff --git a/apps/pythinker-code/src/cli/sub/doctor.ts b/apps/pythinker-code/src/cli/sub/doctor.ts index cc691e07..4dc51afd 100644 --- a/apps/pythinker-code/src/cli/sub/doctor.ts +++ b/apps/pythinker-code/src/cli/sub/doctor.ts @@ -24,7 +24,7 @@ import { } from '#/cli/update/preflight'; import { detectInstallSource } from '#/cli/update/source'; import type { UpdateInstallFailure } from '#/cli/update/types'; -import { getHostPackageRoot, getVersion } from '#/cli/version'; +import { findHostPackageRoot, getVersion } from '#/cli/version'; import { getUpdateInstallLogFile } from '#/utils/paths'; interface WritableLike { @@ -50,7 +50,8 @@ export interface DoctorDeps { export interface DoctorRuntimeInfo { readonly version: string; readonly installSource: string; - readonly packageRoot: string; + /** Absent on a native binary: a packaged install has no `package.json`. */ + readonly packageRoot?: string; readonly executable: string; readonly installations?: readonly string[]; readonly ripgrep?: RgResolution; @@ -62,6 +63,7 @@ export interface DoctorRuntimeInfo { readonly pendingVersion?: string; readonly pendingRequestedBy?: 'automatic' | 'manual'; readonly activeOperation?: string; + readonly lastSuccess?: string; readonly lastFailure?: string; readonly logPath?: string; }; @@ -191,7 +193,7 @@ function resolveDeps(deps: Partial | DoctorDeps | undefined): Resolv return { version: getVersion(), installSource, - packageRoot: getHostPackageRoot(), + packageRoot: findHostPackageRoot() ?? undefined, executable: process.execPath, installations, ripgrep, @@ -206,6 +208,11 @@ function resolveDeps(deps: Partial | DoctorDeps | undefined): Resolv installState.active === null ? undefined : `${installState.active.operation ?? 'install'} ${installState.active.version}`, + lastSuccess: + installState.lastSuccess === null + ? undefined + : `${installState.lastSuccess.version} (installed ` + + `${installState.lastSuccess.installedAt})`, lastFailure: installState.lastFailure === null ? undefined @@ -387,7 +394,7 @@ function formatRuntimeInfo(info: DoctorRuntimeInfo | undefined): string[] { 'Runtime', ` Version: ${info.version}`, ` Install source: ${info.installSource}`, - ` Package root: ${info.packageRoot}`, + ...(info.packageRoot === undefined ? [] : [` Package root: ${info.packageRoot}`]), ` Executable: ${info.executable}`, ...(installations.length > 1 ? [ @@ -414,6 +421,9 @@ function formatRuntimeInfo(info: DoctorRuntimeInfo | undefined): string[] { ...(info.update.activeOperation === undefined ? [] : [` Update operation: ${info.update.activeOperation}`]), + ...(info.update.lastSuccess === undefined + ? [] + : [` Last update success: ${info.update.lastSuccess}`]), ...(info.update.lastFailure === undefined ? [] : [` Last update failure: ${info.update.lastFailure}`]), diff --git a/apps/pythinker-code/src/cli/update/preflight.ts b/apps/pythinker-code/src/cli/update/preflight.ts index c3c68d81..57a8ef6c 100644 --- a/apps/pythinker-code/src/cli/update/preflight.ts +++ b/apps/pythinker-code/src/cli/update/preflight.ts @@ -56,9 +56,13 @@ import { type UpdateRequestOrigin, type UpdateTarget, } from './types'; +import { verifyInstalledVersion, type InstallVerification } from './verify-install'; export type { UpdatePreflightResult } from './types'; +/** Reused for the paths that never reach verification (a failed install). */ +const OK_VERIFICATION: InstallVerification = { ok: true }; + export interface RunUpdatePreflightOptions { readonly stdout?: { write(chunk: string): boolean }; readonly stderr?: { write(chunk: string): boolean }; @@ -81,6 +85,19 @@ function bunCommand(platform: NodeJS.Platform): string { return platform === 'win32' ? 'bun.exe' : 'bun'; } +/** + * Node ≥18.20/20.12 refuses to spawn a `.cmd`/`.bat` file without a shell + * (CVE-2024-27980) and fails with `EINVAL`, which is every npm-family update + * on Windows: `npm.cmd`, `pnpm.cmd`, `yarn.cmd`. Only the package manager + * wrappers need it — the arguments are a fixed flag list plus + * `@`, so nothing here reaches the shell as data. + */ +export function needsShell(cmd: string, platform: NodeJS.Platform): boolean { + if (platform !== 'win32') return false; + const lower = cmd.toLowerCase(); + return lower.endsWith('.cmd') || lower.endsWith('.bat'); +} + export function installCommandFor( source: InstallSource, version: string, @@ -548,6 +565,7 @@ export async function installUpdate( await new Promise((resolve, reject) => { const child = spawn(cmd, [...args], { stdio: 'inherit', + shell: needsShell(cmd, platform), env: env === undefined ? undefined : { ...process.env, ...env }, }); child.once('error', reject); @@ -560,6 +578,11 @@ export async function installUpdate( reject(new Error(`${cmd} exited with ${detail}`)); }); }); + // Exit code 0 is the installer's opinion; this is the fact. Rejecting here + // routes a silent no-op install into the same failure reporting a crashed + // installer gets, instead of printing "Updated …" over an unchanged binary. + const verification = await verifyInstalledVersion(source, version); + if (!verification.ok) throw new Error(verification.reason); } /** Keep the tail only: installers can be chatty, and the state file is small. */ @@ -861,11 +884,19 @@ async function startBackgroundInstall( // `settled` already stops new progress writes; drain the ones in flight so // none of them renames over the outcome below. await progressWrites; + // An installer that exits 0 without replacing the binary must not be + // recorded as a success: the footer would advertise "restart to apply" + // for a version that never runs, on every launch, forever. + const verification = succeeded + ? await verifyInstalledVersion(source, target.version) + : OK_VERIFICATION; + const installed = succeeded && verification.ok; + const outcomeReason = verification.ok ? reason : verification.reason; const attempts = failureAttemptsFor(startedState, target, 'install') + 1; const stderrTail = readStderrTail(); - const message = stderrTail === undefined ? reason : `${reason}: ${stderrTail}`; + const message = stderrTail === undefined ? outcomeReason : `${outcomeReason}: ${stderrTail}`; - const nextState: UpdateInstallState = succeeded + const nextState: UpdateInstallState = installed ? { ...startedState, active: null, @@ -889,7 +920,7 @@ async function startBackgroundInstall( }; try { await writeUpdateInstallState(nextState).catch(() => {}); - if (succeeded) { + if (installed) { trackUpdateEvent(track, 'update_background_install_succeeded', { target_version: target.version, source, @@ -921,6 +952,7 @@ async function startBackgroundInstall( // A detached child gets its own console window on Windows regardless // of stdio; stdio: 'ignore' alone does not suppress it. windowsHide: platform === 'win32', + shell: needsShell(cmd, platform), // stdout stays discarded (install progress is noise); stderr is piped so // the installer's machine-readable progress lines can be recorded and a // failure still keeps the installer's own error text. diff --git a/apps/pythinker-code/src/cli/update/source.ts b/apps/pythinker-code/src/cli/update/source.ts index 7d6904b6..e77794e5 100644 --- a/apps/pythinker-code/src/cli/update/source.ts +++ b/apps/pythinker-code/src/cli/update/source.ts @@ -76,9 +76,21 @@ function npmCommand(platform: NodeJS.Platform): string { return platform === 'win32' ? 'npm.cmd' : 'npm'; } -function execFileText(command: string, args: readonly string[]): Promise { +function execFileText( + command: string, + args: readonly string[], + platform: NodeJS.Platform = process.platform, +): Promise { return new Promise((resolveOutput, reject) => { - execFile(command, [...args], { encoding: 'utf-8' }, (error, stdout) => { + // `npm.cmd` cannot be spawned without a shell on Node ≥18.20/20.12 + // (CVE-2024-27980); without this the npm prefix lookup fails with EINVAL + // and every npm-family Windows install classifies as `unsupported`. + const options = { + encoding: 'utf-8', + shell: platform === 'win32' && command.toLowerCase().endsWith('.cmd'), + windowsHide: true, + } as const; + execFile(command, [...args], options, (error, stdout) => { if (error) { reject(error); return; @@ -140,14 +152,22 @@ export async function detectInstallSource( getPackageRoot: deps.getPackageRoot ?? getHostPackageRoot, getGlobalPrefix: deps.getGlobalPrefix ?? - (() => execFileText(npmCommand(platform), ['prefix', '-g']).then((text) => text.trim())), + (() => + execFileText(npmCommand(platform), ['prefix', '-g'], platform).then((text) => text.trim())), detectNative: deps.detectNative ?? detectNativeInstall, platform, }; if (resolved.detectNative()) return 'native'; - const packageRoot = resolved.getPackageRoot(); + // A layout with no reachable `package.json` cannot be classified, and this + // runs on every launch — it reports "unsupported" rather than throwing. + let packageRoot: string; + try { + packageRoot = resolved.getPackageRoot(); + } catch { + return 'unsupported'; + } const heuristic = classifyByPathHeuristic(packageRoot); if (heuristic !== null) return heuristic; diff --git a/apps/pythinker-code/src/cli/update/verify-install.ts b/apps/pythinker-code/src/cli/update/verify-install.ts new file mode 100644 index 00000000..d2084b8f --- /dev/null +++ b/apps/pythinker-code/src/cli/update/verify-install.ts @@ -0,0 +1,156 @@ +/** + * Post-install verification. + * + * An installer exit code of 0 only says the installer believed it finished. + * It does not say the bytes that will run next launch are the target version: + * a Windows report had `install.ps1` exit 0 repeatedly while the executable on + * disk stayed on the old version, so the footer advertised + * "restart to apply" forever and the recorded outcome was a lie. + * + * This module answers the only question that matters after an install — does + * the thing that runs next report the version we installed? — and it answers + * it from the same artifact the source updates: + * + * - native: the packaged binary at `process.execPath`, probed with + * `--version` (Commander prints and exits before any preflight runs). + * - npm/pnpm/yarn/bun: the host `package.json`, re-read from disk. + * - homebrew: nothing — its update lands through the prepare-on-restart + * lifecycle, not through this install path. + * + * It fails **open**: an unreadable package, a probe that times out or a + * version string it cannot parse all report `ok`. A slow antivirus scan must + * never turn a good install into a recorded failure. Only a version it read + * successfully *and* that disagrees with the target is reported as a mismatch. + */ + +import { execFile } from 'node:child_process'; +import { readFile } from 'node:fs/promises'; + +import { valid } from 'semver'; + +import { findHostPackageJsonPath } from '#/cli/version'; + +import type { InstallSource } from './types'; + +/** Bound on the `--version` probe: a native binary starts in well under this. */ +const VERSION_PROBE_TIMEOUT_MS = 20_000; + +export type InstallVerification = + | { readonly ok: true } + | { readonly ok: false; readonly reason: string }; + +export interface VerifyInstalledVersionDeps { + /** Path of the packaged binary to probe (native sources only). */ + readonly execPath: string; + /** Runs ` --version` and resolves its stdout. */ + readonly probeExecutableVersion: (execPath: string) => Promise; + /** Reads the installed host `package.json`, or null when there is none. */ + readonly readPackageVersion: () => Promise; +} + +const OK: InstallVerification = { ok: true }; + +/** + * Extract the first `x.y.z` from a `--version` output. Commander prints the + * bare version, but a wrapper is free to add a banner around it. + */ +export function parseVersionOutput(output: string): string | null { + // No leading `\b`: a `v` prefix is a word character, so `v1.2.3` would not + // match. A digit or dot before the first number still disqualifies it. + const match = /(? value.replace(/^v/u, '').trim(); + return normalize(found) === normalize(expected); +} + +async function defaultProbeExecutableVersion(execPath: string): Promise { + return new Promise((resolve, reject) => { + execFile( + execPath, + ['--version'], + { + timeout: VERSION_PROBE_TIMEOUT_MS, + windowsHide: true, + encoding: 'utf-8', + // The probe must not check for updates, install anything, or touch the + // install state this verification is about to write. + env: { ...process.env, PYTHINKER_CODE_NO_AUTO_UPDATE: '1' }, + }, + (error, stdout) => { + if (error) { + reject(error); + return; + } + resolve(stdout); + }, + ); + }); +} + +async function defaultReadPackageVersion(): Promise { + const path = findHostPackageJsonPath(); + if (path === null) return null; + const parsed = JSON.parse(await readFile(path, 'utf-8')) as { version?: unknown }; + return typeof parsed.version === 'string' ? parsed.version : null; +} + +/** + * Verify that `expectedVersion` is what an install of `source` actually left + * behind. See the module comment for the fail-open rule. + */ +export async function verifyInstalledVersion( + source: InstallSource, + expectedVersion: string, + overrides: Partial = {}, +): Promise { + if (valid(expectedVersion) === null) return OK; + + const deps: VerifyInstalledVersionDeps = { + execPath: overrides.execPath ?? process.execPath, + probeExecutableVersion: overrides.probeExecutableVersion ?? defaultProbeExecutableVersion, + readPackageVersion: overrides.readPackageVersion ?? defaultReadPackageVersion, + }; + + switch (source) { + case 'native': { + let output: string; + try { + output = await deps.probeExecutableVersion(deps.execPath); + } catch { + return OK; + } + const found = parseVersionOutput(output); + if (found === null || sameVersion(found, expectedVersion)) return OK; + return { + ok: false, + reason: + `the installer reported success but ${deps.execPath} still reports ` + + `${found} (expected ${expectedVersion})`, + }; + } + case 'npm-global': + case 'pnpm-global': + case 'yarn-global': + case 'bun-global': { + let found: string | null; + try { + found = await deps.readPackageVersion(); + } catch { + return OK; + } + if (found === null || sameVersion(found, expectedVersion)) return OK; + return { + ok: false, + reason: + `the installer reported success but the installed package is still ` + + `${found} (expected ${expectedVersion})`, + }; + } + case 'homebrew': + case 'unsupported': + return OK; + } +} diff --git a/apps/pythinker-code/src/cli/version.ts b/apps/pythinker-code/src/cli/version.ts index 7a3d4b5d..873d942d 100644 --- a/apps/pythinker-code/src/cli/version.ts +++ b/apps/pythinker-code/src/cli/version.ts @@ -16,7 +16,15 @@ import { PYTHINKER_BUILD_INFO } from './build-info'; const MODULE_DIR = import.meta.dirname; -export function getHostPackageJsonPath(): string { +/** + * Locate the host `package.json`, or `null` when there is none. + * + * A packaged native binary (SEA) ships no `package.json` at all, so every + * caller that only wants the path for diagnostics must take the `null` branch + * instead of crashing the command — `pythinker doctor` did exactly that on + * native installs. + */ +export function findHostPackageJsonPath(): string | null { // Walk upwards from this file's directory until a `package.json` shows up, // so both dev (`tsx src/main.ts` — this file in `src/cli/`, pkg 2 levels // up) and prod (`node dist/main.mjs` — this code bundled into `dist/`, @@ -31,13 +39,27 @@ export function getHostPackageJsonPath(): string { if (parent === dir) break; dir = parent; } - throw new Error(`Could not locate package.json near ${MODULE_DIR}`); + return null; +} + +export function getHostPackageJsonPath(): string { + const found = findHostPackageJsonPath(); + if (found === null) { + throw new Error(`Could not locate package.json near ${MODULE_DIR}`); + } + return found; } export function getHostPackageRoot(): string { return dirname(getHostPackageJsonPath()); } +/** The host package root, or `null` on a native binary that has no package. */ +export function findHostPackageRoot(): string | null { + const found = findHostPackageJsonPath(); + return found === null ? null : dirname(found); +} + export function getVersion(): string { if (PYTHINKER_BUILD_INFO.version !== undefined) { return PYTHINKER_BUILD_INFO.version; diff --git a/apps/pythinker-code/test/cli/doctor.test.ts b/apps/pythinker-code/test/cli/doctor.test.ts index b0250369..20480349 100644 --- a/apps/pythinker-code/test/cli/doctor.test.ts +++ b/apps/pythinker-code/test/cli/doctor.test.ts @@ -194,6 +194,57 @@ describe('pythinker doctor', () => { ); }); + it('reports the recorded update outcomes', async () => { + const { deps, stdout } = makeDeps(); + + const code = await handleDoctor( + { + ...deps, + runtimeInfo: async () => ({ + version: '0.12.0', + installSource: 'native', + executable: '/usr/local/bin/pythinker', + update: { + latest: '0.13.1', + checkedAt: '2026-08-08T12:00:00.000Z', + lastSuccess: '0.13.1 (installed 2026-08-08T12:01:00.000Z)', + lastFailure: 'install 0.13.1 (attempt 1): still reports 0.12.0', + }, + }), + }, + {}, + ); + + expect(code).toBe(0); + const output = stdout.join(''); + expect(output).toContain(' Last update success: 0.13.1 (installed 2026-08-08T12:01:00.000Z)'); + expect(output).toContain(' Last update failure: install 0.13.1 (attempt 1): still reports 0.12.0'); + }); + + // A packaged native binary ships no package.json. Reporting it used to + // crash the whole command with "Could not locate package.json near …". + it('reports a native install that has no package root', async () => { + const { deps, stdout } = makeDeps(); + + const code = await handleDoctor( + { + ...deps, + runtimeInfo: async () => ({ + version: '1.2.3', + installSource: 'native', + executable: 'C:\\Programs\\Pythinker\\pythinker.exe', + }), + }, + {}, + ); + + expect(code).toBe(0); + const output = stdout.join(''); + expect(output).toContain(' Install source: native'); + expect(output).toContain(' Executable: C:\\Programs\\Pythinker\\pythinker.exe'); + expect(output).not.toContain('Package root'); + }); + it('warns when multiple Pythinker executables are installed', async () => { const { deps, stdout } = makeDeps(); diff --git a/apps/pythinker-code/test/cli/update/preflight.test.ts b/apps/pythinker-code/test/cli/update/preflight.test.ts index 014fd4fc..2b4c9b6f 100644 --- a/apps/pythinker-code/test/cli/update/preflight.test.ts +++ b/apps/pythinker-code/test/cli/update/preflight.test.ts @@ -10,7 +10,13 @@ import { readUpdateInstallState, writeUpdateInstallState, } from '#/cli/update/install-state'; -import { canAutoInstall, runUpdatePreflight, spawnForSource, startManualUpdate } from '#/cli/update/preflight'; +import { + canAutoInstall, + needsShell, + runUpdatePreflight, + spawnForSource, + startManualUpdate, +} from '#/cli/update/preflight'; import { promptForInstallChoice } from '#/cli/update/prompt'; import type * as PromptModule from '#/cli/update/prompt'; import { refreshUpdateCache } from '#/cli/update/refresh'; @@ -44,6 +50,7 @@ const mocks = vi.hoisted(() => ({ readJsonFile: vi.fn(), writeJsonFile: vi.fn(), spawn: vi.fn(), + verifyInstalledVersion: vi.fn(), })); vi.mock('../../../src/cli/update/cache', () => ({ @@ -124,6 +131,19 @@ vi.mock('../../../src/cli/update/rollout', async () => { }; }); +// Post-install verification runs a real probe (a `--version` spawn, or a +// package.json read) — stubbed here so these tests exercise the reporting, +// with its own suite in verify-install.test.ts. +vi.mock('../../../src/cli/update/verify-install', async () => { + const actual = await vi.importActual< + typeof import('../../../src/cli/update/verify-install.js') + >('../../../src/cli/update/verify-install.js'); + return { + ...actual, + verifyInstalledVersion: mocks.verifyInstalledVersion, + }; +}); + vi.mock('node:child_process', async () => { const actual = await vi.importActual('node:child_process'); return { @@ -411,6 +431,7 @@ describe('runUpdatePreflight', () => { mocks.loadTuiConfig.mockResolvedValue(tuiConfig()); mocks.resolveUpdateDeviceId.mockReturnValue('test-device'); mocks.appendRolloutDecisionLog.mockResolvedValue(undefined); + mocks.verifyInstalledVersion.mockResolvedValue({ ok: true }); mocks.tryAcquireUpdateInstallLock.mockResolvedValue({ filePath: '/tmp/pythinker-update-install.lock', release: vi.fn().mockResolvedValue(undefined), @@ -462,7 +483,7 @@ describe('runUpdatePreflight', () => { expect(mocks.spawn).toHaveBeenCalledWith( expect.stringMatching(/^npm(\.cmd)?$/), ['install', '-g', '@pythoughts/pythinker-code@0.5.0'], - { detached: true, windowsHide: false, stdio: ['ignore', 'ignore', 'pipe'] }, + { detached: true, windowsHide: false, shell: false, stdio: ['ignore', 'ignore', 'pipe'] }, ); }); @@ -527,7 +548,7 @@ describe('runUpdatePreflight', () => { expect(mocks.spawn).toHaveBeenCalledWith( expect.stringMatching(/^npm(\.cmd)?$/), ['install', '-g', '@pythoughts/pythinker-code@0.5.0'], - { stdio: 'inherit' }, + { stdio: 'inherit', shell: false }, ); expect(stdout.join('')).toContain('Updated @pythoughts/pythinker-code to 0.5.0'); }); @@ -553,7 +574,7 @@ describe('runUpdatePreflight', () => { expect(mocks.spawn).toHaveBeenCalledWith( expect.stringMatching(/^npm(\.cmd)?$/), ['install', '-g', '@pythoughts/pythinker-code@0.7.0'], - { stdio: 'inherit' }, + { stdio: 'inherit', shell: false }, ); expect(stdout.join('')).toContain('Updated @pythoughts/pythinker-code to 0.7.0'); }); @@ -598,7 +619,7 @@ describe('runUpdatePreflight', () => { expect(mocks.spawn).toHaveBeenCalledWith( expect.stringMatching(/^npm(\.cmd)?$/u), ['install', '-g', '@pythoughts/pythinker-code@0.11.0'], - { detached: true, windowsHide: false, stdio: ['ignore', 'ignore', 'pipe'] }, + { detached: true, windowsHide: false, shell: false, stdio: ['ignore', 'ignore', 'pipe'] }, ); expect(mocks.spawn).not.toHaveBeenCalledWith( expect.stringMatching(/^npm(\.cmd)?$/u), @@ -635,7 +656,7 @@ describe('runUpdatePreflight', () => { expect(mocks.spawn).toHaveBeenCalledWith( expect.stringMatching(/^npm(\.cmd)?$/u), ['install', '-g', '@pythoughts/pythinker-code@0.10.0'], - { detached: true, windowsHide: false, stdio: ['ignore', 'ignore', 'pipe'] }, + { detached: true, windowsHide: false, shell: false, stdio: ['ignore', 'ignore', 'pipe'] }, ); }); @@ -657,7 +678,7 @@ describe('runUpdatePreflight', () => { expect(mocks.spawn).toHaveBeenCalledWith( expect.stringMatching(/^npm(\.cmd)?$/u), ['install', '-g', '@pythoughts/pythinker-code@0.10.0'], - { detached: true, windowsHide: false, stdio: ['ignore', 'ignore', 'pipe'] }, + { detached: true, windowsHide: false, shell: false, stdio: ['ignore', 'ignore', 'pipe'] }, ); } finally { vi.useRealTimers(); @@ -708,7 +729,7 @@ describe('runUpdatePreflight', () => { expect(mocks.spawn).toHaveBeenCalledWith( expect.stringMatching(/^pnpm(\.cmd)?$/), ['add', '-g', '@pythoughts/pythinker-code@0.5.0'], - { stdio: 'inherit' }, + { stdio: 'inherit', shell: false }, ); }); @@ -724,7 +745,7 @@ describe('runUpdatePreflight', () => { expect(mocks.spawn).toHaveBeenCalledWith( expect.stringMatching(/^yarn(\.cmd)?$/), ['global', 'add', '@pythoughts/pythinker-code@0.5.0'], - { stdio: 'inherit' }, + { stdio: 'inherit', shell: false }, ); }); @@ -740,7 +761,7 @@ describe('runUpdatePreflight', () => { expect(mocks.spawn).toHaveBeenCalledWith( expect.stringMatching(/^bun(\.exe)?$/), ['add', '-g', '@pythoughts/pythinker-code@0.5.0'], - { stdio: 'inherit' }, + { stdio: 'inherit', shell: false }, ); }); @@ -802,7 +823,7 @@ describe('runUpdatePreflight', () => { await runUpdatePreflight('0.4.0', options); const call = mocks.spawn.mock.calls[0]; expect(call?.[0]).toBe('bash'); - expect(call?.[2]).toEqual({ stdio: 'inherit' }); + expect(call?.[2]).toEqual({ stdio: 'inherit', shell: false }); const [flag, script] = call?.[1] as string[]; expect(flag).toBe('-c'); // pipefail must come before the pipeline so a failed `curl` is not masked @@ -843,6 +864,8 @@ describe('runUpdatePreflight', () => { { detached: true, windowsHide: true, + // powershell.exe is a real executable: no shell wrapper needed. + shell: false, stdio: ['ignore', 'ignore', 'pipe'], env: expect.objectContaining({ PYTHINKER_VERSION: '0.5.0' }), }, @@ -903,7 +926,7 @@ describe('runUpdatePreflight', () => { expect(mocks.spawn).toHaveBeenCalledWith( expect.stringMatching(/^npm(\.cmd)?$/u), ['install', '-g', '@pythoughts/pythinker-code@0.5.0'], - { stdio: 'inherit' }, + { stdio: 'inherit', shell: false }, ); }); @@ -1051,7 +1074,7 @@ describe('runUpdatePreflight', () => { expect(mocks.spawn).toHaveBeenCalledWith( expect.stringMatching(/^npm(\.cmd)?$/), ['install', '-g', '@pythoughts/pythinker-code@0.5.0'], - { detached: true, windowsHide: false, stdio: ['ignore', 'ignore', 'pipe'] }, + { detached: true, windowsHide: false, shell: false, stdio: ['ignore', 'ignore', 'pipe'] }, ); expect(writeUpdateInstallState).toHaveBeenCalledWith(expect.objectContaining({ active: expect.objectContaining({ @@ -1195,7 +1218,7 @@ describe('runUpdatePreflight', () => { expect(mocks.spawn).toHaveBeenCalledWith( expect.stringMatching(/^npm(\.cmd)?$/u), ['install', '-g', '@pythoughts/pythinker-code@0.6.0'], - { detached: true, windowsHide: false, stdio: ['ignore', 'ignore', 'pipe'] }, + { detached: true, windowsHide: false, shell: false, stdio: ['ignore', 'ignore', 'pipe'] }, ); }); @@ -1258,7 +1281,7 @@ describe('runUpdatePreflight', () => { expect(mocks.spawn).toHaveBeenCalledWith( expect.stringMatching(/^npm(\.cmd)?$/), ['install', '-g', '@pythoughts/pythinker-code@0.6.0'], - { detached: true, windowsHide: false, stdio: ['ignore', 'ignore', 'pipe'] }, + { detached: true, windowsHide: false, shell: false, stdio: ['ignore', 'ignore', 'pipe'] }, ); }); @@ -1282,7 +1305,7 @@ describe('runUpdatePreflight', () => { expect(mocks.spawn).toHaveBeenCalledWith( expect.stringMatching(/^npm(\.cmd)?$/), ['install', '-g', '@pythoughts/pythinker-code@0.6.0'], - { detached: true, windowsHide: false, stdio: ['ignore', 'ignore', 'pipe'] }, + { detached: true, windowsHide: false, shell: false, stdio: ['ignore', 'ignore', 'pipe'] }, ); }); @@ -1351,7 +1374,7 @@ describe('runUpdatePreflight', () => { expect(mocks.spawn).toHaveBeenCalledWith( expect.stringMatching(/^npm(\.cmd)?$/), ['install', '-g', '@pythoughts/pythinker-code@0.6.0'], - { detached: true, windowsHide: false, stdio: ['ignore', 'ignore', 'pipe'] }, + { detached: true, windowsHide: false, shell: false, stdio: ['ignore', 'ignore', 'pipe'] }, ); }); @@ -1388,6 +1411,53 @@ describe('runUpdatePreflight', () => { .toBeLessThan(release.mock.invocationCallOrder[0] ?? Number.POSITIVE_INFINITY); }); + it('records a failure when the installer exits 0 without installing the target', async () => { + // The Windows report this exists for: install.ps1 exited 0 repeatedly + // while the executable on disk stayed on the old version, so the footer + // advertised "restart to apply" for a version that never ran. + mocks.readUpdateCache.mockResolvedValue(cacheWith('0.5.0')); + mocks.readUpdateInstallState.mockResolvedValue(installState()); + mocks.refreshUpdateCache.mockResolvedValue(cacheWith('0.5.0')); + mocks.detectInstallSource.mockResolvedValue('native'); + mocks.verifyInstalledVersion.mockResolvedValue({ + ok: false, + reason: 'the installer reported success but /bin/pythinker still reports 0.4.0 (expected 0.5.0)', + }); + mockSpawnExit(0); + const { options } = captureOutput(); + + await expect(runUpdatePreflight('0.4.0', options)).resolves.toBe('continue'); + await flushBackgroundInstall(); + + expect(mocks.verifyInstalledVersion).toHaveBeenCalledWith('native', '0.5.0'); + expect(successOutcomeStates()).toHaveLength(0); + expect(writeUpdateInstallState).toHaveBeenLastCalledWith(expect.objectContaining({ + active: null, + lastFailure: expect.objectContaining({ + version: '0.5.0', + attempts: 1, + message: expect.stringContaining('still reports 0.4.0'), + }), + })); + }); + + it('does not verify an install the installer already reported as failed', async () => { + mocks.readUpdateCache.mockResolvedValue(cacheWith('0.5.0')); + mocks.readUpdateInstallState.mockResolvedValue(installState()); + mocks.refreshUpdateCache.mockResolvedValue(cacheWith('0.5.0')); + mocks.detectInstallSource.mockResolvedValue('native'); + mockSpawnExit(1); + const { options } = captureOutput(); + + await expect(runUpdatePreflight('0.4.0', options)).resolves.toBe('continue'); + await flushBackgroundInstall(); + + expect(mocks.verifyInstalledVersion).not.toHaveBeenCalled(); + expect(writeUpdateInstallState).toHaveBeenLastCalledWith(expect.objectContaining({ + lastFailure: expect.objectContaining({ version: '0.5.0' }), + })); + }); + it('keeps the install lock until a delayed terminal state write completes', async () => { mocks.readUpdateCache.mockResolvedValue(cacheWith('0.5.0')); mocks.readUpdateInstallState.mockResolvedValue(installState()); @@ -1420,7 +1490,9 @@ describe('runUpdatePreflight', () => { await expect(runUpdatePreflight('0.4.0', first.options)).resolves.toBe('continue'); child.emit('exit', 0, null); - await Promise.resolve(); + // The finalizer verifies the installed version before it writes the + // outcome, so the terminal write is more than one microtask away. + await flushBackgroundInstall(); expect(terminalWriteStarted).toBe(true); expect(held).toBe(true); @@ -1504,7 +1576,7 @@ describe('runUpdatePreflight', () => { expect(mocks.spawn).toHaveBeenCalledWith( expect.stringMatching(/^npm(\.cmd)?$/), ['install', '-g', '@pythoughts/pythinker-code@0.5.0'], - { detached: true, windowsHide: false, stdio: ['ignore', 'ignore', 'pipe'] }, + { detached: true, windowsHide: false, shell: false, stdio: ['ignore', 'ignore', 'pipe'] }, ); }); @@ -1870,7 +1942,7 @@ describe('runUpdatePreflight', () => { expect(mocks.spawn).toHaveBeenCalledWith( expect.stringMatching(/^npm(\.cmd)?$/), ['install', '-g', '@pythoughts/pythinker-code@0.5.0'], - { detached: true, windowsHide: false, stdio: ['ignore', 'ignore', 'pipe'] }, + { detached: true, windowsHide: false, shell: false, stdio: ['ignore', 'ignore', 'pipe'] }, ); expect(track).toHaveBeenCalledWith('update_background_install_started', expect.objectContaining({ target_version: '0.5.0', @@ -1996,7 +2068,7 @@ describe('runUpdatePreflight', () => { expect(mocks.spawn).toHaveBeenCalledWith( expect.stringMatching(/^npm(\.cmd)?$/), ['install', '-g', '@pythoughts/pythinker-code@0.5.0'], - { detached: true, windowsHide: false, stdio: ['ignore', 'ignore', 'pipe'] }, + { detached: true, windowsHide: false, shell: false, stdio: ['ignore', 'ignore', 'pipe'] }, ); expect(track).toHaveBeenCalledWith('update_background_install_started', expect.objectContaining({ target_version: '0.5.0', @@ -2332,6 +2404,30 @@ describe('spawnForSource native', () => { }); }); +describe('needsShell', () => { + // Node >=18.20/20.12 refuses to spawn a .cmd without a shell + // (CVE-2024-27980), which is every npm-family update on Windows. + it('is true for the Windows package-manager shims', () => { + expect(needsShell('npm.cmd', 'win32')).toBe(true); + expect(needsShell('pnpm.cmd', 'win32')).toBe(true); + expect(needsShell('YARN.CMD', 'win32')).toBe(true); + }); + + it('is false for real executables and for every other platform', () => { + expect(needsShell('powershell.exe', 'win32')).toBe(false); + expect(needsShell('bun.exe', 'win32')).toBe(false); + expect(needsShell('npm.cmd', 'darwin')).toBe(false); + expect(needsShell('npm', 'linux')).toBe(false); + }); + + it('matches the command spawnForSource picks for each npm-family source', () => { + for (const source of ['npm-global', 'pnpm-global', 'yarn-global'] as const) { + expect(needsShell(spawnForSource(source, '0.5.0', 'win32').cmd, 'win32')).toBe(true); + } + expect(needsShell(spawnForSource('bun-global', '0.5.0', 'win32').cmd, 'win32')).toBe(false); + }); +}); + describe('canAutoInstall native', () => { it('is true on win32 (rename-aside replace no longer needs the platform gate)', () => { expect(canAutoInstall('native', 'win32')).toBe(true); @@ -2352,6 +2448,7 @@ describe('startManualUpdate', () => { mocks.loadTuiConfig.mockResolvedValue(tuiConfig()); mocks.resolveUpdateDeviceId.mockReturnValue('test-device'); mocks.appendRolloutDecisionLog.mockResolvedValue(undefined); + mocks.verifyInstalledVersion.mockResolvedValue({ ok: true }); mocks.tryAcquireUpdateInstallLock.mockResolvedValue({ filePath: '/tmp/pythinker-update-install.lock', release: vi.fn().mockResolvedValue(undefined), diff --git a/apps/pythinker-code/test/cli/update/source.test.ts b/apps/pythinker-code/test/cli/update/source.test.ts index f20cb898..2c699996 100644 --- a/apps/pythinker-code/test/cli/update/source.test.ts +++ b/apps/pythinker-code/test/cli/update/source.test.ts @@ -85,6 +85,22 @@ describe('classifyInstallSource (npm prefix matching)', () => { }); describe('detectInstallSource', () => { + // Every launch calls this. A layout with no reachable package.json used to + // throw out of the preflight instead of classifying as unsupported. + it('returns unsupported when the package root cannot be resolved', async () => { + await expect( + detectInstallSource({ + getPackageRoot: () => { + throw new Error('Could not locate package.json near /opt/pythinker'); + }, + getGlobalPrefix: async () => '/usr/local', + detectNative: () => false, + platform: 'linux', + }), + ).resolves.toBe('unsupported'); + }); + + it('returns pnpm-global when packageRoot matches pnpm heuristic', async () => { await expect( detectInstallSource({ diff --git a/apps/pythinker-code/test/cli/update/verify-install.test.ts b/apps/pythinker-code/test/cli/update/verify-install.test.ts new file mode 100644 index 00000000..51777d7c --- /dev/null +++ b/apps/pythinker-code/test/cli/update/verify-install.test.ts @@ -0,0 +1,140 @@ +import { describe, expect, it, vi } from 'vitest'; + +import { parseVersionOutput, verifyInstalledVersion } from '#/cli/update/verify-install'; + +const NEVER_CALLED = { + probeExecutableVersion: vi.fn(async () => { + throw new Error('probe must not run for this source'); + }), + readPackageVersion: vi.fn(async () => { + throw new Error('package read must not run for this source'); + }), +}; + +describe('parseVersionOutput', () => { + it('reads the bare version Commander prints', () => { + expect(parseVersionOutput('0.13.1\n')).toBe('0.13.1'); + }); + + it('finds the version inside surrounding text', () => { + expect(parseVersionOutput('Pythinker Code v1.2.3 (build 9)')).toBe('1.2.3'); + }); + + it('keeps a prerelease suffix', () => { + expect(parseVersionOutput('2.0.0-rc.1')).toBe('2.0.0-rc.1'); + }); + + it('returns null when there is no version to read', () => { + expect(parseVersionOutput('command not found')).toBeNull(); + }); +}); + +describe('verifyInstalledVersion native', () => { + it('reports the mismatch when the binary still runs the old version', async () => { + const result = await verifyInstalledVersion('native', '0.13.1', { + execPath: 'C:\\Programs\\Pythinker\\pythinker.exe', + probeExecutableVersion: async () => '0.12.0\n', + }); + + expect(result).toEqual({ + ok: false, + reason: expect.stringContaining('still reports 0.12.0 (expected 0.13.1)'), + }); + expect(result).toEqual({ ok: false, reason: expect.stringContaining('pythinker.exe') }); + }); + + it('accepts the install when the binary reports the target version', async () => { + await expect( + verifyInstalledVersion('native', '0.13.1', { + probeExecutableVersion: async () => 'v0.13.1', + }), + ).resolves.toEqual({ ok: true }); + }); + + it('probes the executable that was replaced', async () => { + const probe = vi.fn(async () => '0.13.1'); + await verifyInstalledVersion('native', '0.13.1', { + execPath: '/usr/local/bin/pythinker', + probeExecutableVersion: probe, + }); + + expect(probe).toHaveBeenCalledWith('/usr/local/bin/pythinker'); + }); + + // Fail open: an antivirus scan or a slow first start must never turn a good + // install into a recorded failure that parks the version after two attempts. + it('accepts the install when the probe cannot run', async () => { + await expect( + verifyInstalledVersion('native', '0.13.1', { + probeExecutableVersion: async () => { + throw new Error('ETIMEDOUT'); + }, + }), + ).resolves.toEqual({ ok: true }); + }); + + it('accepts the install when the output carries no version', async () => { + await expect( + verifyInstalledVersion('native', '0.13.1', { + probeExecutableVersion: async () => '', + }), + ).resolves.toEqual({ ok: true }); + }); +}); + +describe('verifyInstalledVersion npm family', () => { + it('reads the installed package rather than spawning the binary', async () => { + const result = await verifyInstalledVersion('npm-global', '0.13.1', { + ...NEVER_CALLED, + readPackageVersion: async () => '0.12.0', + }); + + expect(result).toEqual({ + ok: false, + reason: expect.stringContaining('still 0.12.0 (expected 0.13.1)'), + }); + expect(NEVER_CALLED.probeExecutableVersion).not.toHaveBeenCalled(); + }); + + it('accepts a package that now carries the target version', async () => { + for (const source of ['npm-global', 'pnpm-global', 'yarn-global', 'bun-global'] as const) { + await expect( + verifyInstalledVersion(source, '0.13.1', { + ...NEVER_CALLED, + readPackageVersion: async () => '0.13.1', + }), + ).resolves.toEqual({ ok: true }); + } + }); + + it('accepts the install when the package cannot be read', async () => { + await expect( + verifyInstalledVersion('npm-global', '0.13.1', { + ...NEVER_CALLED, + readPackageVersion: async () => { + throw new Error('ENOENT'); + }, + }), + ).resolves.toEqual({ ok: true }); + }); +}); + +describe('verifyInstalledVersion other sources', () => { + it('checks nothing for homebrew (it installs on the next launch)', async () => { + await expect( + verifyInstalledVersion('homebrew', '0.13.1', NEVER_CALLED), + ).resolves.toEqual({ ok: true }); + }); + + it('checks nothing for an unsupported layout', async () => { + await expect( + verifyInstalledVersion('unsupported', '0.13.1', NEVER_CALLED), + ).resolves.toEqual({ ok: true }); + }); + + it('checks nothing when the target version is not a version', async () => { + await expect( + verifyInstalledVersion('native', 'latest', NEVER_CALLED), + ).resolves.toEqual({ ok: true }); + }); +}); diff --git a/apps/pythinker-web/public/install.ps1 b/apps/pythinker-web/public/install.ps1 index 0c64344f..862ae735 100644 --- a/apps/pythinker-web/public/install.ps1 +++ b/apps/pythinker-web/public/install.ps1 @@ -380,6 +380,25 @@ Unix / macOS / Linux users: } } + # Machine-readable progress for the parent process, mirroring install.sh. + # The background installer has no TTY, so stdout stays human-only (and is + # discarded by the spawn) and stderr carries the protocol: one + # newline-terminated line per update. Without these lines a Windows update in + # flight is indistinguishable from a wedged one. + function Write-MachineProgress([string]$Fields) { + try { [Console]::Error.WriteLine("progress: $Fields") } catch {} + } + + function Write-MachineDownloadProgress([long]$Received, $TotalBytes) { + if ($null -ne $TotalBytes -and [long]$TotalBytes -gt 0) { + $percent = [int][Math]::Floor(($Received * 100) / [long]$TotalBytes) + if ($percent -gt 100) { $percent = 100 } + Write-MachineProgress "state=downloading percent=$percent transferred=$Received total=$([long]$TotalBytes)" + } else { + Write-MachineProgress "state=downloading transferred=$Received" + } + } + function Download-File($Client, [string]$Uri, [string]$Destination, [string]$Label) { $lastError = $null @@ -436,6 +455,9 @@ Unix / macOS / Linux users: $buffer = New-Object byte[] 131072 $stopwatch = [Diagnostics.Stopwatch]::StartNew() $lastRenderMilliseconds = [long]-1000 + $lastMachineMilliseconds = [long]-1000 + + Write-MachineDownloadProgress $received $totalBytes while ($true) { $read = $inputStream.ReadAsync($buffer, 0, $buffer.Length, $attemptCts.Token).GetAwaiter().GetResult() @@ -449,6 +471,13 @@ Unix / macOS / Linux users: $lastRenderMilliseconds = $stopwatch.ElapsedMilliseconds $frameIndex++ } + + # One line per second at most: the parent throttles its own writes, + # and the pipe is shared with the failure tail. + if (($stopwatch.ElapsedMilliseconds - $lastMachineMilliseconds) -ge 1000) { + Write-MachineDownloadProgress $received $totalBytes + $lastMachineMilliseconds = $stopwatch.ElapsedMilliseconds + } } $outputStream.Flush($true) @@ -466,10 +495,12 @@ Unix / macOS / Linux users: if ($received -le 0) { throw "$Label returned an empty file" } [System.IO.File]::Move($partialPath, $Destination) + Write-MachineProgress "state=done transferred=$received" Write-DownloadComplete $received $stopwatch.Elapsed.TotalSeconds return } catch { $lastError = $_.Exception.Message + Write-MachineProgress 'state=failed' } finally { if ($null -ne $outputStream) { $outputStream.Dispose() } if ($null -ne $inputStream) { $inputStream.Dispose() } @@ -561,6 +592,8 @@ Unix / macOS / Linux users: Stop-Installer "release assets for $ResolvedVersion were not available after ${maxElapsed}s.$detail" } + Write-MachineProgress "state=waiting retry_in=$delay elapsed=$elapsed" + if ($useAnimation) { $waitFrames = @('◐', '◓', '◑', '◒') for ($remaining = $delay; $remaining -gt 0; $remaining--) { From dbb7e60f38a1531845c91db03942e38f03ce0ba9 Mon Sep 17 00:00:00 2001 From: elkaix Date: Sat, 8 Aug 2026 18:39:23 -0400 Subject: [PATCH 2/5] fix(update): run Windows shims through the interpreter and verify natively --- .changeset/doctor-native-install.md | 5 + .../update-verify-before-reporting-success.md | 5 + .changeset/windows-installer-progress.md | 5 + .changeset/windows-package-manager-updates.md | 5 + .../src/cli/update/preflight.ts | 46 ++++-- apps/pythinker-code/src/cli/update/source.ts | 33 +++-- .../src/cli/update/verify-install.ts | 111 +++++---------- .../test/cli/update/preflight.test.ts | 132 ++++++++++++------ .../test/cli/update/verify-install.test.ts | 93 +++++------- apps/pythinker-web/public/install.ps1 | 5 +- 10 files changed, 240 insertions(+), 200 deletions(-) create mode 100644 .changeset/doctor-native-install.md create mode 100644 .changeset/update-verify-before-reporting-success.md create mode 100644 .changeset/windows-installer-progress.md create mode 100644 .changeset/windows-package-manager-updates.md diff --git a/.changeset/doctor-native-install.md b/.changeset/doctor-native-install.md new file mode 100644 index 00000000..62882bb4 --- /dev/null +++ b/.changeset/doctor-native-install.md @@ -0,0 +1,5 @@ +--- +"@pythoughts/pythinker-code": patch +--- + +Fix `pythinker doctor` crashing on native installs, and report the last recorded update outcome. diff --git a/.changeset/update-verify-before-reporting-success.md b/.changeset/update-verify-before-reporting-success.md new file mode 100644 index 00000000..8f7aa890 --- /dev/null +++ b/.changeset/update-verify-before-reporting-success.md @@ -0,0 +1,5 @@ +--- +"@pythoughts/pythinker-code": patch +--- + +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. diff --git a/.changeset/windows-installer-progress.md b/.changeset/windows-installer-progress.md new file mode 100644 index 00000000..a0f83827 --- /dev/null +++ b/.changeset/windows-installer-progress.md @@ -0,0 +1,5 @@ +--- +"@pythoughts/pythinker-code": patch +--- + +Show download progress under the prompt while a Windows update installs, instead of nothing until it finishes. diff --git a/.changeset/windows-package-manager-updates.md b/.changeset/windows-package-manager-updates.md new file mode 100644 index 00000000..fd675466 --- /dev/null +++ b/.changeset/windows-package-manager-updates.md @@ -0,0 +1,5 @@ +--- +"@pythoughts/pythinker-code": patch +--- + +Fix automatic updates on Windows for npm, pnpm, and yarn installs, which failed to start at all. diff --git a/apps/pythinker-code/src/cli/update/preflight.ts b/apps/pythinker-code/src/cli/update/preflight.ts index 57a8ef6c..a53208f7 100644 --- a/apps/pythinker-code/src/cli/update/preflight.ts +++ b/apps/pythinker-code/src/cli/update/preflight.ts @@ -86,18 +86,32 @@ function bunCommand(platform: NodeJS.Platform): string { } /** - * Node ≥18.20/20.12 refuses to spawn a `.cmd`/`.bat` file without a shell - * (CVE-2024-27980) and fails with `EINVAL`, which is every npm-family update - * on Windows: `npm.cmd`, `pnpm.cmd`, `yarn.cmd`. Only the package manager - * wrappers need it — the arguments are a fixed flag list plus - * `@`, so nothing here reaches the shell as data. + * Node ≥18.20/20.12 refuses to spawn a `.cmd`/`.bat` file directly + * (CVE-2024-27980) and fails with `EINVAL` — which is every npm-family update + * on Windows: `npm.cmd`, `pnpm.cmd`, `yarn.cmd`. The command interpreter runs + * them instead. It is spelled out as argv rather than `shell: true` so the + * exact command line is visible here (and asserted in tests) instead of being + * assembled by Node's string joining. */ -export function needsShell(cmd: string, platform: NodeJS.Platform): boolean { +function viaCommandInterpreter(command: SpawnCommand): SpawnCommand { + return { + ...command, + cmd: process.env['ComSpec'] ?? 'cmd.exe', + args: ['/d', '/s', '/c', command.cmd, ...command.args], + }; +} + +/** True for the Windows package-manager shims that cannot be spawned directly. */ +export function isWindowsShim(cmd: string, platform: NodeJS.Platform): boolean { if (platform !== 'win32') return false; const lower = cmd.toLowerCase(); return lower.endsWith('.cmd') || lower.endsWith('.bat'); } +function spawnable(command: SpawnCommand, platform: NodeJS.Platform): SpawnCommand { + return isWindowsShim(command.cmd, platform) ? viaCommandInterpreter(command) : command; +} + export function installCommandFor( source: InstallSource, version: string, @@ -162,11 +176,20 @@ export function spawnForSource( ): SpawnCommand { switch (source) { case 'npm-global': - return { cmd: withCmdSuffix('npm', platform), args: ['install', '-g', `${NPM_PACKAGE_NAME}@${version}`] }; + return spawnable( + { cmd: withCmdSuffix('npm', platform), args: ['install', '-g', `${NPM_PACKAGE_NAME}@${version}`] }, + platform, + ); case 'pnpm-global': - return { cmd: withCmdSuffix('pnpm', platform), args: ['add', '-g', `${NPM_PACKAGE_NAME}@${version}`] }; + return spawnable( + { cmd: withCmdSuffix('pnpm', platform), args: ['add', '-g', `${NPM_PACKAGE_NAME}@${version}`] }, + platform, + ); case 'yarn-global': - return { cmd: withCmdSuffix('yarn', platform), args: ['global', 'add', `${NPM_PACKAGE_NAME}@${version}`] }; + return spawnable( + { cmd: withCmdSuffix('yarn', platform), args: ['global', 'add', `${NPM_PACKAGE_NAME}@${version}`] }, + platform, + ); case 'bun-global': return { cmd: bunCommand(platform), args: ['add', '-g', `${NPM_PACKAGE_NAME}@${version}`] }; case 'homebrew': @@ -565,7 +588,6 @@ export async function installUpdate( await new Promise((resolve, reject) => { const child = spawn(cmd, [...args], { stdio: 'inherit', - shell: needsShell(cmd, platform), env: env === undefined ? undefined : { ...process.env, ...env }, }); child.once('error', reject); @@ -928,6 +950,9 @@ async function startBackgroundInstall( logUpdateInfo(logger, 'background update install succeeded', { targetVersion: target.version, source, + // Present when the install was recorded without proof, so a report + // of "it says updated but it did not" is answerable from the log. + unverified: verification.ok ? verification.unverified : undefined, }); return; } @@ -952,7 +977,6 @@ async function startBackgroundInstall( // A detached child gets its own console window on Windows regardless // of stdio; stdio: 'ignore' alone does not suppress it. windowsHide: platform === 'win32', - shell: needsShell(cmd, platform), // stdout stays discarded (install progress is noise); stderr is piped so // the installer's machine-readable progress lines can be recorded and a // failure still keeps the installer's own error text. diff --git a/apps/pythinker-code/src/cli/update/source.ts b/apps/pythinker-code/src/cli/update/source.ts index e77794e5..35113c42 100644 --- a/apps/pythinker-code/src/cli/update/source.ts +++ b/apps/pythinker-code/src/cli/update/source.ts @@ -81,22 +81,25 @@ function execFileText( args: readonly string[], platform: NodeJS.Platform = process.platform, ): Promise { + // `npm.cmd` cannot be spawned directly on Node ≥18.20/20.12 + // (CVE-2024-27980): it fails with EINVAL, and every npm-family Windows + // install then classifies as `unsupported` and never auto-updates. + const viaInterpreter = platform === 'win32' && command.toLowerCase().endsWith('.cmd'); + const spawnCommand = viaInterpreter ? process.env['ComSpec'] ?? 'cmd.exe' : command; + const spawnArgs = viaInterpreter ? ['/d', '/s', '/c', command, ...args] : [...args]; return new Promise((resolveOutput, reject) => { - // `npm.cmd` cannot be spawned without a shell on Node ≥18.20/20.12 - // (CVE-2024-27980); without this the npm prefix lookup fails with EINVAL - // and every npm-family Windows install classifies as `unsupported`. - const options = { - encoding: 'utf-8', - shell: platform === 'win32' && command.toLowerCase().endsWith('.cmd'), - windowsHide: true, - } as const; - execFile(command, [...args], options, (error, stdout) => { - if (error) { - reject(error); - return; - } - resolveOutput(stdout); - }); + execFile( + spawnCommand, + spawnArgs, + { encoding: 'utf-8', windowsHide: true }, + (error, stdout) => { + if (error) { + reject(error); + return; + } + resolveOutput(stdout); + }, + ); }); } diff --git a/apps/pythinker-code/src/cli/update/verify-install.ts b/apps/pythinker-code/src/cli/update/verify-install.ts index d2084b8f..e5df7d7f 100644 --- a/apps/pythinker-code/src/cli/update/verify-install.ts +++ b/apps/pythinker-code/src/cli/update/verify-install.ts @@ -7,48 +7,43 @@ * disk stayed on the old version, so the footer advertised * "restart to apply" forever and the recorded outcome was a lie. * - * This module answers the only question that matters after an install — does - * the thing that runs next report the version we installed? — and it answers - * it from the same artifact the source updates: + * Only a `native` install is verified, and only against the artifact the + * installer replaces — the packaged binary at `process.execPath`, probed with + * `--version` (Commander prints and exits before any preflight runs). The + * npm family is deliberately left unverified: a global reinstall rewrites the + * very directory this process was loaded from, so a read there proves nothing + * about the next launch and a wrong answer would park a healthy version. * - * - native: the packaged binary at `process.execPath`, probed with - * `--version` (Commander prints and exits before any preflight runs). - * - npm/pnpm/yarn/bun: the host `package.json`, re-read from disk. - * - homebrew: nothing — its update lands through the prepare-on-restart - * lifecycle, not through this install path. - * - * It fails **open**: an unreadable package, a probe that times out or a - * version string it cannot parse all report `ok`. A slow antivirus scan must - * never turn a good install into a recorded failure. Only a version it read - * successfully *and* that disagrees with the target is reported as a mismatch. + * It fails **open**: a probe that times out, cannot run, or prints no version + * reports `ok` with an `unverified` note for the caller to log. A slow + * antivirus scan must never turn a good install into a recorded failure. Only + * a version read successfully *and* disagreeing with the target is a mismatch. */ import { execFile } from 'node:child_process'; -import { readFile } from 'node:fs/promises'; - import { valid } from 'semver'; -import { findHostPackageJsonPath } from '#/cli/version'; - +import { formatErrorMessage } from './format-error'; import type { InstallSource } from './types'; /** Bound on the `--version` probe: a native binary starts in well under this. */ const VERSION_PROBE_TIMEOUT_MS = 20_000; export type InstallVerification = - | { readonly ok: true } + /** Installed as expected, or not checkable — `unverified` says which. */ + | { readonly ok: true; readonly unverified?: string } | { readonly ok: false; readonly reason: string }; export interface VerifyInstalledVersionDeps { - /** Path of the packaged binary to probe (native sources only). */ + /** Path of the packaged binary to probe (native installs only). */ readonly execPath: string; /** Runs ` --version` and resolves its stdout. */ readonly probeExecutableVersion: (execPath: string) => Promise; - /** Reads the installed host `package.json`, or null when there is none. */ - readonly readPackageVersion: () => Promise; } -const OK: InstallVerification = { ok: true }; +function unverified(note: string): InstallVerification { + return { ok: true, unverified: note }; +} /** * Extract the first `x.y.z` from a `--version` output. Commander prints the @@ -90,13 +85,6 @@ async function defaultProbeExecutableVersion(execPath: string): Promise }); } -async function defaultReadPackageVersion(): Promise { - const path = findHostPackageJsonPath(); - if (path === null) return null; - const parsed = JSON.parse(await readFile(path, 'utf-8')) as { version?: unknown }; - return typeof parsed.version === 'string' ? parsed.version : null; -} - /** * Verify that `expectedVersion` is what an install of `source` actually left * behind. See the module comment for the fail-open rule. @@ -106,51 +94,28 @@ export async function verifyInstalledVersion( expectedVersion: string, overrides: Partial = {}, ): Promise { - if (valid(expectedVersion) === null) return OK; + if (source !== 'native') return unverified(`not verified for ${source} installs`); + if (valid(expectedVersion) === null) { + return unverified(`not a version to verify against: ${expectedVersion}`); + } - const deps: VerifyInstalledVersionDeps = { - execPath: overrides.execPath ?? process.execPath, - probeExecutableVersion: overrides.probeExecutableVersion ?? defaultProbeExecutableVersion, - readPackageVersion: overrides.readPackageVersion ?? defaultReadPackageVersion, - }; + const execPath = overrides.execPath ?? process.execPath; + const probe = overrides.probeExecutableVersion ?? defaultProbeExecutableVersion; - switch (source) { - case 'native': { - let output: string; - try { - output = await deps.probeExecutableVersion(deps.execPath); - } catch { - return OK; - } - const found = parseVersionOutput(output); - if (found === null || sameVersion(found, expectedVersion)) return OK; - return { - ok: false, - reason: - `the installer reported success but ${deps.execPath} still reports ` + - `${found} (expected ${expectedVersion})`, - }; - } - case 'npm-global': - case 'pnpm-global': - case 'yarn-global': - case 'bun-global': { - let found: string | null; - try { - found = await deps.readPackageVersion(); - } catch { - return OK; - } - if (found === null || sameVersion(found, expectedVersion)) return OK; - return { - ok: false, - reason: - `the installer reported success but the installed package is still ` + - `${found} (expected ${expectedVersion})`, - }; - } - case 'homebrew': - case 'unsupported': - return OK; + let output: string; + try { + output = await probe(execPath); + } catch (error) { + return unverified(`${execPath} could not be run: ${formatErrorMessage(error)}`); } + + const found = parseVersionOutput(output); + if (found === null) return unverified(`${execPath} printed no version`); + if (sameVersion(found, expectedVersion)) return { ok: true }; + return { + ok: false, + reason: + `the installer reported success but ${execPath} still reports ` + + `${found} (expected ${expectedVersion})`, + }; } diff --git a/apps/pythinker-code/test/cli/update/preflight.test.ts b/apps/pythinker-code/test/cli/update/preflight.test.ts index 2b4c9b6f..d6ded06f 100644 --- a/apps/pythinker-code/test/cli/update/preflight.test.ts +++ b/apps/pythinker-code/test/cli/update/preflight.test.ts @@ -12,7 +12,7 @@ import { } from '#/cli/update/install-state'; import { canAutoInstall, - needsShell, + isWindowsShim, runUpdatePreflight, spawnForSource, startManualUpdate, @@ -483,7 +483,7 @@ describe('runUpdatePreflight', () => { expect(mocks.spawn).toHaveBeenCalledWith( expect.stringMatching(/^npm(\.cmd)?$/), ['install', '-g', '@pythoughts/pythinker-code@0.5.0'], - { detached: true, windowsHide: false, shell: false, stdio: ['ignore', 'ignore', 'pipe'] }, + { detached: true, windowsHide: false, stdio: ['ignore', 'ignore', 'pipe'] }, ); }); @@ -548,7 +548,7 @@ describe('runUpdatePreflight', () => { expect(mocks.spawn).toHaveBeenCalledWith( expect.stringMatching(/^npm(\.cmd)?$/), ['install', '-g', '@pythoughts/pythinker-code@0.5.0'], - { stdio: 'inherit', shell: false }, + { stdio: 'inherit' }, ); expect(stdout.join('')).toContain('Updated @pythoughts/pythinker-code to 0.5.0'); }); @@ -574,7 +574,7 @@ describe('runUpdatePreflight', () => { expect(mocks.spawn).toHaveBeenCalledWith( expect.stringMatching(/^npm(\.cmd)?$/), ['install', '-g', '@pythoughts/pythinker-code@0.7.0'], - { stdio: 'inherit', shell: false }, + { stdio: 'inherit' }, ); expect(stdout.join('')).toContain('Updated @pythoughts/pythinker-code to 0.7.0'); }); @@ -619,7 +619,7 @@ describe('runUpdatePreflight', () => { expect(mocks.spawn).toHaveBeenCalledWith( expect.stringMatching(/^npm(\.cmd)?$/u), ['install', '-g', '@pythoughts/pythinker-code@0.11.0'], - { detached: true, windowsHide: false, shell: false, stdio: ['ignore', 'ignore', 'pipe'] }, + { detached: true, windowsHide: false, stdio: ['ignore', 'ignore', 'pipe'] }, ); expect(mocks.spawn).not.toHaveBeenCalledWith( expect.stringMatching(/^npm(\.cmd)?$/u), @@ -656,7 +656,7 @@ describe('runUpdatePreflight', () => { expect(mocks.spawn).toHaveBeenCalledWith( expect.stringMatching(/^npm(\.cmd)?$/u), ['install', '-g', '@pythoughts/pythinker-code@0.10.0'], - { detached: true, windowsHide: false, shell: false, stdio: ['ignore', 'ignore', 'pipe'] }, + { detached: true, windowsHide: false, stdio: ['ignore', 'ignore', 'pipe'] }, ); }); @@ -678,7 +678,7 @@ describe('runUpdatePreflight', () => { expect(mocks.spawn).toHaveBeenCalledWith( expect.stringMatching(/^npm(\.cmd)?$/u), ['install', '-g', '@pythoughts/pythinker-code@0.10.0'], - { detached: true, windowsHide: false, shell: false, stdio: ['ignore', 'ignore', 'pipe'] }, + { detached: true, windowsHide: false, stdio: ['ignore', 'ignore', 'pipe'] }, ); } finally { vi.useRealTimers(); @@ -729,7 +729,7 @@ describe('runUpdatePreflight', () => { expect(mocks.spawn).toHaveBeenCalledWith( expect.stringMatching(/^pnpm(\.cmd)?$/), ['add', '-g', '@pythoughts/pythinker-code@0.5.0'], - { stdio: 'inherit', shell: false }, + { stdio: 'inherit' }, ); }); @@ -745,7 +745,7 @@ describe('runUpdatePreflight', () => { expect(mocks.spawn).toHaveBeenCalledWith( expect.stringMatching(/^yarn(\.cmd)?$/), ['global', 'add', '@pythoughts/pythinker-code@0.5.0'], - { stdio: 'inherit', shell: false }, + { stdio: 'inherit' }, ); }); @@ -761,7 +761,7 @@ describe('runUpdatePreflight', () => { expect(mocks.spawn).toHaveBeenCalledWith( expect.stringMatching(/^bun(\.exe)?$/), ['add', '-g', '@pythoughts/pythinker-code@0.5.0'], - { stdio: 'inherit', shell: false }, + { stdio: 'inherit' }, ); }); @@ -823,7 +823,7 @@ describe('runUpdatePreflight', () => { await runUpdatePreflight('0.4.0', options); const call = mocks.spawn.mock.calls[0]; expect(call?.[0]).toBe('bash'); - expect(call?.[2]).toEqual({ stdio: 'inherit', shell: false }); + expect(call?.[2]).toEqual({ stdio: 'inherit' }); const [flag, script] = call?.[1] as string[]; expect(flag).toBe('-c'); // pipefail must come before the pipeline so a failed `curl` is not masked @@ -864,8 +864,6 @@ describe('runUpdatePreflight', () => { { detached: true, windowsHide: true, - // powershell.exe is a real executable: no shell wrapper needed. - shell: false, stdio: ['ignore', 'ignore', 'pipe'], env: expect.objectContaining({ PYTHINKER_VERSION: '0.5.0' }), }, @@ -926,7 +924,7 @@ describe('runUpdatePreflight', () => { expect(mocks.spawn).toHaveBeenCalledWith( expect.stringMatching(/^npm(\.cmd)?$/u), ['install', '-g', '@pythoughts/pythinker-code@0.5.0'], - { stdio: 'inherit', shell: false }, + { stdio: 'inherit' }, ); }); @@ -1074,7 +1072,7 @@ describe('runUpdatePreflight', () => { expect(mocks.spawn).toHaveBeenCalledWith( expect.stringMatching(/^npm(\.cmd)?$/), ['install', '-g', '@pythoughts/pythinker-code@0.5.0'], - { detached: true, windowsHide: false, shell: false, stdio: ['ignore', 'ignore', 'pipe'] }, + { detached: true, windowsHide: false, stdio: ['ignore', 'ignore', 'pipe'] }, ); expect(writeUpdateInstallState).toHaveBeenCalledWith(expect.objectContaining({ active: expect.objectContaining({ @@ -1218,7 +1216,7 @@ describe('runUpdatePreflight', () => { expect(mocks.spawn).toHaveBeenCalledWith( expect.stringMatching(/^npm(\.cmd)?$/u), ['install', '-g', '@pythoughts/pythinker-code@0.6.0'], - { detached: true, windowsHide: false, shell: false, stdio: ['ignore', 'ignore', 'pipe'] }, + { detached: true, windowsHide: false, stdio: ['ignore', 'ignore', 'pipe'] }, ); }); @@ -1281,7 +1279,7 @@ describe('runUpdatePreflight', () => { expect(mocks.spawn).toHaveBeenCalledWith( expect.stringMatching(/^npm(\.cmd)?$/), ['install', '-g', '@pythoughts/pythinker-code@0.6.0'], - { detached: true, windowsHide: false, shell: false, stdio: ['ignore', 'ignore', 'pipe'] }, + { detached: true, windowsHide: false, stdio: ['ignore', 'ignore', 'pipe'] }, ); }); @@ -1305,7 +1303,7 @@ describe('runUpdatePreflight', () => { expect(mocks.spawn).toHaveBeenCalledWith( expect.stringMatching(/^npm(\.cmd)?$/), ['install', '-g', '@pythoughts/pythinker-code@0.6.0'], - { detached: true, windowsHide: false, shell: false, stdio: ['ignore', 'ignore', 'pipe'] }, + { detached: true, windowsHide: false, stdio: ['ignore', 'ignore', 'pipe'] }, ); }); @@ -1374,7 +1372,7 @@ describe('runUpdatePreflight', () => { expect(mocks.spawn).toHaveBeenCalledWith( expect.stringMatching(/^npm(\.cmd)?$/), ['install', '-g', '@pythoughts/pythinker-code@0.6.0'], - { detached: true, windowsHide: false, shell: false, stdio: ['ignore', 'ignore', 'pipe'] }, + { detached: true, windowsHide: false, stdio: ['ignore', 'ignore', 'pipe'] }, ); }); @@ -1576,7 +1574,7 @@ describe('runUpdatePreflight', () => { expect(mocks.spawn).toHaveBeenCalledWith( expect.stringMatching(/^npm(\.cmd)?$/), ['install', '-g', '@pythoughts/pythinker-code@0.5.0'], - { detached: true, windowsHide: false, shell: false, stdio: ['ignore', 'ignore', 'pipe'] }, + { detached: true, windowsHide: false, stdio: ['ignore', 'ignore', 'pipe'] }, ); }); @@ -1942,7 +1940,7 @@ describe('runUpdatePreflight', () => { expect(mocks.spawn).toHaveBeenCalledWith( expect.stringMatching(/^npm(\.cmd)?$/), ['install', '-g', '@pythoughts/pythinker-code@0.5.0'], - { detached: true, windowsHide: false, shell: false, stdio: ['ignore', 'ignore', 'pipe'] }, + { detached: true, windowsHide: false, stdio: ['ignore', 'ignore', 'pipe'] }, ); expect(track).toHaveBeenCalledWith('update_background_install_started', expect.objectContaining({ target_version: '0.5.0', @@ -2068,7 +2066,7 @@ describe('runUpdatePreflight', () => { expect(mocks.spawn).toHaveBeenCalledWith( expect.stringMatching(/^npm(\.cmd)?$/), ['install', '-g', '@pythoughts/pythinker-code@0.5.0'], - { detached: true, windowsHide: false, shell: false, stdio: ['ignore', 'ignore', 'pipe'] }, + { detached: true, windowsHide: false, stdio: ['ignore', 'ignore', 'pipe'] }, ); expect(track).toHaveBeenCalledWith('update_background_install_started', expect.objectContaining({ target_version: '0.5.0', @@ -2273,6 +2271,39 @@ describe('runUpdatePreflight', () => { expect(messages[0]).not.toContain('percent='); }); + it('parses the exact lines install.ps1 and install.sh emit', async () => { + // Captured from a real run of the installer's progress helpers. Windows + // shipped without these lines, which is why an update in flight looked + // identical to a wedged one there. Drift on either side fails here. + mocks.readUpdateCache.mockResolvedValue(cacheWith('0.5.0')); + mocks.readUpdateInstallState.mockResolvedValue(installState()); + mocks.refreshUpdateCache.mockResolvedValue(cacheWith('0.5.0')); + mocks.detectInstallSource.mockResolvedValue('npm-global'); + mockSpawnExitWithStderr( + 0, + 'progress: state=waiting retry_in=4 elapsed=8\n' + + 'progress: state=downloading transferred=5242880\n' + + 'progress: state=downloading percent=25 transferred=5242880 total=20971520\n' + + 'progress: state=done transferred=20971520\n', + ); + const { options } = captureOutput(); + + await expect(runUpdatePreflight('0.4.0', options)).resolves.toBe('continue'); + await flushBackgroundInstall(); + + const states = progressActiveStates() as Array<{ + active: { progress: { state: string; percent?: number; transferred?: number } }; + }>; + expect(states[0]?.active.progress).toMatchObject({ state: 'waiting' }); + expect(states.at(-1)?.active.progress).toMatchObject({ + state: 'done', + transferred: 20_971_520, + }); + // The downloading lines in between are dropped by the 2s write throttle, + // not by the parser — a line it could not read would throw instead. + expect(states).toHaveLength(2); + }); + it('ignores unknown keys and non-numeric percent values without throwing', async () => { mocks.readUpdateCache.mockResolvedValue(cacheWith('0.5.0')); mocks.readUpdateInstallState.mockResolvedValue(installState()); @@ -2404,27 +2435,50 @@ describe('spawnForSource native', () => { }); }); -describe('needsShell', () => { - // Node >=18.20/20.12 refuses to spawn a .cmd without a shell - // (CVE-2024-27980), which is every npm-family update on Windows. - it('is true for the Windows package-manager shims', () => { - expect(needsShell('npm.cmd', 'win32')).toBe(true); - expect(needsShell('pnpm.cmd', 'win32')).toBe(true); - expect(needsShell('YARN.CMD', 'win32')).toBe(true); +describe('windows package-manager shims', () => { + // Node >=18.20/20.12 refuses to spawn a .cmd directly (CVE-2024-27980), so + // every npm-family update on Windows failed with EINVAL. The command + // interpreter runs them, and the exact argv is asserted here because it + // cannot be exercised from a non-Windows test run. + it('runs npm.cmd through the command interpreter', () => { + const { cmd, args } = spawnForSource('npm-global', '0.5.0', 'win32'); + expect(cmd.toLowerCase()).toContain('cmd.exe'); + expect(args).toEqual([ + '/d', + '/s', + '/c', + 'npm.cmd', + 'install', + '-g', + '@pythoughts/pythinker-code@0.5.0', + ]); + }); + + it('runs pnpm.cmd and yarn.cmd the same way', () => { + expect(spawnForSource('pnpm-global', '0.5.0', 'win32').args).toEqual([ + '/d', '/s', '/c', 'pnpm.cmd', 'add', '-g', '@pythoughts/pythinker-code@0.5.0', + ]); + expect(spawnForSource('yarn-global', '0.5.0', 'win32').args).toEqual([ + '/d', '/s', '/c', 'yarn.cmd', 'global', 'add', '@pythoughts/pythinker-code@0.5.0', + ]); }); - it('is false for real executables and for every other platform', () => { - expect(needsShell('powershell.exe', 'win32')).toBe(false); - expect(needsShell('bun.exe', 'win32')).toBe(false); - expect(needsShell('npm.cmd', 'darwin')).toBe(false); - expect(needsShell('npm', 'linux')).toBe(false); + it('leaves real executables alone', () => { + expect(spawnForSource('bun-global', '0.5.0', 'win32')).toEqual({ + cmd: 'bun.exe', + args: ['add', '-g', '@pythoughts/pythinker-code@0.5.0'], + }); + expect(spawnForSource('native', '0.5.0', 'win32').cmd).toBe('powershell.exe'); }); - it('matches the command spawnForSource picks for each npm-family source', () => { - for (const source of ['npm-global', 'pnpm-global', 'yarn-global'] as const) { - expect(needsShell(spawnForSource(source, '0.5.0', 'win32').cmd, 'win32')).toBe(true); - } - expect(needsShell(spawnForSource('bun-global', '0.5.0', 'win32').cmd, 'win32')).toBe(false); + it('never wraps anything off Windows', () => { + expect(spawnForSource('npm-global', '0.5.0', 'darwin')).toEqual({ + cmd: 'npm', + args: ['install', '-g', '@pythoughts/pythinker-code@0.5.0'], + }); + expect(isWindowsShim('npm.cmd', 'darwin')).toBe(false); + expect(isWindowsShim('npm.cmd', 'win32')).toBe(true); + expect(isWindowsShim('powershell.exe', 'win32')).toBe(false); }); }); diff --git a/apps/pythinker-code/test/cli/update/verify-install.test.ts b/apps/pythinker-code/test/cli/update/verify-install.test.ts index 51777d7c..efb002be 100644 --- a/apps/pythinker-code/test/cli/update/verify-install.test.ts +++ b/apps/pythinker-code/test/cli/update/verify-install.test.ts @@ -2,12 +2,9 @@ import { describe, expect, it, vi } from 'vitest'; import { parseVersionOutput, verifyInstalledVersion } from '#/cli/update/verify-install'; -const NEVER_CALLED = { +const NEVER_PROBED = { probeExecutableVersion: vi.fn(async () => { - throw new Error('probe must not run for this source'); - }), - readPackageVersion: vi.fn(async () => { - throw new Error('package read must not run for this source'); + throw new Error('the binary must not be probed for this source'); }), }; @@ -61,80 +58,54 @@ describe('verifyInstalledVersion native', () => { expect(probe).toHaveBeenCalledWith('/usr/local/bin/pythinker'); }); - // Fail open: an antivirus scan or a slow first start must never turn a good - // install into a recorded failure that parks the version after two attempts. - it('accepts the install when the probe cannot run', async () => { + // Fail open, but say so: an antivirus scan or a slow first start must never + // turn a good install into a recorded failure that parks the version after + // two attempts — and the note is what makes the next report diagnosable. + it('accepts the install unverified when the probe cannot run', async () => { await expect( verifyInstalledVersion('native', '0.13.1', { + execPath: '/usr/local/bin/pythinker', probeExecutableVersion: async () => { throw new Error('ETIMEDOUT'); }, }), - ).resolves.toEqual({ ok: true }); + ).resolves.toEqual({ + ok: true, + unverified: expect.stringContaining('/usr/local/bin/pythinker could not be run'), + }); }); - it('accepts the install when the output carries no version', async () => { + it('accepts the install unverified when the output carries no version', async () => { await expect( verifyInstalledVersion('native', '0.13.1', { probeExecutableVersion: async () => '', }), - ).resolves.toEqual({ ok: true }); - }); -}); - -describe('verifyInstalledVersion npm family', () => { - it('reads the installed package rather than spawning the binary', async () => { - const result = await verifyInstalledVersion('npm-global', '0.13.1', { - ...NEVER_CALLED, - readPackageVersion: async () => '0.12.0', - }); - - expect(result).toEqual({ - ok: false, - reason: expect.stringContaining('still 0.12.0 (expected 0.13.1)'), - }); - expect(NEVER_CALLED.probeExecutableVersion).not.toHaveBeenCalled(); - }); - - it('accepts a package that now carries the target version', async () => { - for (const source of ['npm-global', 'pnpm-global', 'yarn-global', 'bun-global'] as const) { - await expect( - verifyInstalledVersion(source, '0.13.1', { - ...NEVER_CALLED, - readPackageVersion: async () => '0.13.1', - }), - ).resolves.toEqual({ ok: true }); - } + ).resolves.toEqual({ ok: true, unverified: expect.stringContaining('printed no version') }); }); - it('accepts the install when the package cannot be read', async () => { + it('accepts the install unverified when the target is not a version', async () => { await expect( - verifyInstalledVersion('npm-global', '0.13.1', { - ...NEVER_CALLED, - readPackageVersion: async () => { - throw new Error('ENOENT'); - }, - }), - ).resolves.toEqual({ ok: true }); + verifyInstalledVersion('native', 'latest', NEVER_PROBED), + ).resolves.toEqual({ ok: true, unverified: expect.stringContaining('latest') }); }); }); describe('verifyInstalledVersion other sources', () => { - it('checks nothing for homebrew (it installs on the next launch)', async () => { - await expect( - verifyInstalledVersion('homebrew', '0.13.1', NEVER_CALLED), - ).resolves.toEqual({ ok: true }); - }); - - it('checks nothing for an unsupported layout', async () => { - await expect( - verifyInstalledVersion('unsupported', '0.13.1', NEVER_CALLED), - ).resolves.toEqual({ ok: true }); - }); - - it('checks nothing when the target version is not a version', async () => { - await expect( - verifyInstalledVersion('native', 'latest', NEVER_CALLED), - ).resolves.toEqual({ ok: true }); + // A global reinstall rewrites the directory this process was loaded from, + // so nothing readable here proves what the next launch will run. + it('leaves every non-native source unverified without probing', async () => { + for (const source of [ + 'npm-global', + 'pnpm-global', + 'yarn-global', + 'bun-global', + 'homebrew', + 'unsupported', + ] as const) { + await expect( + verifyInstalledVersion(source, '0.13.1', NEVER_PROBED), + ).resolves.toEqual({ ok: true, unverified: `not verified for ${source} installs` }); + } + expect(NEVER_PROBED.probeExecutableVersion).not.toHaveBeenCalled(); }); }); diff --git a/apps/pythinker-web/public/install.ps1 b/apps/pythinker-web/public/install.ps1 index 862ae735..47c9b8c6 100644 --- a/apps/pythinker-web/public/install.ps1 +++ b/apps/pythinker-web/public/install.ps1 @@ -500,7 +500,6 @@ Unix / macOS / Linux users: return } catch { $lastError = $_.Exception.Message - Write-MachineProgress 'state=failed' } finally { if ($null -ne $outputStream) { $outputStream.Dispose() } if ($null -ne $inputStream) { $inputStream.Dispose() } @@ -520,6 +519,10 @@ Unix / macOS / Linux users: } } + # Emitted once, after the last attempt: a `failed` line between retries + # would drop the parent's footer out of its downloading state and back to + # a failure it is about to recover from. + Write-MachineProgress 'state=failed' Stop-Installer "$Label failed after 3 attempts: $lastError" } From 832856e7b9ad280e3c838c50aaa0000448af6fd3 Mon Sep 17 00:00:00 2001 From: elkaix Date: Sat, 8 Aug 2026 18:44:07 -0400 Subject: [PATCH 3/5] fix(update): record why a success could not be verified --- apps/pythinker-code/src/cli/sub/doctor.ts | 5 +++- .../src/cli/update/install-state.ts | 1 + .../src/cli/update/preflight.ts | 1 + apps/pythinker-code/src/cli/update/types.ts | 6 +++++ .../src/cli/update/verify-install.ts | 7 +++--- apps/pythinker-code/test/cli/doctor.test.ts | 7 ++++-- .../test/cli/update/preflight.test.ts | 24 +++++++++++++++++++ .../test/cli/update/verify-install.test.ts | 8 +++---- 8 files changed, 48 insertions(+), 11 deletions(-) diff --git a/apps/pythinker-code/src/cli/sub/doctor.ts b/apps/pythinker-code/src/cli/sub/doctor.ts index 4dc51afd..05de4920 100644 --- a/apps/pythinker-code/src/cli/sub/doctor.ts +++ b/apps/pythinker-code/src/cli/sub/doctor.ts @@ -212,7 +212,10 @@ function resolveDeps(deps: Partial | DoctorDeps | undefined): Resolv installState.lastSuccess === null ? undefined : `${installState.lastSuccess.version} (installed ` + - `${installState.lastSuccess.installedAt})`, + `${installState.lastSuccess.installedAt})` + + (installState.lastSuccess.unverified === undefined + ? '' + : ` — unverified: ${installState.lastSuccess.unverified}`), lastFailure: installState.lastFailure === null ? undefined diff --git a/apps/pythinker-code/src/cli/update/install-state.ts b/apps/pythinker-code/src/cli/update/install-state.ts index c9a7539e..891c61f6 100644 --- a/apps/pythinker-code/src/cli/update/install-state.ts +++ b/apps/pythinker-code/src/cli/update/install-state.ts @@ -153,6 +153,7 @@ const UpdateInstallStateSchema: z.ZodType = z version: z.string().min(1), installedAt: z.string().min(1), notifiedAt: z.string().min(1).nullable(), + unverified: z.string().min(1).optional(), }) .strict() .nullable(), diff --git a/apps/pythinker-code/src/cli/update/preflight.ts b/apps/pythinker-code/src/cli/update/preflight.ts index a53208f7..b508d282 100644 --- a/apps/pythinker-code/src/cli/update/preflight.ts +++ b/apps/pythinker-code/src/cli/update/preflight.ts @@ -927,6 +927,7 @@ async function startBackgroundInstall( version: target.version, installedAt: nowIso(), notifiedAt: null, + unverified: verification.ok ? verification.unverified : undefined, }, } : { diff --git a/apps/pythinker-code/src/cli/update/types.ts b/apps/pythinker-code/src/cli/update/types.ts index caf048c1..03e0da0d 100644 --- a/apps/pythinker-code/src/cli/update/types.ts +++ b/apps/pythinker-code/src/cli/update/types.ts @@ -112,6 +112,12 @@ export interface UpdateInstallSuccess { readonly version: string; readonly installedAt: string; readonly notifiedAt: string | null; + /** + * Why this success was recorded without proof that the new version runs. + * Absent when the installed binary was probed and matched. `doctor` prints + * it, so "it says updated but it did not" is answerable in one command. + */ + readonly unverified?: string; } export interface UpdateInstallState { diff --git a/apps/pythinker-code/src/cli/update/verify-install.ts b/apps/pythinker-code/src/cli/update/verify-install.ts index e5df7d7f..bd42dc04 100644 --- a/apps/pythinker-code/src/cli/update/verify-install.ts +++ b/apps/pythinker-code/src/cli/update/verify-install.ts @@ -94,10 +94,9 @@ export async function verifyInstalledVersion( expectedVersion: string, overrides: Partial = {}, ): Promise { - if (source !== 'native') return unverified(`not verified for ${source} installs`); - if (valid(expectedVersion) === null) { - return unverified(`not a version to verify against: ${expectedVersion}`); - } + // Non-native sources are not checkable from here (see the module comment), + // and a note on every npm install would be noise rather than a signal. + if (source !== 'native' || valid(expectedVersion) === null) return { ok: true }; const execPath = overrides.execPath ?? process.execPath; const probe = overrides.probeExecutableVersion ?? defaultProbeExecutableVersion; diff --git a/apps/pythinker-code/test/cli/doctor.test.ts b/apps/pythinker-code/test/cli/doctor.test.ts index 20480349..b844f94f 100644 --- a/apps/pythinker-code/test/cli/doctor.test.ts +++ b/apps/pythinker-code/test/cli/doctor.test.ts @@ -207,7 +207,8 @@ describe('pythinker doctor', () => { update: { latest: '0.13.1', checkedAt: '2026-08-08T12:00:00.000Z', - lastSuccess: '0.13.1 (installed 2026-08-08T12:01:00.000Z)', + lastSuccess: + '0.13.1 (installed 2026-08-08T12:01:00.000Z) — unverified: probe timed out', lastFailure: 'install 0.13.1 (attempt 1): still reports 0.12.0', }, }), @@ -217,7 +218,9 @@ describe('pythinker doctor', () => { expect(code).toBe(0); const output = stdout.join(''); - expect(output).toContain(' Last update success: 0.13.1 (installed 2026-08-08T12:01:00.000Z)'); + expect(output).toContain( + ' Last update success: 0.13.1 (installed 2026-08-08T12:01:00.000Z) — unverified: probe timed out', + ); expect(output).toContain(' Last update failure: install 0.13.1 (attempt 1): still reports 0.12.0'); }); diff --git a/apps/pythinker-code/test/cli/update/preflight.test.ts b/apps/pythinker-code/test/cli/update/preflight.test.ts index d6ded06f..e124bdd6 100644 --- a/apps/pythinker-code/test/cli/update/preflight.test.ts +++ b/apps/pythinker-code/test/cli/update/preflight.test.ts @@ -1439,6 +1439,30 @@ describe('runUpdatePreflight', () => { })); }); + it('records why a success could not be verified', async () => { + mocks.readUpdateCache.mockResolvedValue(cacheWith('0.5.0')); + mocks.readUpdateInstallState.mockResolvedValue(installState()); + mocks.refreshUpdateCache.mockResolvedValue(cacheWith('0.5.0')); + mocks.detectInstallSource.mockResolvedValue('native'); + mocks.verifyInstalledVersion.mockResolvedValue({ + ok: true, + unverified: '/usr/local/bin/pythinker could not be run: ETIMEDOUT', + }); + mockSpawnExit(0); + const { options } = captureOutput(); + + await expect(runUpdatePreflight('0.4.0', options)).resolves.toBe('continue'); + await flushBackgroundInstall(); + + expect(writeUpdateInstallState).toHaveBeenLastCalledWith(expect.objectContaining({ + lastFailure: null, + lastSuccess: expect.objectContaining({ + version: '0.5.0', + unverified: expect.stringContaining('ETIMEDOUT'), + }), + })); + }); + it('does not verify an install the installer already reported as failed', async () => { mocks.readUpdateCache.mockResolvedValue(cacheWith('0.5.0')); mocks.readUpdateInstallState.mockResolvedValue(installState()); diff --git a/apps/pythinker-code/test/cli/update/verify-install.test.ts b/apps/pythinker-code/test/cli/update/verify-install.test.ts index efb002be..7095176c 100644 --- a/apps/pythinker-code/test/cli/update/verify-install.test.ts +++ b/apps/pythinker-code/test/cli/update/verify-install.test.ts @@ -83,17 +83,17 @@ describe('verifyInstalledVersion native', () => { ).resolves.toEqual({ ok: true, unverified: expect.stringContaining('printed no version') }); }); - it('accepts the install unverified when the target is not a version', async () => { + it('checks nothing when the target is not a version', async () => { await expect( verifyInstalledVersion('native', 'latest', NEVER_PROBED), - ).resolves.toEqual({ ok: true, unverified: expect.stringContaining('latest') }); + ).resolves.toEqual({ ok: true }); }); }); describe('verifyInstalledVersion other sources', () => { // A global reinstall rewrites the directory this process was loaded from, // so nothing readable here proves what the next launch will run. - it('leaves every non-native source unverified without probing', async () => { + it('checks nothing for a source it cannot prove, without probing', async () => { for (const source of [ 'npm-global', 'pnpm-global', @@ -104,7 +104,7 @@ describe('verifyInstalledVersion other sources', () => { ] as const) { await expect( verifyInstalledVersion(source, '0.13.1', NEVER_PROBED), - ).resolves.toEqual({ ok: true, unverified: `not verified for ${source} installs` }); + ).resolves.toEqual({ ok: true }); } expect(NEVER_PROBED.probeExecutableVersion).not.toHaveBeenCalled(); }); From d0f4c81817ff1fb744ef79190a73d84d2108a5cd Mon Sep 17 00:00:00 2001 From: elkaix Date: Sat, 8 Aug 2026 18:53:43 -0400 Subject: [PATCH 4/5] fix(update): keep the unverified reason on manual installs too --- apps/pythinker-code/src/cli/sub/upgrade.ts | 6 ++-- .../src/cli/update/preflight.ts | 8 +++-- apps/pythinker-code/test/cli/upgrade.test.ts | 32 +++++++++++++++++-- 3 files changed, 40 insertions(+), 6 deletions(-) diff --git a/apps/pythinker-code/src/cli/sub/upgrade.ts b/apps/pythinker-code/src/cli/sub/upgrade.ts index e2b545bd..63cc797e 100644 --- a/apps/pythinker-code/src/cli/sub/upgrade.ts +++ b/apps/pythinker-code/src/cli/sub/upgrade.ts @@ -13,6 +13,7 @@ import { } from '#/cli/update/install-state'; import { isTargetInstallable, selectUpdateTarget } from '#/cli/update/select'; import { detectInstallSource } from '#/cli/update/source'; +import type { InstallVerification } from '#/cli/update/verify-install'; import { canAutoInstall, installCommandFor, @@ -47,7 +48,7 @@ export interface UpgradeDeps { source: InstallSource, version: string, platform: NodeJS.Platform, - ) => Promise; + ) => Promise; readonly promptForInstallChoice: ( options: InstallPromptOptions, ) => Promise; @@ -180,7 +181,7 @@ export async function handleUpgrade( target_version: target.version, source, }); - await deps.installUpdate(source, target.version, deps.platform); + const verification = await deps.installUpdate(source, target.version, deps.platform); await deps.writeUpdateInstallState({ ...installState, active: null, @@ -189,6 +190,7 @@ export async function handleUpgrade( version: target.version, installedAt: nowIso(), notifiedAt: null, + unverified: verification.ok ? verification.unverified : undefined, }, }).catch(() => {}); trackUpgradeEvent(deps.track, 'upgrade_command_succeeded', { diff --git a/apps/pythinker-code/src/cli/update/preflight.ts b/apps/pythinker-code/src/cli/update/preflight.ts index b508d282..70566734 100644 --- a/apps/pythinker-code/src/cli/update/preflight.ts +++ b/apps/pythinker-code/src/cli/update/preflight.ts @@ -583,7 +583,7 @@ export async function installUpdate( source: InstallSource, version: string, platform: NodeJS.Platform, -): Promise { +): Promise { const { cmd, args, env } = spawnForSource(source, version, platform); await new Promise((resolve, reject) => { const child = spawn(cmd, [...args], { @@ -605,6 +605,9 @@ export async function installUpdate( // installer gets, instead of printing "Updated …" over an unchanged binary. const verification = await verifyInstalledVersion(source, version); if (!verification.ok) throw new Error(verification.reason); + // Returned so the caller can record *why* a success is unproven; see + // verify-install.ts for the fail-open rule. + return verification; } /** Keep the tail only: installers can be chatty, and the state file is small. */ @@ -1449,7 +1452,7 @@ export async function runUpdatePreflight( if (lock === null) return 'continue'; try { - await installUpdate(source, userVisibleTarget.version, platform); + const verification = await installUpdate(source, userVisibleTarget.version, platform); await writeUpdateInstallState({ ...installState, active: null, @@ -1458,6 +1461,7 @@ export async function runUpdatePreflight( version: userVisibleTarget.version, installedAt: nowIso(), notifiedAt: null, + unverified: verification.ok ? verification.unverified : undefined, }, }).catch(() => {}); stdout.write(renderInstallSuccessMessage(userVisibleTarget)); diff --git a/apps/pythinker-code/test/cli/upgrade.test.ts b/apps/pythinker-code/test/cli/upgrade.test.ts index 12f9b2c4..e282c7d6 100644 --- a/apps/pythinker-code/test/cli/upgrade.test.ts +++ b/apps/pythinker-code/test/cli/upgrade.test.ts @@ -5,6 +5,7 @@ import { emptyUpdateInstallState } from '#/cli/update/install-state'; import type { UpdateInstallLockHandle } from '#/cli/update/install-lock'; import type { InstallPromptChoiceValue } from '#/cli/update/prompt'; import type { InstallSource, UpdateCache, UpdateInstallState } from '#/cli/update/types'; +import type { InstallVerification } from '#/cli/update/verify-install'; function cacheWith( version: string | null, @@ -69,7 +70,11 @@ function createDeps(overrides: { readonly source?: InstallSource; readonly isInteractive?: boolean; readonly promptForInstallChoice?: () => Promise; - readonly installUpdate?: (source: InstallSource, version: string, platform: NodeJS.Platform) => Promise; + readonly installUpdate?: ( + source: InstallSource, + version: string, + platform: NodeJS.Platform, + ) => Promise; readonly readUpdateInstallState?: () => Promise; readonly writeUpdateInstallState?: (state: UpdateInstallState) => Promise; readonly tryAcquireUpdateInstallLock?: () => Promise; @@ -80,7 +85,7 @@ function createDeps(overrides: { source: InstallSource, version: string, platform: NodeJS.Platform, - ) => Promise>().mockResolvedValue(undefined); + ) => Promise>().mockResolvedValue({ ok: true }); return { refreshUpdateCache: vi @@ -212,6 +217,29 @@ describe('handleUpgrade', () => { expect(stdout.join('')).toContain('To update manually, run: npm install -g @pythoughts/pythinker-code@0.5.0'); }); + it('records why a manual install could not be verified', async () => { + const { writable } = captureOutput(); + const writeUpdateInstallState = vi.fn().mockResolvedValue(undefined); + const deps = createDeps({ + latest: '0.5.0', + source: 'native', + installUpdate: vi.fn().mockResolvedValue({ + ok: true, + unverified: '/usr/local/bin/pythinker could not be run: ETIMEDOUT', + }), + writeUpdateInstallState, + }); + + await expect(handleUpgrade('0.4.0', { ...deps, ...writable })).resolves.toBe(0); + + expect(writeUpdateInstallState).toHaveBeenCalledWith(expect.objectContaining({ + lastSuccess: expect.objectContaining({ + version: '0.5.0', + unverified: expect.stringContaining('ETIMEDOUT'), + }), + })); + }); + it('returns a failing exit code when the foreground install fails', async () => { const { stderr, writable } = captureOutput(); const deps = createDeps({ From 2f6934953f43ccd74401755389345b7c115e7b81 Mon Sep 17 00:00:00 2001 From: elkaix Date: Sat, 8 Aug 2026 19:03:13 -0400 Subject: [PATCH 5/5] fix(update): make a failed verification unrepresentable at the install boundary --- apps/pythinker-code/src/cli/sub/upgrade.ts | 8 +++---- .../src/cli/update/preflight.ts | 14 ++++++++----- .../src/cli/update/verify-install.ts | 9 ++++++++ apps/pythinker-code/test/cli/upgrade.test.ts | 21 +++++++++---------- 4 files changed, 32 insertions(+), 20 deletions(-) diff --git a/apps/pythinker-code/src/cli/sub/upgrade.ts b/apps/pythinker-code/src/cli/sub/upgrade.ts index 63cc797e..062ba2ad 100644 --- a/apps/pythinker-code/src/cli/sub/upgrade.ts +++ b/apps/pythinker-code/src/cli/sub/upgrade.ts @@ -13,7 +13,7 @@ import { } from '#/cli/update/install-state'; import { isTargetInstallable, selectUpdateTarget } from '#/cli/update/select'; import { detectInstallSource } from '#/cli/update/source'; -import type { InstallVerification } from '#/cli/update/verify-install'; +import type { InstallOutcome } from '#/cli/update/verify-install'; import { canAutoInstall, installCommandFor, @@ -48,7 +48,7 @@ export interface UpgradeDeps { source: InstallSource, version: string, platform: NodeJS.Platform, - ) => Promise; + ) => Promise; readonly promptForInstallChoice: ( options: InstallPromptOptions, ) => Promise; @@ -181,7 +181,7 @@ export async function handleUpgrade( target_version: target.version, source, }); - const verification = await deps.installUpdate(source, target.version, deps.platform); + const outcome = await deps.installUpdate(source, target.version, deps.platform); await deps.writeUpdateInstallState({ ...installState, active: null, @@ -190,7 +190,7 @@ export async function handleUpgrade( version: target.version, installedAt: nowIso(), notifiedAt: null, - unverified: verification.ok ? verification.unverified : undefined, + unverified: outcome.unverified, }, }).catch(() => {}); trackUpgradeEvent(deps.track, 'upgrade_command_succeeded', { diff --git a/apps/pythinker-code/src/cli/update/preflight.ts b/apps/pythinker-code/src/cli/update/preflight.ts index 70566734..be4a281e 100644 --- a/apps/pythinker-code/src/cli/update/preflight.ts +++ b/apps/pythinker-code/src/cli/update/preflight.ts @@ -56,7 +56,11 @@ import { type UpdateRequestOrigin, type UpdateTarget, } from './types'; -import { verifyInstalledVersion, type InstallVerification } from './verify-install'; +import { + verifyInstalledVersion, + type InstallOutcome, + type InstallVerification, +} from './verify-install'; export type { UpdatePreflightResult } from './types'; @@ -583,7 +587,7 @@ export async function installUpdate( source: InstallSource, version: string, platform: NodeJS.Platform, -): Promise { +): Promise { const { cmd, args, env } = spawnForSource(source, version, platform); await new Promise((resolve, reject) => { const child = spawn(cmd, [...args], { @@ -607,7 +611,7 @@ export async function installUpdate( if (!verification.ok) throw new Error(verification.reason); // Returned so the caller can record *why* a success is unproven; see // verify-install.ts for the fail-open rule. - return verification; + return { unverified: verification.unverified }; } /** Keep the tail only: installers can be chatty, and the state file is small. */ @@ -1452,7 +1456,7 @@ export async function runUpdatePreflight( if (lock === null) return 'continue'; try { - const verification = await installUpdate(source, userVisibleTarget.version, platform); + const outcome = await installUpdate(source, userVisibleTarget.version, platform); await writeUpdateInstallState({ ...installState, active: null, @@ -1461,7 +1465,7 @@ export async function runUpdatePreflight( version: userVisibleTarget.version, installedAt: nowIso(), notifiedAt: null, - unverified: verification.ok ? verification.unverified : undefined, + unverified: outcome.unverified, }, }).catch(() => {}); stdout.write(renderInstallSuccessMessage(userVisibleTarget)); diff --git a/apps/pythinker-code/src/cli/update/verify-install.ts b/apps/pythinker-code/src/cli/update/verify-install.ts index bd42dc04..63642929 100644 --- a/apps/pythinker-code/src/cli/update/verify-install.ts +++ b/apps/pythinker-code/src/cli/update/verify-install.ts @@ -29,6 +29,15 @@ import type { InstallSource } from './types'; /** Bound on the `--version` probe: a native binary starts in well under this. */ const VERSION_PROBE_TIMEOUT_MS = 20_000; +/** + * What a *completed* install still could not prove. A mismatch is never one of + * these — it is thrown by the installer path — so callers cannot mistake a + * failed verification for a success they are free to record. + */ +export interface InstallOutcome { + readonly unverified?: string; +} + export type InstallVerification = /** Installed as expected, or not checkable — `unverified` says which. */ | { readonly ok: true; readonly unverified?: string } diff --git a/apps/pythinker-code/test/cli/upgrade.test.ts b/apps/pythinker-code/test/cli/upgrade.test.ts index e282c7d6..421388a3 100644 --- a/apps/pythinker-code/test/cli/upgrade.test.ts +++ b/apps/pythinker-code/test/cli/upgrade.test.ts @@ -5,7 +5,7 @@ import { emptyUpdateInstallState } from '#/cli/update/install-state'; import type { UpdateInstallLockHandle } from '#/cli/update/install-lock'; import type { InstallPromptChoiceValue } from '#/cli/update/prompt'; import type { InstallSource, UpdateCache, UpdateInstallState } from '#/cli/update/types'; -import type { InstallVerification } from '#/cli/update/verify-install'; +import type { InstallOutcome } from '#/cli/update/verify-install'; function cacheWith( version: string | null, @@ -74,7 +74,7 @@ function createDeps(overrides: { source: InstallSource, version: string, platform: NodeJS.Platform, - ) => Promise; + ) => Promise; readonly readUpdateInstallState?: () => Promise; readonly writeUpdateInstallState?: (state: UpdateInstallState) => Promise; readonly tryAcquireUpdateInstallLock?: () => Promise; @@ -85,7 +85,7 @@ function createDeps(overrides: { source: InstallSource, version: string, platform: NodeJS.Platform, - ) => Promise>().mockResolvedValue({ ok: true }); + ) => Promise>().mockResolvedValue({}); return { refreshUpdateCache: vi @@ -219,24 +219,23 @@ describe('handleUpgrade', () => { it('records why a manual install could not be verified', async () => { const { writable } = captureOutput(); + const unverified = '/usr/local/bin/pythinker could not be run: ETIMEDOUT'; const writeUpdateInstallState = vi.fn().mockResolvedValue(undefined); + // A native target without an artifact for this platform is refused before + // the install runs, so the manifest has to advertise the running one. const deps = createDeps({ latest: '0.5.0', source: 'native', - installUpdate: vi.fn().mockResolvedValue({ - ok: true, - unverified: '/usr/local/bin/pythinker could not be run: ETIMEDOUT', - }), + manifest: manifestForRunningTarget('0.5.0'), + installUpdate: vi.fn().mockResolvedValue({ unverified }), writeUpdateInstallState, }); await expect(handleUpgrade('0.4.0', { ...deps, ...writable })).resolves.toBe(0); + expect(deps.installUpdate).toHaveBeenCalledWith('native', '0.5.0', 'darwin'); expect(writeUpdateInstallState).toHaveBeenCalledWith(expect.objectContaining({ - lastSuccess: expect.objectContaining({ - version: '0.5.0', - unverified: expect.stringContaining('ETIMEDOUT'), - }), + lastSuccess: expect.objectContaining({ version: '0.5.0', unverified }), })); });