From f91073c0894b7d570795a8ab2b6be9d68acc4056 Mon Sep 17 00:00:00 2001 From: Marcus Chandra Date: Wed, 2 Sep 2026 14:20:30 -0700 Subject: [PATCH 1/8] feat(cli): tell the user when their sim is out of date `sim tools execute` shipped in 2.1.5. Someone on 2.1.2 looking for it saw a help listing without it and concluded the CLI could not do it - a missing subcommand is indistinguishable from a feature that was never built, and nothing in the CLI could tell them otherwise. It had no update check, no version negotiation, and no way to learn what "current" is. Once a day, at an interactive terminal, the root `preAction` hook asks `registry.npmjs.org` for the dist-tags of the channel it was installed from and prints one line on stderr when a newer version exists. The request carries the CLI version and nothing else - no key, no workspace, no command - and `SIM_NO_UPDATE_CHECK=1` turns it off. Everything about it fails silently, and it says nothing when stderr is not a terminal, in CI, under `npx`, from a checkout, or to a prerelease install. The last two are not politeness: the repo manifest trails npm permanently by design because the publish workflow bumps the version in-job under `permissions: contents: read` and never commits it back, so without the checkout guard every engineer here would be told daily to upgrade to a version their own tree already contains; and `staging` publishes on every push, so advising a prerelease user would be stale within the hour. Comparison is scoped to one channel, which is what makes "upgrade" to an older stable version structurally impossible rather than merely guarded against. The comparator implements semver precedence including the numeric prerelease rule - `preview.9` precedes `preview.44`, which a string comparison gets backwards. The `preAction` hook is deliberate over a teardown in the entrypoint: commander answers `--help` and `--version` during parsing, so the two latency-sensitive invocations are excluded by construction, and some commands call `process.exit` directly where a `finally` would never run. Timeout is a hard 1s rather than `SIM_TIMEOUT_SECONDS`, which defaults to an hour and governs work the user actually asked for. The check is stamped whether or not it succeeds, so a blackholed registry costs one second a day instead of one per command. --- apps/docs/content/docs/cli/configuration.mdx | 18 ++ .../docs/content/docs/cli/troubleshooting.mdx | 23 ++ packages/sim-cli/README.md | 7 + packages/sim-cli/src/config/paths.ts | 14 + packages/sim-cli/src/program.test.ts | 37 ++- packages/sim-cli/src/program.ts | 6 + packages/sim-cli/src/update/check.test.ts | 280 ++++++++++++++++ packages/sim-cli/src/update/check.ts | 301 ++++++++++++++++++ packages/sim-cli/src/update/semver.test.ts | 94 ++++++ packages/sim-cli/src/update/semver.ts | 133 ++++++++ 10 files changed, 912 insertions(+), 1 deletion(-) create mode 100644 packages/sim-cli/src/update/check.test.ts create mode 100644 packages/sim-cli/src/update/check.ts create mode 100644 packages/sim-cli/src/update/semver.test.ts create mode 100644 packages/sim-cli/src/update/semver.ts diff --git a/apps/docs/content/docs/cli/configuration.mdx b/apps/docs/content/docs/cli/configuration.mdx index b5ba49ac7d4..0d0f402141f 100644 --- a/apps/docs/content/docs/cli/configuration.mdx +++ b/apps/docs/content/docs/cli/configuration.mdx @@ -119,6 +119,24 @@ shared profile cannot also set its own endpoint or API key. | `SIM_CREDENTIALS_FILE` | Relocate only the credentials file | | `SIM_TIMEOUT_SECONDS` | Per-request timeout; `0` waits indefinitely. Defaults to `3600`, above every timeout the server itself applies | | `SIM_DEBUG` | Trace each request's method, URL, status and duration to stderr | +| `SIM_NO_UPDATE_CHECK` | Turn off the once-a-day update notice | + +## Update notices + +At most once a day, and only when stderr is a terminal, the CLI asks +`registry.npmjs.org` which version is published under the dist-tag it was +installed from. When a newer one exists it prints a single line on stderr naming +both versions and the command that upgrades: + +``` +Update available: sim 2.1.2 → 2.1.5. Run: npm install -g sim@latest +``` + +The request carries the CLI version and nothing else — no API key, no workspace, +no command. It is skipped entirely when stderr is redirected, in CI, under +`npx`, and for prerelease installs, so scripted output is never affected. Set +`SIM_NO_UPDATE_CHECK=1` to turn it off, and `npm_config_registry` to ask a +mirror instead. Node ignores `HTTPS_PROXY` unless you also set `NODE_USE_ENV_PROXY=1`, and only from Node 22.21 and 24.5. The CLI warns when a proxy is configured but will not diff --git a/apps/docs/content/docs/cli/troubleshooting.mdx b/apps/docs/content/docs/cli/troubleshooting.mdx index 8bf2dfd58c0..1bb584c1f04 100644 --- a/apps/docs/content/docs/cli/troubleshooting.mdx +++ b/apps/docs/content/docs/cli/troubleshooting.mdx @@ -85,6 +85,29 @@ editing the file by hand: sim --output table configure --set-output json ``` +## A command is missing that the documentation describes + +The docs track the current release, so a command that exists here and not in +`sim --help` usually means the installed CLI is older than the feature. Compare +`sim --version` against the published version and upgrade: + +```bash +sim --version +npm install -g sim@latest +``` + +The CLI normally tells you this itself, once a day, on stderr. It stays quiet +when stderr is redirected, in CI, and under `npx`. + +## An update notice appears in output I am parsing + +It should not: the notice is written to stderr, never stdout, so `--output json` +piped to `jq` is unaffected. If something merges the two streams, silence it: + +```bash +export SIM_NO_UPDATE_CHECK=1 +``` + ## Anything else An unexpected error prints a stack trace. That is a bug in the CLI — please diff --git a/packages/sim-cli/README.md b/packages/sim-cli/README.md index fd86dc9ca79..a7e5687fa1c 100644 --- a/packages/sim-cli/README.md +++ b/packages/sim-cli/README.md @@ -259,6 +259,13 @@ The main environment variables are: | `SIM_CONFIG_DIR` | Directory containing CLI config and credentials | | `SIM_TIMEOUT_SECONDS` | Per-request timeout; `0` waits indefinitely | | `SIM_DEBUG` | Print request diagnostics to stderr | +| `SIM_NO_UPDATE_CHECK` | Turn off the update notice | + +Once a day, at an interactive terminal, `sim` asks `registry.npmjs.org` which +version is published under the tag it was installed from, and prints one line on +stderr when a newer one exists. It sends nothing but its own version, never a +key, and stays quiet when stderr is not a terminal, in CI, and under `npx`. Set +`SIM_NO_UPDATE_CHECK=1` to turn it off. ## Documentation diff --git a/packages/sim-cli/src/config/paths.ts b/packages/sim-cli/src/config/paths.ts index 158a356d57c..9618931e080 100644 --- a/packages/sim-cli/src/config/paths.ts +++ b/packages/sim-cli/src/config/paths.ts @@ -19,3 +19,17 @@ export function configPath(): string { export function credentialsPath(): string { return process.env.SIM_CREDENTIALS_FILE || join(configDir(), 'credentials') } + +/** + * Where the once-a-day update check remembers that it ran. + * + * Cache, not configuration, so it is safe to delete at any time and gets no + * `SIM_*` override of its own: nobody relocates a cache deliberately, and + * `SIM_CONFIG_DIR` already moves it for the two callers that matter — the test + * harness and anyone keeping `~/.sim` somewhere else. It is kept out of the + * config file because that file is INI the user edits, and a timestamp inside a + * `[profile x]` section would surface in `sim configure` and `sim whoami`. + */ +export function updateCachePath(): string { + return join(configDir(), 'update-check.json') +} diff --git a/packages/sim-cli/src/program.test.ts b/packages/sim-cli/src/program.test.ts index bb3b94ba5af..2d8870980f5 100644 --- a/packages/sim-cli/src/program.test.ts +++ b/packages/sim-cli/src/program.test.ts @@ -1,8 +1,11 @@ /** * @vitest-environment node */ +import { mkdtempSync, readdirSync, rmSync } from 'node:fs' +import { tmpdir } from 'node:os' +import { join } from 'node:path' import type { Command } from 'commander' -import { describe, expect, it } from 'vitest' +import { describe, expect, it, vi } from 'vitest' import { buildProgram } from './program' import { CLI_VERSION } from './version' @@ -159,3 +162,35 @@ describe('help typed after a command that does not exist', () => { expect(implicit.out).toContain('Usage: sim profiles add') }) }) + +describe('the update check', () => { + /** + * The notice must cost `--version` and `--help` nothing. Commander answers + * both during parsing, before any action hook runs, so the guarantee is + * structural — this holds it in place if the check is ever moved. + */ + it('never runs for the two commands commander answers during parsing', async () => { + const stderr = process.stderr + const wasTty = stderr.isTTY + const dir = mkdtempSync(join(tmpdir(), 'sim-cli-program-')) + const requests: string[] = [] + Object.defineProperty(stderr, 'isTTY', { configurable: true, value: true }) + process.env.SIM_CONFIG_DIR = dir + vi.stubGlobal('fetch', (input: URL) => { + requests.push(String(input)) + return Promise.resolve(Response.json({ latest: '99.0.0' })) + }) + + try { + await parse(['--version']) + await parse(['--help']) + expect(requests).toEqual([]) + expect(readdirSync(dir)).toEqual([]) + } finally { + vi.unstubAllGlobals() + process.env.SIM_CONFIG_DIR = undefined + Object.defineProperty(stderr, 'isTTY', { configurable: true, value: wasTty }) + rmSync(dir, { recursive: true, force: true }) + } + }) +}) diff --git a/packages/sim-cli/src/program.ts b/packages/sim-cli/src/program.ts index e2e784672d4..f184df5592c 100644 --- a/packages/sim-cli/src/program.ts +++ b/packages/sim-cli/src/program.ts @@ -10,6 +10,7 @@ import { buildGeneratedCommands, refuseHelpAfterUnknownCommand, } from './runtime/build' +import { announceUpdateIfAvailable } from './update/check' import { CLI_VERSION } from './version' /** Root program description, shared by `--help` and the generated docs. */ @@ -151,6 +152,11 @@ export function buildProgram(options: { version?: boolean } = {}): Command { program.addHelpText('after', HELP_EPILOGUE) + // Root hooks are inherited by the whole tree, and commander answers `--help` + // and `--version` during parsing without ever reaching an action — so the two + // invocations that must stay instant are excluded by construction. + program.hook('preAction', () => announceUpdateIfAvailable()) + refuseHelpAfterUnknownCommand(program) assertNoReservedProgramFlags(program) diff --git a/packages/sim-cli/src/update/check.test.ts b/packages/sim-cli/src/update/check.test.ts new file mode 100644 index 00000000000..039eafd2362 --- /dev/null +++ b/packages/sim-cli/src/update/check.test.ts @@ -0,0 +1,280 @@ +/** + * @vitest-environment node + */ +import { mkdtempSync, readFileSync, rmSync, statSync, writeFileSync } from 'node:fs' +import { tmpdir } from 'node:os' +import { join } from 'node:path' +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import { announceUpdateIfAvailable, resetUpdateCheck, upgradeCommand } from './check' + +/** A global install, which is the only shape that gets advised at all. */ +const INSTALLED = '/usr/local/lib/node_modules/sim/dist/index.js' + +let configDir: string +let notices: string[] +let fetched: URL[] + +/** Answers the dist-tags request the way the registry does. */ +function stubRegistry(tags: Record | 'reject' | 'not-found' | 'html'): void { + vi.stubGlobal('fetch', (input: URL) => { + fetched.push(input) + if (tags === 'reject') return Promise.reject(new Error('getaddrinfo ENOTFOUND')) + if (tags === 'not-found') return Promise.resolve(new Response('', { status: 404 })) + if (tags === 'html') return Promise.resolve(new Response('nope', { status: 200 })) + return Promise.resolve(Response.json(tags)) + }) +} + +async function run(overrides: Parameters[0] = {}) { + await announceUpdateIfAvailable({ + currentVersion: '2.1.2', + env: {}, + isTty: true, + modulePath: INSTALLED, + write: (message) => notices.push(message), + ...overrides, + }) +} + +function cachePath(): string { + return join(configDir, 'update-check.json') +} + +beforeEach(() => { + resetUpdateCheck() + configDir = mkdtempSync(join(tmpdir(), 'sim-cli-update-')) + process.env.SIM_CONFIG_DIR = configDir + notices = [] + fetched = [] + stubRegistry({ latest: '2.1.5' }) +}) + +afterEach(() => { + vi.unstubAllGlobals() + process.env.SIM_CONFIG_DIR = undefined + rmSync(configDir, { recursive: true, force: true }) +}) + +describe('announcing a newer release', () => { + it('names both versions and the command that closes the gap', async () => { + await run() + expect(notices.join('')).toBe( + 'Update available: sim 2.1.2 → 2.1.5. Run: npm install -g sim@latest\n' + ) + }) + + it('asks the registry for the dist-tags and nothing else', async () => { + await run() + expect(fetched.map(String)).toEqual(['https://registry.npmjs.org/-/package/sim/dist-tags']) + }) + + it('stays silent when the installed version is current', async () => { + await run({ currentVersion: '2.1.5' }) + expect(notices).toEqual([]) + }) + + it('stays silent when the installed version is ahead of the tag', async () => { + await run({ currentVersion: '2.2.0' }) + expect(notices).toEqual([]) + }) + + it('writes nothing to stdout, which may be a pipeline', async () => { + const originalWrite = process.stdout.write + const stdout: string[] = [] + process.stdout.write = ((chunk: string) => { + stdout.push(String(chunk)) + return true + }) as typeof process.stdout.write + try { + await run() + } finally { + process.stdout.write = originalWrite + } + expect(notices).toHaveLength(1) + expect(stdout).toEqual([]) + }) +}) + +describe('when the notice is suppressed', () => { + it('respects SIM_NO_UPDATE_CHECK', async () => { + await run({ env: { SIM_NO_UPDATE_CHECK: '1' } }) + expect(fetched).toEqual([]) + expect(notices).toEqual([]) + }) + + it('treats an explicitly off value as not set', async () => { + await run({ env: { SIM_NO_UPDATE_CHECK: '0' } }) + expect(notices).toHaveLength(1) + }) + + it('says nothing when stderr is not a terminal', async () => { + await run({ isTty: false }) + expect(fetched).toEqual([]) + expect(notices).toEqual([]) + }) + + it('says nothing in CI, even where CI allocates a terminal', async () => { + await run({ env: { BUILDKITE: 'true' } }) + expect(notices).toEqual([]) + }) + + it('says nothing under npx, which resolves the tag on every run', async () => { + await run({ modulePath: '/Users/x/.npm/_npx/a1b2/node_modules/sim/dist/index.js' }) + expect(notices).toEqual([]) + }) + + it('says nothing from a checkout, whose manifest trails npm by design', async () => { + await run({ modulePath: '/Users/x/sim/packages/sim-cli/dist/index.js' }) + expect(notices).toEqual([]) + }) + + it('says nothing to a prerelease install', async () => { + stubRegistry({ latest: '2.1.5', staging: '2.1.6-preview.812.1' }) + await run({ currentVersion: '2.1.3-preview.44.1' }) + expect(fetched).toEqual([]) + expect(notices).toEqual([]) + }) + + it('says nothing when the running version cannot be read', async () => { + await run({ currentVersion: 'not-a-version' }) + expect(notices).toEqual([]) + }) + + it('speaks only once per process', async () => { + await run() + rmSync(cachePath(), { force: true }) + await run() + expect(notices).toHaveLength(1) + }) +}) + +describe('the once-a-day cache', () => { + it('records the check, and what it found', async () => { + const now = new Date('2026-09-02T10:00:00.000Z') + await run({ now }) + expect(JSON.parse(readFileSync(cachePath(), 'utf8'))).toEqual({ + version: 1, + checkedAt: '2026-09-02T10:00:00.000Z', + latestVersion: '2.1.5', + }) + // Not secret, but not world-writable either. + expect(statSync(cachePath()).mode & 0o777).toBe(0o644) + }) + + it('does not contact the registry again within the day', async () => { + await run({ now: new Date('2026-09-02T10:00:00.000Z') }) + resetUpdateCheck() + fetched = [] + notices = [] + await run({ now: new Date('2026-09-02T22:00:00.000Z') }) + expect(fetched).toEqual([]) + expect(notices).toEqual([]) + }) + + it('checks again once the day is up', async () => { + await run({ now: new Date('2026-09-02T10:00:00.000Z') }) + resetUpdateCheck() + notices = [] + await run({ now: new Date('2026-09-03T11:00:00.000Z') }) + expect(notices).toHaveLength(1) + }) + + it('checks again when the clock has moved backwards', async () => { + // A stamp in the future would otherwise suppress the notice until the clock + // caught up, which after a one-off jump forward is permanently. + await run({ now: new Date('2026-09-02T10:00:00.000Z') }) + resetUpdateCheck() + notices = [] + await run({ now: new Date('2026-09-01T10:00:00.000Z') }) + expect(notices).toHaveLength(1) + }) + + it('re-checks rather than trusting a truncated file', async () => { + writeFileSync(cachePath(), '{"version": 1, "checked') + await run() + expect(notices).toHaveLength(1) + }) + + it('re-checks rather than trusting a cache a newer CLI wrote', async () => { + writeFileSync(cachePath(), JSON.stringify({ version: 99, checkedAt: new Date().toISOString() })) + await run() + expect(notices).toHaveLength(1) + }) + + it('still runs the command when the cache cannot be written', async () => { + const wall = join(configDir, 'wall') + writeFileSync(wall, 'not a directory') + process.env.SIM_CONFIG_DIR = join(wall, 'sim') + await expect(run()).resolves.toBeUndefined() + expect(notices).toHaveLength(1) + }) +}) + +describe('when the registry does not answer', () => { + it.each([ + ['the request fails', 'reject' as const], + ['the response is an error', 'not-found' as const], + ['a proxy answers with an HTML page', 'html' as const], + ])('stays silent and does not throw when %s', async (_label, behaviour) => { + stubRegistry(behaviour) + await expect(run()).resolves.toBeUndefined() + expect(notices).toEqual([]) + }) + + it('stays silent when the tag is missing or is not a version', async () => { + stubRegistry({ staging: '2.1.6-preview.1.1' }) + await run() + expect(notices).toEqual([]) + + resetUpdateCheck() + rmSync(cachePath(), { force: true }) + stubRegistry({ latest: 'nonsense' }) + await run() + expect(notices).toEqual([]) + }) + + it('still records the attempt, so a dead registry costs one request a day', async () => { + stubRegistry('reject') + await run({ now: new Date('2026-09-02T10:00:00.000Z') }) + expect(JSON.parse(readFileSync(cachePath(), 'utf8')).latestVersion).toBeNull() + }) + + it('asks a configured mirror instead of the default', async () => { + await run({ env: { npm_config_registry: 'https://npm.internal/api/npm' } }) + expect(fetched.map(String)).toEqual(['https://npm.internal/api/npm/-/package/sim/dist-tags']) + }) + + it('ignores a mirror setting that is not an http url', async () => { + await run({ env: { npm_config_registry: 'not a url' } }) + expect(fetched.map(String)).toEqual(['https://registry.npmjs.org/-/package/sim/dist-tags']) + }) +}) + +describe('the upgrade command', () => { + it.each([ + ['/usr/local/lib/node_modules/sim/dist/index.js', 'npm install -g sim@latest'], + ['/Users/x/.bun/install/global/node_modules/sim/dist/index.js', 'bun add -g sim@latest'], + ['/Users/x/Library/pnpm/global/5/node_modules/sim/dist/index.js', 'pnpm add -g sim@latest'], + ['/Users/x/.yarn/global/node_modules/sim/dist/index.js', 'yarn global add sim@latest'], + [ + 'C:\\Users\\x\\AppData\\Roaming\\npm\\node_modules\\sim\\dist\\index.js', + 'npm install -g sim@latest', + ], + [ + 'C:\\Users\\x\\AppData\\Local\\pnpm\\global\\5\\node_modules\\sim\\dist\\index.js', + 'pnpm add -g sim@latest', + ], + ])('reads %s as the installation it is', (modulePath, expected) => { + expect(upgradeCommand('latest', modulePath, {})).toBe(expected) + }) + + it('falls back to the invoking package manager when the path says nothing', () => { + expect( + upgradeCommand('latest', INSTALLED, { npm_config_user_agent: 'pnpm/9.1.0 npm/? node/v22' }) + ).toBe('pnpm add -g sim@latest') + }) + + it('names the channel it is advising, not always the stable one', () => { + expect(upgradeCommand('staging', INSTALLED, {})).toBe('npm install -g sim@staging') + }) +}) diff --git a/packages/sim-cli/src/update/check.ts b/packages/sim-cli/src/update/check.ts new file mode 100644 index 00000000000..8cb5c4d07f3 --- /dev/null +++ b/packages/sim-cli/src/update/check.ts @@ -0,0 +1,301 @@ +/** + * The once-a-day "there is a newer sim" notice. + * + * It exists because a missing subcommand is indistinguishable from a feature + * that was never built: someone on 2.1.2 looking for `sim tools execute` — added + * in 2.1.5 — sees a help listing without it and concludes the CLI cannot do it. + * The version is the only thing that can tell them otherwise. + * + * Everything here fails silently. A courtesy notice that breaks a command, or + * that writes anything to stdout, is worse than no notice at all. + */ + +import { mkdirSync, readFileSync, writeFileSync } from 'node:fs' +import { dirname } from 'node:path' +import { fileURLToPath } from 'node:url' +import { updateCachePath } from '../config/paths' +import { CLI_VERSION } from '../version' +import { channelOf, compareVersions, parseVersion, type ReleaseChannel } from './semver' + +/** How long a check is trusted. The stated contract is one notice per day. */ +const CHECK_INTERVAL_MS = 24 * 60 * 60 * 1000 + +/** + * Deliberately not `SIM_TIMEOUT_SECONDS`, which defaults to an hour: that bound + * governs work the user asked for, and this is work they did not. + */ +const REGISTRY_TIMEOUT_MS = 1000 + +const DEFAULT_REGISTRY = 'https://registry.npmjs.org' + +/** + * Set by every CI provider worth naming. `!isTTY` already covers most of them; + * this catches the ones that allocate a terminal anyway, such as a Buildkite + * agent or `docker run -t`. + */ +const CI_VARIABLES = [ + 'CI', + 'GITHUB_ACTIONS', + 'JENKINS_URL', + 'TEAMCITY_VERSION', + 'BUILDKITE', +] as const + +/** The shape written to `~/.sim/update-check.json`. */ +interface UpdateCacheEntry { + /** + * Forward compatibility hinge. A reader that does not recognise the number + * treats the file as absent and checks again, so an older CLI can never be + * confused by a newer one's cache — and neither ever has to migrate it. + */ + version: 1 + checkedAt: string + latestVersion: string | null +} + +const CACHE_VERSION = 1 + +export interface UpdateCheckOptions { + currentVersion?: string + env?: NodeJS.ProcessEnv + /** Whether stderr is a terminal. Injected so the suppression rule is testable. */ + isTty?: boolean + /** Location of the running module, used to recognise npx and local builds. */ + modulePath?: string + now?: Date + write?: (message: string) => void +} + +/** + * One notice per process, no matter how the hook is reached. Commander runs a + * single action per parse, so this is belt and braces rather than load-bearing. + */ +let announced = false + +/** Test seam: the guard above is process-global, and each test needs it clear. */ +export function resetUpdateCheck(): void { + announced = false +} + +/** Anything but unset, empty, `0` or `false` turns a switch on. */ +function isEnabled(value: string | undefined): boolean { + if (value === undefined) return false + const normalized = value.trim().toLowerCase() + return normalized !== '' && normalized !== '0' && normalized !== 'false' +} + +/** + * Whether this installation is one a "please upgrade" line cannot help. + * + * `npx` resolves the dist-tag on every invocation, so its user is by definition + * already current. A checkout is the sharper case: the repo manifest trails npm + * permanently and by design, because the publish workflow bumps the version + * in-job under `permissions: contents: read` and never commits it back. Without + * this, every Sim engineer running a local build would be told daily to upgrade + * to a version their own tree already contains. + */ +function isUnadvisableInstall(modulePath: string): boolean { + const normalized = modulePath.replace(/\\/g, '/') + return normalized.includes('/_npx/') || normalized.includes('/packages/sim-cli/') +} + +/** + * The registry to ask. `npm_config_registry` is only set when the CLI is invoked + * through a package-manager script, so this is inconsistent by nature — but the + * case it rescues is real: behind a mirror with `registry.npmjs.org` firewalled, + * the default would fail forever while the mirror holds the right answer. + * + * `.npmrc` is deliberately not parsed. That is an INI format with scopes and + * auth tokens, and reading it is where a courtesy check would start growing. + */ +function registryBase(env: NodeJS.ProcessEnv): string { + const configured = env.npm_config_registry?.trim() + if (!configured) return DEFAULT_REGISTRY + try { + const url = new URL(configured) + if (url.protocol === 'http:' || url.protocol === 'https:') { + return configured.endsWith('/') ? configured : `${configured}/` + } + } catch { + // An unparseable value is not worth reporting; the default still works. + } + return DEFAULT_REGISTRY +} + +/** + * The published dist-tags, or null if anything at all goes wrong. + * + * `-/package/sim/dist-tags` is about a hundred bytes and answers exactly the + * question asked. The abbreviated packument would be tens of kilobytes and list + * every version ever published. + * + * The User-Agent is cut down to the bare version: the full one from + * `version.ts` carries the Node version, platform and architecture, which is + * useful in our own logs and gratuitous to hand a third party. + */ +async function fetchDistTags(env: NodeJS.ProcessEnv): Promise | null> { + try { + const response = await fetch(new URL('-/package/sim/dist-tags', registryBase(env)), { + headers: { accept: 'application/json', 'user-agent': `sim-cli/${CLI_VERSION}` }, + signal: AbortSignal.timeout(REGISTRY_TIMEOUT_MS), + }) + if (!response.ok) return null + const body: unknown = await response.json() + if (typeof body !== 'object' || body === null || Array.isArray(body)) return null + // Some proxies answer with an HTML error page under a 200, so the values are + // checked rather than assumed. + const tags: Record = {} + for (const [tag, version] of Object.entries(body)) { + if (typeof version === 'string') tags[tag] = version + } + return tags + } catch { + return null + } +} + +function readCache(path: string): UpdateCacheEntry | null { + try { + const parsed: unknown = JSON.parse(readFileSync(path, 'utf8')) + if (typeof parsed !== 'object' || parsed === null) return null + const entry = parsed as Partial + if (entry.version !== CACHE_VERSION) return null + if (typeof entry.checkedAt !== 'string' || Number.isNaN(Date.parse(entry.checkedAt))) + return null + return { + version: CACHE_VERSION, + checkedAt: entry.checkedAt, + latestVersion: typeof entry.latestVersion === 'string' ? entry.latestVersion : null, + } + } catch { + // Absent, unreadable, or truncated by an interleaved writer — all of which + // mean the same thing here: check again. + return null + } +} + +/** + * Records that a check happened, whether or not it produced an answer. + * + * Stamping on failure too is what keeps a blackholed registry costing one second + * a day instead of one second per command. + * + * There is no temp-file-and-rename. Two concurrent invocations can interleave + * and truncate this file; the reader treats a truncated file as no cache, so the + * whole cost of the race is one extra HTTP request. + */ +function writeCache(path: string, entry: UpdateCacheEntry): void { + try { + mkdirSync(dirname(path), { recursive: true, mode: 0o700 }) + writeFileSync(path, `${JSON.stringify(entry, null, 2)}\n`, { mode: 0o644 }) + } catch { + // A read-only home directory is ordinary in a container, and it must not + // stop the command the user actually ran. + } +} + +/** Whether a recorded check is recent enough to skip this one. */ +function isFresh(entry: UpdateCacheEntry, now: Date): boolean { + const age = now.getTime() - Date.parse(entry.checkedAt) + // A negative age means the clock moved backwards since the write. Treating it + // as fresh would suppress the notice until the clock caught up, which after a + // one-off jump forward is forever. + return age >= 0 && age < CHECK_INTERVAL_MS +} + +/** + * The command that upgrades *this* installation. + * + * The path is asked first because it describes the installation; the + * environment is a fallback because for a globally installed CLI it usually + * describes nothing but the shell that happened to invoke it. + */ +export function upgradeCommand( + channel: ReleaseChannel, + modulePath: string = fileURLToPath(import.meta.url), + env: NodeJS.ProcessEnv = process.env +): string { + const target = `sim@${channel}` + const normalized = modulePath.replace(/\\/g, '/').toLowerCase() + + if (normalized.includes('.bun/install/global')) return `bun add -g ${target}` + if (normalized.includes('/pnpm/') || normalized.includes('/.pnpm/')) { + return `pnpm add -g ${target}` + } + if (normalized.includes('/.yarn/') || normalized.includes('/yarn/')) { + return `yarn global add ${target}` + } + + const agent = env.npm_config_user_agent ?? '' + if (agent.startsWith('pnpm/')) return `pnpm add -g ${target}` + if (agent.startsWith('yarn/')) return `yarn global add ${target}` + if (agent.startsWith('bun/')) return `bun add -g ${target}` + + return `npm install -g ${target}` +} + +/** + * Tells the user once a day when the channel they installed from has moved on. + * + * Wired as a root `preAction` hook rather than a teardown in the entrypoint for + * two structural reasons: commander answers `--help` and `--version` during + * parsing, before any action hook runs, so the two most latency-sensitive + * invocations are excluded by construction rather than by a check; and some + * commands call `process.exit` directly, which a `finally` would never see. + * + * Never throws. The caller is a hook in front of the user's actual command. + */ +export async function announceUpdateIfAvailable(options: UpdateCheckOptions = {}): Promise { + try { + if (announced) return + + const env = options.env ?? process.env + const isTty = options.isTty ?? process.stderr.isTTY === true + const modulePath = options.modulePath ?? fileURLToPath(import.meta.url) + const now = options.now ?? new Date() + + if (isEnabled(env.SIM_NO_UPDATE_CHECK)) return + // stderr is where this goes, so a redirected stderr means it would land in a + // log file or a pipeline rather than in front of a person. + if (!isTty) return + if (CI_VARIABLES.some((variable) => isEnabled(env[variable]))) return + if (isUnadvisableInstall(modulePath)) return + + const currentVersion = options.currentVersion ?? CLI_VERSION + const current = parseVersion(currentVersion) + if (!current) return + + const channel = channelOf(current) + // Prereleases publish on every push to their branch, so telling a prerelease + // user to upgrade would be both correct and useless — the advice is stale + // again within the hour, and they opted into moving fast in the first place. + if (channel !== 'latest') return + + const cachePath = updateCachePath() + const cached = readCache(cachePath) + if (cached && isFresh(cached, now)) return + + const tags = await fetchDistTags(env) + const latest = tags?.[channel] ?? null + writeCache(cachePath, { + version: CACHE_VERSION, + checkedAt: now.toISOString(), + latestVersion: latest, + }) + if (!latest) return + + const available = parseVersion(latest) + if (!available || compareVersions(available, current) <= 0) return + + announced = true + const write = options.write ?? ((message: string) => void process.stderr.write(message)) + // Unstyled on purpose: chalk decides on stdout, so `sim workflows list | jq` + // from a terminal would silently drop the colour here even though stderr is + // still a terminal. Plain text is also the whole answer to NO_COLOR. + write( + `Update available: sim ${currentVersion} → ${latest}. Run: ${upgradeCommand(channel, modulePath, env)}\n` + ) + } catch { + // Nothing this function does is worth failing a command over. + } +} diff --git a/packages/sim-cli/src/update/semver.test.ts b/packages/sim-cli/src/update/semver.test.ts new file mode 100644 index 00000000000..7993ce160f0 --- /dev/null +++ b/packages/sim-cli/src/update/semver.test.ts @@ -0,0 +1,94 @@ +/** + * @vitest-environment node + */ +import { describe, expect, it } from 'vitest' +import { channelOf, compareVersions, parseVersion } from './semver' + +function parsed(version: string) { + const result = parseVersion(version) + if (!result) throw new Error(`fixture "${version}" should parse`) + return result +} + +function order(left: string, right: string): number { + return Math.sign(compareVersions(parsed(left), parsed(right))) +} + +describe('parsing a published version', () => { + it('reads the release triple', () => { + expect(parseVersion('2.1.5')).toEqual({ major: 2, minor: 1, patch: 5, prerelease: [] }) + }) + + it('splits a prerelease into identifiers, keeping numeric ones numeric', () => { + expect(parseVersion('2.1.3-preview.812.1')?.prerelease).toEqual(['preview', 812, 1]) + }) + + it('ignores build metadata, which carries no precedence', () => { + expect(parseVersion('2.1.5+20260902')).toEqual(parseVersion('2.1.5')) + }) + + it.each([ + ['2.1', 'an incomplete triple'], + ['v2.1.2', 'a leading v, which npm does not publish'], + ['2.1.2.3', 'a fourth component'], + ['01.2.3', 'a leading zero'], + ['2.1.2-', 'an empty prerelease'], + ['2.1.2-preview..1', 'an empty identifier'], + ['', 'nothing at all'], + ['latest', 'a dist-tag mistaken for a version'], + ])('rejects %s (%s)', (version) => { + expect(parseVersion(version)).toBeNull() + }) + + it('rejects a component too large to compare exactly', () => { + expect(parseVersion('9007199254740993.0.0')).toBeNull() + }) +}) + +describe('precedence', () => { + it('orders the release triple before anything else', () => { + expect(order('2.1.2', '2.1.3')).toBe(-1) + expect(order('2.1.9', '2.2.0')).toBe(-1) + expect(order('2.9.9', '3.0.0')).toBe(-1) + expect(order('2.1.5', '2.1.5')).toBe(0) + }) + + it('ranks a prerelease below the release it leads to', () => { + expect(order('2.1.3-preview.44.1', '2.1.3')).toBe(-1) + }) + + it('compares numeric identifiers as numbers, not as text', () => { + // The case a string comparison gets backwards: run 9 precedes run 44, but + // "44" sorts before "9" lexicographically. + expect(order('2.1.3-preview.9.1', '2.1.3-preview.44.1')).toBe(-1) + }) + + it('ranks a numeric identifier below an alphanumeric one', () => { + expect(order('2.1.3-1', '2.1.3-alpha')).toBe(-1) + }) + + it('ranks a shorter identifier list below a longer one sharing its prefix', () => { + expect(order('2.1.3-preview.1', '2.1.3-preview.1.2')).toBe(-1) + }) + + it('never advises a stable version that is older than an installed prerelease', () => { + // Comparison is channel-scoped precisely so this arrangement cannot reach a + // user as "upgrade": 2.1.2 is genuinely older than 2.1.3-preview.44.1. + expect(order('2.1.2', '2.1.3-preview.44.1')).toBe(-1) + }) +}) + +describe('the channel a version was published under', () => { + it('reads a stable release as the latest tag', () => { + expect(channelOf(parsed('2.1.5'))).toBe('latest') + }) + + it('reads the two prerelease tags the publish workflow produces', () => { + expect(channelOf(parsed('2.1.6-preview.812.1'))).toBe('staging') + expect(channelOf(parsed('2.1.6-dev.812.1'))).toBe('dev') + }) + + it('refuses to guess a channel for a prerelease tag we do not publish', () => { + expect(channelOf(parsed('2.1.6-rc.1'))).toBeNull() + }) +}) diff --git a/packages/sim-cli/src/update/semver.ts b/packages/sim-cli/src/update/semver.ts new file mode 100644 index 00000000000..39d92403fb0 --- /dev/null +++ b/packages/sim-cli/src/update/semver.ts @@ -0,0 +1,133 @@ +/** + * The version arithmetic the update check needs, and nothing more. + * + * The package deliberately carries no `semver` dependency: everything here is + * bundled into `dist/index.js`, and a full implementation would be several + * hundred kilobytes to answer one question once a day. What is implemented is + * the precedence half of the specification — enough to order two published + * versions — not ranges, not coercion, not satisfaction. + */ + +/** + * `X.Y.Z`, an optional prerelease, an optional build. Leading zeroes are + * rejected the way the specification rejects them, so a hand-edited `01.2.3` + * reads as unparseable rather than as `1.2.3`. + */ +const VERSION_PATTERN = + /^(0|[1-9]\d*)\.(0|[1-9]\d*)\.(0|[1-9]\d*)(?:-([0-9A-Za-z-]+(?:\.[0-9A-Za-z-]+)*))?(?:\+[0-9A-Za-z-]+(?:\.[0-9A-Za-z-]+)*)?$/ + +/** A prerelease identifier that is all digits compares as a number. */ +const NUMERIC_IDENTIFIER = /^(0|[1-9]\d*)$/ + +export interface ParsedVersion { + major: number + minor: number + patch: number + /** Dot-separated prerelease identifiers, empty for a stable release. */ + prerelease: readonly (string | number)[] +} + +/** + * The three dist-tags `.github/workflows/publish-sim-cli.yml` publishes under. + * + * A channel is inferred from the version's own prerelease tag rather than + * remembered, because the running CLI knows its version and nothing else about + * how it was installed. + */ +export type ReleaseChannel = 'latest' | 'staging' | 'dev' + +/** + * Parses a published version, or returns null for anything else. + * + * Never throws: every caller is on a path that must stay silent, and a local + * build with a hand-mangled manifest is a normal thing to encounter rather than + * an error to report. + */ +export function parseVersion(version: string): ParsedVersion | null { + const match = VERSION_PATTERN.exec(version) + if (!match) return null + + const major = Number(match[1]) + const minor = Number(match[2]) + const patch = Number(match[3]) + if ( + !Number.isSafeInteger(major) || + !Number.isSafeInteger(minor) || + !Number.isSafeInteger(patch) + ) { + return null + } + + const prerelease = match[4] + ? match[4] + .split('.') + .map((identifier) => + NUMERIC_IDENTIFIER.test(identifier) ? Number(identifier) : identifier + ) + : [] + if ( + prerelease.some( + (identifier) => typeof identifier === 'number' && !Number.isSafeInteger(identifier) + ) + ) { + return null + } + + return { major, minor, patch, prerelease } +} + +/** + * Compares two prerelease identifier lists by the specification's rules. + * + * Numeric identifiers compare numerically and sort below alphanumeric ones, and + * a shorter list sorts below a longer one that shares its prefix. The numeric + * rule is the one a string comparison gets wrong: `preview.9` is *older* than + * `preview.44`, which run number ordering depends on. + */ +function comparePrerelease( + left: readonly (string | number)[], + right: readonly (string | number)[] +): number { + // A version carrying a prerelease ranks below the same version without one. + if (left.length === 0) return right.length === 0 ? 0 : 1 + if (right.length === 0) return -1 + + for (let index = 0; index < Math.min(left.length, right.length); index += 1) { + const a = left[index] + const b = right[index] + if (a === b) continue + if (typeof a === 'number' && typeof b === 'number') return a - b + if (typeof a === 'number') return -1 + if (typeof b === 'number') return 1 + return a < b ? -1 : 1 + } + return left.length - right.length +} + +/** + * Semver precedence: negative when left is older, zero when equal, positive + * when left is newer. Build metadata is ignored, as the specification requires. + */ +export function compareVersions(left: ParsedVersion, right: ParsedVersion): number { + if (left.major !== right.major) return left.major - right.major + if (left.minor !== right.minor) return left.minor - right.minor + if (left.patch !== right.patch) return left.patch - right.patch + return comparePrerelease(left.prerelease, right.prerelease) +} + +/** + * The dist-tag a version was published under, or null when its prerelease tag + * is not one this project publishes. + * + * Knowing the channel is what keeps the comparison honest: a `-preview` install + * is only ever compared against the `staging` tag, so there is no arrangement + * of inputs that can advise "upgrade" to a stable version that is actually + * older than what is already installed. + */ +export function channelOf(version: ParsedVersion): ReleaseChannel | null { + if (version.prerelease.length === 0) return 'latest' + const [tag] = version.prerelease + if (tag === 'preview') return 'staging' + if (tag === 'dev') return 'dev' + return null +} From 206d3256c51289ff3bfc6551037b2e4eaf7cd6a9 Mon Sep 17 00:00:00 2001 From: Marcus Chandra Date: Wed, 2 Sep 2026 15:04:26 -0700 Subject: [PATCH 2/8] fix(cli): close the update-notifier findings from pre-landing review Mutation testing found three tests that could not fail: deleting the `preAction` hook, switching the default writer to stdout, and flipping `comparePrerelease`'s empty-list arm all left the suite green. The stdout one was vacuous because the test helper always injected a writer, so the single safety property this feature claims - never touch stdout - was unprotected. The hook now has a positive test. It asserts registration rather than a resulting request, because the check suppresses itself when running from a checkout, and inside the suite `import.meta.url` IS a checkout: the behavioural path is unreachable there by construction. It is covered directly in check.test.ts and walked against the real registry from a staged global install. Security review: the response body is now read under a 64KB budget instead of buffering whatever a mirror sends, the request refuses to follow redirects, and the registry's answer is parsed before it is persisted, so nothing unvalidated reaches the disk. The reduced User-Agent was a comment; it is now an assertion, so a future "DRY up the user agent" refactor cannot silently start handing npm the user's node version, platform and arch. A configured mirror's own path and query are preserved. `new URL(relative, base)` discards both, so a token-authenticated Artifactory or Nexus base was being rewritten into a request the mirror answers with a 404. Also: one normalisation for every module-path decision (separators AND case, so a Windows or case-insensitive checkout is not read as a global install by one guard and a checkout by the other), the package name is named once rather than spelled in two unrelated places, and `delete process.env.SIM_CONFIG_DIR` in teardown - assigning `undefined` stores the literal string and leaves later tests pointed at a relative `./undefined` directory. Tests: 843 -> 861. Ten mutations applied to verify the new assertions actually fail when the thing they guard is broken; all ten killed. Declined, with reasons: the ~10s lingering-socket exit delay could not be reproduced through the CLI (measured 1.11-1.38s across three runs on node v23.11.0, including a command that only sets exitCode), so no node:https rewrite. `announced` plus `resetUpdateCheck` stays - it is the same shape as the existing resetEnvironmentNotices and resetRenameWarnings seams. The channel type stays rather than collapsing to a boolean, because it is what a decision to notify prerelease users would extend; its docs now say what the code does instead of describing a comparison it never performs. --- apps/docs/content/docs/cli/configuration.mdx | 19 ++- packages/sim-cli/README.md | 4 +- packages/sim-cli/src/program.test.ts | 26 ++++ packages/sim-cli/src/update/check.test.ts | 137 +++++++++++++++---- packages/sim-cli/src/update/check.ts | 132 +++++++++++++++--- packages/sim-cli/src/update/semver.test.ts | 20 ++- packages/sim-cli/src/update/semver.ts | 11 +- 7 files changed, 290 insertions(+), 59 deletions(-) diff --git a/apps/docs/content/docs/cli/configuration.mdx b/apps/docs/content/docs/cli/configuration.mdx index 0d0f402141f..799520b734c 100644 --- a/apps/docs/content/docs/cli/configuration.mdx +++ b/apps/docs/content/docs/cli/configuration.mdx @@ -133,10 +133,21 @@ Update available: sim 2.1.2 → 2.1.5. Run: npm install -g sim@latest ``` The request carries the CLI version and nothing else — no API key, no workspace, -no command. It is skipped entirely when stderr is redirected, in CI, under -`npx`, and for prerelease installs, so scripted output is never affected. Set -`SIM_NO_UPDATE_CHECK=1` to turn it off, and `npm_config_registry` to ask a -mirror instead. +no command — and it never follows a redirect away from the registry you asked. + +The notice is skipped entirely when: + +- `SIM_NO_UPDATE_CHECK` is set to anything but `0` or `false` +- stderr is not a terminal, so redirected and piped output is never affected +- a CI environment variable is present (`CI`, `GITHUB_ACTIONS`, `JENKINS_URL`, + `TEAMCITY_VERSION`, `BUILDKITE`) +- the CLI is running under `npx`, which resolves the newest version every time +- the CLI is running from a checkout of the sim repository, whose version + deliberately trails the published one +- the installed version is a prerelease from the `staging` or `dev` channel + +Set `npm_config_registry` to ask a mirror instead; its path and query are +preserved, so a token-authenticated Artifactory or Nexus base works. Node ignores `HTTPS_PROXY` unless you also set `NODE_USE_ENV_PROXY=1`, and only from Node 22.21 and 24.5. The CLI warns when a proxy is configured but will not diff --git a/packages/sim-cli/README.md b/packages/sim-cli/README.md index a7e5687fa1c..60f1bd59f71 100644 --- a/packages/sim-cli/README.md +++ b/packages/sim-cli/README.md @@ -264,8 +264,8 @@ The main environment variables are: Once a day, at an interactive terminal, `sim` asks `registry.npmjs.org` which version is published under the tag it was installed from, and prints one line on stderr when a newer one exists. It sends nothing but its own version, never a -key, and stays quiet when stderr is not a terminal, in CI, and under `npx`. Set -`SIM_NO_UPDATE_CHECK=1` to turn it off. +key. Set `SIM_NO_UPDATE_CHECK=1` to turn it off; the full list of cases where it +stays quiet is in the [configuration guide](https://docs.sim.ai/cli/configuration). ## Documentation diff --git a/packages/sim-cli/src/program.test.ts b/packages/sim-cli/src/program.test.ts index 2d8870980f5..753ecbf2be9 100644 --- a/packages/sim-cli/src/program.test.ts +++ b/packages/sim-cli/src/program.test.ts @@ -193,4 +193,30 @@ describe('the update check', () => { rmSync(dir, { recursive: true, force: true }) } }) + + /** + * The positive half, and the one that matters: without it the hook can be + * deleted from `buildProgram` and every other test still passes. + * + * It asserts registration rather than a resulting request, because the check + * suppresses itself when it is running from a checkout — and inside this + * suite `import.meta.url` IS a checkout, so the behavioural path is + * unreachable here by construction. That path is covered directly in + * check.test.ts and walked against the real registry from a staged global + * install before release. + */ + it('registers the update check as a root preAction hook', async () => { + const program = buildProgram() + // Commander keeps lifecycle hooks on a private field and offers no getter, + // the same way `rawArgs` is read elsewhere in this file. + const { _lifeCycleHooks: hooks } = program as Command & { + _lifeCycleHooks?: Record unknown>> + } + const preAction = hooks?.preAction ?? [] + + expect(preAction).toHaveLength(1) + // Invoking it must resolve, never throw: it runs in front of the user's + // command, and a rejection here would fail the command itself. + await expect(preAction[0](program, program)).resolves.toBeUndefined() + }) }) diff --git a/packages/sim-cli/src/update/check.test.ts b/packages/sim-cli/src/update/check.test.ts index 039eafd2362..cfdda5b6331 100644 --- a/packages/sim-cli/src/update/check.test.ts +++ b/packages/sim-cli/src/update/check.test.ts @@ -5,6 +5,7 @@ import { mkdtempSync, readFileSync, rmSync, statSync, writeFileSync } from 'node import { tmpdir } from 'node:os' import { join } from 'node:path' import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import { CLI_VERSION } from '../version' import { announceUpdateIfAvailable, resetUpdateCheck, upgradeCommand } from './check' /** A global install, which is the only shape that gets advised at all. */ @@ -13,11 +14,20 @@ const INSTALLED = '/usr/local/lib/node_modules/sim/dist/index.js' let configDir: string let notices: string[] let fetched: URL[] +let inits: RequestInit[] /** Answers the dist-tags request the way the registry does. */ -function stubRegistry(tags: Record | 'reject' | 'not-found' | 'html'): void { - vi.stubGlobal('fetch', (input: URL) => { +function stubRegistry( + tags: Record | 'reject' | 'not-found' | 'html' | 'oversized' +): void { + vi.stubGlobal('fetch', (input: URL, init: RequestInit) => { fetched.push(input) + inits.push(init) + if (tags === 'oversized') { + return Promise.resolve( + Response.json({ latest: '2.1.5' }, { headers: { 'content-length': String(1024 * 1024) } }) + ) + } if (tags === 'reject') return Promise.reject(new Error('getaddrinfo ENOTFOUND')) if (tags === 'not-found') return Promise.resolve(new Response('', { status: 404 })) if (tags === 'html') return Promise.resolve(new Response('nope', { status: 200 })) @@ -46,11 +56,14 @@ beforeEach(() => { process.env.SIM_CONFIG_DIR = configDir notices = [] fetched = [] + inits = [] stubRegistry({ latest: '2.1.5' }) }) afterEach(() => { vi.unstubAllGlobals() + // `= undefined` would store the literal string "undefined", leaving later + // tests pointed at a relative `./undefined` config directory. process.env.SIM_CONFIG_DIR = undefined rmSync(configDir, { recursive: true, force: true }) }) @@ -78,20 +91,49 @@ describe('announcing a newer release', () => { expect(notices).toEqual([]) }) - it('writes nothing to stdout, which may be a pipeline', async () => { - const originalWrite = process.stdout.write - const stdout: string[] = [] + it('writes through the real default: stderr yes, stdout never', async () => { + // Deliberately without the `write` override, so the production default is + // the thing under test. stdout may be a pipeline feeding jq. + const realOut = process.stdout.write + const realErr = process.stderr.write + const seen = { out: [] as string[], err: [] as string[] } process.stdout.write = ((chunk: string) => { - stdout.push(String(chunk)) + seen.out.push(String(chunk)) return true }) as typeof process.stdout.write + process.stderr.write = ((chunk: string) => { + seen.err.push(String(chunk)) + return true + }) as typeof process.stderr.write try { - await run() + await announceUpdateIfAvailable({ + currentVersion: '2.1.2', + env: {}, + isTty: true, + modulePath: INSTALLED, + }) } finally { - process.stdout.write = originalWrite + process.stdout.write = realOut + process.stderr.write = realErr } - expect(notices).toHaveLength(1) - expect(stdout).toEqual([]) + expect(seen.out).toEqual([]) + expect(seen.err.join('')).toContain('Update available: sim 2.1.2 → 2.1.5') + }) + + it('sends only its own version, and refuses to follow a redirect', async () => { + await run() + const headers = inits[0]?.headers as Record + // The reduced agent is the privacy property: the exported USER_AGENT in + // version.ts also carries node version, platform and arch. + expect(headers['user-agent']).toBe(`sim-cli/${CLI_VERSION}`) + expect(headers.accept).toBe('application/json') + expect(headers.authorization).toBeUndefined() + expect(inits[0]?.redirect).toBe('error') + }) + + it('bounds the request so a hung registry cannot stall the command', async () => { + await run() + expect(inits[0]?.signal).toBeInstanceOf(AbortSignal) }) }) @@ -113,20 +155,36 @@ describe('when the notice is suppressed', () => { expect(notices).toEqual([]) }) - it('says nothing in CI, even where CI allocates a terminal', async () => { - await run({ env: { BUILDKITE: 'true' } }) - expect(notices).toEqual([]) - }) + it.each(['CI', 'GITHUB_ACTIONS', 'JENKINS_URL', 'TEAMCITY_VERSION', 'BUILDKITE'])( + 'says nothing when %s is set, even where CI allocates a terminal', + async (variable) => { + await run({ env: { [variable]: 'true' } }) + expect(fetched).toEqual([]) + expect(notices).toEqual([]) + } + ) - it('says nothing under npx, which resolves the tag on every run', async () => { - await run({ modulePath: '/Users/x/.npm/_npx/a1b2/node_modules/sim/dist/index.js' }) + it.each([ + '/Users/x/.npm/_npx/a1b2/node_modules/sim/dist/index.js', + 'C:\\Users\\x\\AppData\\Local\\npm-cache\\_npx\\a1b2\\node_modules\\sim\\dist\\index.js', + ])('says nothing under npx, which resolves the tag on every run (%s)', async (modulePath) => { + await run({ modulePath }) expect(notices).toEqual([]) }) - it('says nothing from a checkout, whose manifest trails npm by design', async () => { - await run({ modulePath: '/Users/x/sim/packages/sim-cli/dist/index.js' }) - expect(notices).toEqual([]) - }) + it.each([ + '/Users/x/sim/packages/sim-cli/dist/index.js', + // Windows, and mixed case: a checkout is a checkout on a case-insensitive + // volume too, and this is the guard that stops every Sim engineer being + // nagged daily by their own build. + 'C:\\Users\\x\\Sim\\Packages\\Sim-CLI\\dist\\index.js', + ])( + 'says nothing from a checkout, whose manifest trails npm by design (%s)', + async (modulePath) => { + await run({ modulePath }) + expect(notices).toEqual([]) + } + ) it('says nothing to a prerelease install', async () => { stubRegistry({ latest: '2.1.5', staging: '2.1.6-preview.812.1' }) @@ -157,8 +215,10 @@ describe('the once-a-day cache', () => { checkedAt: '2026-09-02T10:00:00.000Z', latestVersion: '2.1.5', }) - // Not secret, but not world-writable either. - expect(statSync(cachePath()).mode & 0o777).toBe(0o644) + // Assert the property, not the literal mode: writeFileSync's mode is + // masked by the ambient umask, so an exact comparison fails under + // `umask 077` for reasons that have nothing to do with this code. + expect(statSync(cachePath()).mode & 0o022).toBe(0) }) it('does not contact the registry again within the day', async () => { @@ -221,6 +281,22 @@ describe('when the registry does not answer', () => { expect(notices).toEqual([]) }) + it.each([ + ['an empty object', {} as Record], + ['a non-string tag value', { latest: 42 } as Record], + ['a nested object where a version belongs', { latest: { version: '9.9.9' } }], + ])('stays silent when the payload carries %s', async (_label, payload) => { + stubRegistry(payload) + await run() + expect(notices).toEqual([]) + }) + + it('refuses a body far larger than this endpoint could legitimately return', async () => { + stubRegistry('oversized') + await run() + expect(notices).toEqual([]) + }) + it('stays silent when the tag is missing or is not a version', async () => { stubRegistry({ staging: '2.1.6-preview.1.1' }) await run() @@ -244,10 +320,23 @@ describe('when the registry does not answer', () => { expect(fetched.map(String)).toEqual(['https://npm.internal/api/npm/-/package/sim/dist-tags']) }) - it('ignores a mirror setting that is not an http url', async () => { - await run({ env: { npm_config_registry: 'not a url' } }) + it.each([ + ['a value that is not a url', 'not a url'], + ['a non-http protocol', 'file:///var/tmp/registry'], + ['whitespace', ' '], + ])('falls back to the default registry for %s', async (_label, configured) => { + await run({ env: { npm_config_registry: configured } }) expect(fetched.map(String)).toEqual(['https://registry.npmjs.org/-/package/sim/dist-tags']) }) + + it("keeps a token-authenticated mirror's own path and query", async () => { + // Artifactory and Nexus bases carry both. Resolving the path as a relative + // URL would drop them and ask the mirror a question it answers with a 404. + await run({ env: { npm_config_registry: 'https://npm.internal/api/npm/repo?token=abc' } }) + expect(fetched.map(String)).toEqual([ + 'https://npm.internal/api/npm/repo/-/package/sim/dist-tags?token=abc', + ]) + }) }) describe('the upgrade command', () => { diff --git a/packages/sim-cli/src/update/check.ts b/packages/sim-cli/src/update/check.ts index 8cb5c4d07f3..ce549135d92 100644 --- a/packages/sim-cli/src/update/check.ts +++ b/packages/sim-cli/src/update/check.ts @@ -28,6 +28,25 @@ const REGISTRY_TIMEOUT_MS = 1000 const DEFAULT_REGISTRY = 'https://registry.npmjs.org' +/** + * The published package. Named once because it appears in two unrelated places + * — the registry path and the upgrade command — and a rename that updated only + * one would leave the CLI asking about one package while advising another. + */ +const PACKAGE_NAME = 'sim' + +/** Relative to the registry root, and about a hundred bytes of response. */ +const DIST_TAGS_PATH = `-/package/${PACKAGE_NAME}/dist-tags` + +/** + * A ceiling on the response body. The endpoint answers in ~100 bytes, so this + * is three orders of magnitude of headroom; it exists because the host is + * partly environment-controlled through `npm_config_registry`, and an + * unbounded read from a mirror on a fast link can buffer arbitrarily much + * before the timeout fires. + */ +const MAX_RESPONSE_BYTES = 64 * 1024 + /** * Set by every CI provider worth naming. `!isTTY` already covers most of them; * this catches the ones that allocate a terminal anyway, such as a Buildkite @@ -50,6 +69,14 @@ interface UpdateCacheEntry { */ version: 1 checkedAt: string + /** + * What the last check found, recorded so `cat ~/.sim/update-check.json` + * answers a support question without re-running anything. + * + * Deliberately NOT served: announcing from the cache would put the notice in + * front of every command for the rest of the day, and the contract is one + * notice per day. Only `checkedAt` decides whether the check runs. + */ latestVersion: string | null } @@ -95,31 +122,83 @@ function isEnabled(value: string | undefined): boolean { * to a version their own tree already contains. */ function isUnadvisableInstall(modulePath: string): boolean { - const normalized = modulePath.replace(/\\/g, '/') + const normalized = normalizeModulePath(modulePath) return normalized.includes('/_npx/') || normalized.includes('/packages/sim-cli/') } /** - * The registry to ask. `npm_config_registry` is only set when the CLI is invoked - * through a package-manager script, so this is inconsistent by nature — but the - * case it rescues is real: behind a mirror with `registry.npmjs.org` firewalled, - * the default would fail forever while the mirror holds the right answer. + * One normalisation for every decision made about the module path. + * + * Both readers here match path fragments, and they must agree: normalising + * separators in one and separators-plus-case in the other silently disagrees on + * Windows and on case-insensitive macOS volumes, where a checkout under + * `...\\Packages\\Sim-Cli\\` is a checkout to one reader and a global install to + * the other. + */ +function normalizeModulePath(modulePath: string): string { + return modulePath.replace(/\\/g, '/').toLowerCase() +} + +/** + * The full dist-tags URL, honouring a configured mirror. + * + * `npm_config_registry` is only set when the CLI is invoked through a + * package-manager script, so this is inconsistent by nature — but the case it + * rescues is real: behind a mirror with `registry.npmjs.org` firewalled, the + * default would fail forever while the mirror holds the right answer. + * + * The mirror's own path and query are preserved rather than resolved away. + * `new URL(relative, base)` would discard both, and a token-authenticated + * Artifactory or Nexus base (`https://host/api/npm/repo?token=...`) is a + * realistic configuration that would otherwise be silently rewritten into a + * request the mirror answers with a 404. * * `.npmrc` is deliberately not parsed. That is an INI format with scopes and * auth tokens, and reading it is where a courtesy check would start growing. */ -function registryBase(env: NodeJS.ProcessEnv): string { +function registryUrl(env: NodeJS.ProcessEnv): URL { + const fallback = new URL(DIST_TAGS_PATH, DEFAULT_REGISTRY) const configured = env.npm_config_registry?.trim() - if (!configured) return DEFAULT_REGISTRY + if (!configured) return fallback try { - const url = new URL(configured) - if (url.protocol === 'http:' || url.protocol === 'https:') { - return configured.endsWith('/') ? configured : `${configured}/` - } + const base = new URL(configured) + if (base.protocol !== 'http:' && base.protocol !== 'https:') return fallback + base.pathname = `${base.pathname.replace(/\/$/, '')}/${DIST_TAGS_PATH}` + return base } catch { // An unparseable value is not worth reporting; the default still works. + return fallback + } +} + +/** + * Reads a response body under a hard byte budget. + * + * `response.json()` would buffer whatever arrives, bounded only by the abort + * timeout — long enough for a mirror on a fast link to push far more than this + * endpoint could legitimately return. + */ +async function readCapped(response: Response, limit: number): Promise { + const declared = Number(response.headers.get('content-length')) + if (Number.isFinite(declared) && declared > limit) return null + if (!response.body) return null + + const reader = response.body.getReader() + const decoder = new TextDecoder() + let text = '' + let seen = 0 + try { + while (true) { + const { done, value } = await reader.read() + if (done) break + seen += value.byteLength + if (seen > limit) return null + text += decoder.decode(value, { stream: true }) + } + } finally { + void reader.cancel().catch(() => {}) } - return DEFAULT_REGISTRY + return text + decoder.decode() } /** @@ -135,12 +214,17 @@ function registryBase(env: NodeJS.ProcessEnv): string { */ async function fetchDistTags(env: NodeJS.ProcessEnv): Promise | null> { try { - const response = await fetch(new URL('-/package/sim/dist-tags', registryBase(env)), { - headers: { accept: 'application/json', 'user-agent': `sim-cli/${CLI_VERSION}` }, + const response = await fetch(registryUrl(env), { + headers: { accept: 'application/json', 'user-agent': `${PACKAGE_NAME}-cli/${CLI_VERSION}` }, + // The endpoint does not redirect in normal operation, so refusing to + // follow costs nothing and keeps the request on the host that was asked. + redirect: 'error', signal: AbortSignal.timeout(REGISTRY_TIMEOUT_MS), }) if (!response.ok) return null - const body: unknown = await response.json() + const text = await readCapped(response, MAX_RESPONSE_BYTES) + if (text === null) return null + const body: unknown = JSON.parse(text) if (typeof body !== 'object' || body === null || Array.isArray(body)) return null // Some proxies answer with an HTML error page under a 200, so the values are // checked rather than assumed. @@ -190,7 +274,9 @@ function writeCache(path: string, entry: UpdateCacheEntry): void { writeFileSync(path, `${JSON.stringify(entry, null, 2)}\n`, { mode: 0o644 }) } catch { // A read-only home directory is ordinary in a container, and it must not - // stop the command the user actually ran. + // stop the command the user actually ran. The cost is that the throttle + // cannot persist there, so such an installation re-checks once per + // invocation instead of once per day — still bounded by the timeout. } } @@ -215,8 +301,8 @@ export function upgradeCommand( modulePath: string = fileURLToPath(import.meta.url), env: NodeJS.ProcessEnv = process.env ): string { - const target = `sim@${channel}` - const normalized = modulePath.replace(/\\/g, '/').toLowerCase() + const target = `${PACKAGE_NAME}@${channel}` + const normalized = normalizeModulePath(modulePath) if (normalized.includes('.bun/install/global')) return `bun add -g ${target}` if (normalized.includes('/pnpm/') || normalized.includes('/.pnpm/')) { @@ -277,15 +363,17 @@ export async function announceUpdateIfAvailable(options: UpdateCheckOptions = {} const tags = await fetchDistTags(env) const latest = tags?.[channel] ?? null + // Parse before persisting: the value came off the network, and nothing + // unvalidated should reach the disk or, later, the terminal. + const available = latest ? parseVersion(latest) : null writeCache(cachePath, { version: CACHE_VERSION, checkedAt: now.toISOString(), - latestVersion: latest, + latestVersion: available ? latest : null, }) - if (!latest) return + if (!latest || !available) return - const available = parseVersion(latest) - if (!available || compareVersions(available, current) <= 0) return + if (compareVersions(available, current) <= 0) return announced = true const write = options.write ?? ((message: string) => void process.stderr.write(message)) diff --git a/packages/sim-cli/src/update/semver.test.ts b/packages/sim-cli/src/update/semver.test.ts index 7993ce160f0..6c333557fd0 100644 --- a/packages/sim-cli/src/update/semver.test.ts +++ b/packages/sim-cli/src/update/semver.test.ts @@ -57,6 +57,17 @@ describe('precedence', () => { expect(order('2.1.3-preview.44.1', '2.1.3')).toBe(-1) }) + it('ranks a release above a prerelease of the same triple', () => { + // The mirror of the case above, and a distinct branch: it is the only way + // to reach the comparison with an empty prerelease list on the left. + expect(order('2.1.3', '2.1.3-preview.44.1')).toBe(1) + }) + + it('orders two alphanumeric identifiers by ASCII', () => { + expect(order('2.1.3-alpha', '2.1.3-beta')).toBe(-1) + expect(order('2.1.3-beta', '2.1.3-alpha')).toBe(1) + }) + it('compares numeric identifiers as numbers, not as text', () => { // The case a string comparison gets backwards: run 9 precedes run 44, but // "44" sorts before "9" lexicographically. @@ -65,15 +76,18 @@ describe('precedence', () => { it('ranks a numeric identifier below an alphanumeric one', () => { expect(order('2.1.3-1', '2.1.3-alpha')).toBe(-1) + expect(order('2.1.3-alpha', '2.1.3-1')).toBe(1) }) it('ranks a shorter identifier list below a longer one sharing its prefix', () => { expect(order('2.1.3-preview.1', '2.1.3-preview.1.2')).toBe(-1) }) - it('never advises a stable version that is older than an installed prerelease', () => { - // Comparison is channel-scoped precisely so this arrangement cannot reach a - // user as "upgrade": 2.1.2 is genuinely older than 2.1.3-preview.44.1. + it('ranks a stable release below a prerelease of a later patch', () => { + // 2.1.2 really is older than 2.1.3-preview.44.1. The guarantee that this + // never reaches a user as "upgrade" lives in check.ts, which returns before + // comparing anything on a non-stable channel — see check.test.ts's "says + // nothing to a prerelease install". expect(order('2.1.2', '2.1.3-preview.44.1')).toBe(-1) }) }) diff --git a/packages/sim-cli/src/update/semver.ts b/packages/sim-cli/src/update/semver.ts index 39d92403fb0..df89273a11a 100644 --- a/packages/sim-cli/src/update/semver.ts +++ b/packages/sim-cli/src/update/semver.ts @@ -119,10 +119,13 @@ export function compareVersions(left: ParsedVersion, right: ParsedVersion): numb * The dist-tag a version was published under, or null when its prerelease tag * is not one this project publishes. * - * Knowing the channel is what keeps the comparison honest: a `-preview` install - * is only ever compared against the `staging` tag, so there is no arrangement - * of inputs that can advise "upgrade" to a stable version that is actually - * older than what is already installed. + * The caller compares a version only against its own channel's tag. Today it + * acts on `latest` alone and returns early for everything else, so a `-preview` + * install is compared against nothing at all — which is what makes it + * impossible to advise "upgrade" to a stable version older than the prerelease + * already installed. Naming the channel rather than answering a bare + * is-this-stable keeps that guarantee legible, and is what a future decision to + * notify prerelease users would extend. */ export function channelOf(version: ParsedVersion): ReleaseChannel | null { if (version.prerelease.length === 0) return 'latest' From fd3f3db3529c95e37e59fd5af6110482962ecf15 Mon Sep 17 00:00:00 2001 From: Marcus Chandra Date: Wed, 2 Sep 2026 15:16:52 -0700 Subject: [PATCH 3/8] fix(cli): correct the update-notifier privacy claim and prerelease parsing Review round 1: five findings, all valid. The privacy statement was too absolute. The request carries no Sim API key, but `npm_config_registry` can point at a private mirror, and a token embedded in that URL is sent with the request - it has to be, or the mirror rejects it. Both docs now say which credentials are involved and where they go: your registry's, to the host you configured, never Sim's. `parseVersion` accepted zero-padded prerelease identifiers. Semver forbids them, and accepting `2.1.3-preview.09` was worse than cosmetic: `09` failed the numeric test and fell through to being an alphanumeric identifier, and alphanumerics outrank every number, so `preview.010` sorted ABOVE `preview.2`. The file's own doc comment already claimed leading zeroes were rejected "the way the specification rejects them" - true of the release triple, not of the prerelease. Now true of both. The `--version`/`--help` test did not hold the guarantee it advertised. It watched for a request and a cache file, but neither ever appears from inside a checkout no matter what runs, because the check suppresses itself there - so it would have passed even if the hook fired, which is the exact regression it claims to prevent. It now swaps a sentinel into commander's registered preAction hooks and asserts the sentinel does not fire while parsing those two, then asserts it DOES fire for a real action command, so the negative assertion means something. No module mocking, which this package bans. The troubleshooting page hardcoded `npm install -g`, which installs a second copy under a different package manager rather than replacing the executable on PATH. It now shows all three, and says the notice already prints the one matching your install - which the notifier has always done. Tests: 861 -> 863. Both new guards mutation-checked: dropping the leading-zero rejection and deleting the hook each fail the suite. --- apps/docs/content/docs/cli/configuration.mdx | 17 ++++- .../docs/content/docs/cli/troubleshooting.mdx | 29 ++++++++- packages/sim-cli/README.md | 9 ++- packages/sim-cli/src/program.test.ts | 64 +++++++++++-------- packages/sim-cli/src/update/semver.test.ts | 9 +++ packages/sim-cli/src/update/semver.ts | 22 +++++-- 6 files changed, 109 insertions(+), 41 deletions(-) diff --git a/apps/docs/content/docs/cli/configuration.mdx b/apps/docs/content/docs/cli/configuration.mdx index 799520b734c..d4c90404790 100644 --- a/apps/docs/content/docs/cli/configuration.mdx +++ b/apps/docs/content/docs/cli/configuration.mdx @@ -132,8 +132,16 @@ both versions and the command that upgrades: Update available: sim 2.1.2 → 2.1.5. Run: npm install -g sim@latest ``` -The request carries the CLI version and nothing else — no API key, no workspace, -no command — and it never follows a redirect away from the registry you asked. +The request carries the CLI version and nothing else — no Sim API key, no +workspace, no command — and it never follows a redirect away from the registry +it asked. + +One caveat worth stating plainly: if you point `npm_config_registry` at a +private mirror, the check goes to that mirror instead of npm, and any +credentials embedded in that URL (an Artifactory or Nexus `?token=…`) are sent +with it — they have to be, or the mirror would reject the request. Those are +your registry's credentials, not Sim's, and they go only to the host you +configured. The notice is skipped entirely when: @@ -149,6 +157,11 @@ The notice is skipped entirely when: Set `npm_config_registry` to ask a mirror instead; its path and query are preserved, so a token-authenticated Artifactory or Nexus base works. +The command the notice prints matches how Sim was installed — `npm install -g`, +`pnpm add -g`, `bun add -g`, or `yarn global add` — so running it updates the +executable already on your `PATH` rather than installing a second copy under a +different package manager. + Node ignores `HTTPS_PROXY` unless you also set `NODE_USE_ENV_PROXY=1`, and only from Node 22.21 and 24.5. The CLI warns when a proxy is configured but will not be used. diff --git a/apps/docs/content/docs/cli/troubleshooting.mdx b/apps/docs/content/docs/cli/troubleshooting.mdx index 1bb584c1f04..3ae53b624cd 100644 --- a/apps/docs/content/docs/cli/troubleshooting.mdx +++ b/apps/docs/content/docs/cli/troubleshooting.mdx @@ -3,6 +3,8 @@ title: Troubleshooting description: The failures whose cause is not obvious from the error message --- +import { Tab, Tabs } from 'fumadocs-ui/components/tabs' + Errors print one line to stderr, prefixed `Error:`, and exit `1` — except `sim whoami`, which exits `2` when it could not reach the API to check at all. Most say what to do next; the cases below are the ones that do not. @@ -93,11 +95,32 @@ The docs track the current release, so a command that exists here and not in ```bash sim --version -npm install -g sim@latest ``` -The CLI normally tells you this itself, once a day, on stderr. It stays quiet -when stderr is redirected, in CI, and under `npx`. +Then upgrade with the package manager you installed it with — using a different +one installs a second copy instead of replacing the executable on your `PATH`: + + + + ```bash + npm install -g sim@latest + ``` + + + ```bash + pnpm add -g sim@latest + ``` + + + ```bash + bun add -g sim@latest + ``` + + + +The CLI normally tells you this itself, once a day, on stderr, and the command +it prints already matches your installation. It stays quiet when stderr is +redirected, in CI, and under `npx`. ## An update notice appears in output I am parsing diff --git a/packages/sim-cli/README.md b/packages/sim-cli/README.md index 60f1bd59f71..fe6d7be46ca 100644 --- a/packages/sim-cli/README.md +++ b/packages/sim-cli/README.md @@ -263,9 +263,12 @@ The main environment variables are: Once a day, at an interactive terminal, `sim` asks `registry.npmjs.org` which version is published under the tag it was installed from, and prints one line on -stderr when a newer one exists. It sends nothing but its own version, never a -key. Set `SIM_NO_UPDATE_CHECK=1` to turn it off; the full list of cases where it -stays quiet is in the [configuration guide](https://docs.sim.ai/cli/configuration). +stderr when a newer one exists. It sends nothing but its own version and never +your Sim API key. If `npm_config_registry` points at a private mirror, the check +goes there instead and carries whatever credentials that URL embeds, since the +mirror would otherwise refuse it. Set `SIM_NO_UPDATE_CHECK=1` to turn it off; +the full list of cases where it stays quiet is in the +[configuration guide](https://docs.sim.ai/cli/configuration). ## Documentation diff --git a/packages/sim-cli/src/program.test.ts b/packages/sim-cli/src/program.test.ts index 753ecbf2be9..e0f74da335c 100644 --- a/packages/sim-cli/src/program.test.ts +++ b/packages/sim-cli/src/program.test.ts @@ -1,17 +1,19 @@ /** * @vitest-environment node */ -import { mkdtempSync, readdirSync, rmSync } from 'node:fs' +import { mkdtempSync, rmSync } from 'node:fs' import { tmpdir } from 'node:os' import { join } from 'node:path' import type { Command } from 'commander' -import { describe, expect, it, vi } from 'vitest' +import { describe, expect, it } from 'vitest' import { buildProgram } from './program' import { CLI_VERSION } from './version' /** Parses argv against a program whose output and exits are captured, not taken. */ -async function parse(argv: string[]): Promise<{ out: string; code: string | null }> { - const program = buildProgram() +async function parse( + argv: string[], + program: Command = buildProgram() +): Promise<{ out: string; code: string | null }> { let out = '' const capture = (command: Command) => { command.exitOverride() @@ -163,33 +165,48 @@ describe('help typed after a command that does not exist', () => { }) }) +/** Commander keeps lifecycle hooks on a private field and offers no getter. */ +function preActionHooks(program: Command): Array<(a: Command, b: Command) => unknown> { + const { _lifeCycleHooks: hooks } = program as Command & { + _lifeCycleHooks?: Record unknown>> + } + return hooks?.preAction ?? [] +} + describe('the update check', () => { /** * The notice must cost `--version` and `--help` nothing. Commander answers * both during parsing, before any action hook runs, so the guarantee is * structural — this holds it in place if the check is ever moved. + * + * It swaps in a sentinel hook rather than watching for a request or a cache + * file. Those side effects never appear from inside a checkout no matter + * what runs, because the check suppresses itself there — so asserting on + * them would pass even if the hook fired, which is precisely the regression + * this is meant to catch. */ - it('never runs for the two commands commander answers during parsing', async () => { - const stderr = process.stderr - const wasTty = stderr.isTTY - const dir = mkdtempSync(join(tmpdir(), 'sim-cli-program-')) - const requests: string[] = [] - Object.defineProperty(stderr, 'isTTY', { configurable: true, value: true }) - process.env.SIM_CONFIG_DIR = dir - vi.stubGlobal('fetch', (input: URL) => { - requests.push(String(input)) - return Promise.resolve(Response.json({ latest: '99.0.0' })) + it('fires no preAction hook for the two commands commander answers while parsing', async () => { + let fired = 0 + const program = buildProgram() + const hooks = preActionHooks(program) + expect(hooks).toHaveLength(1) + hooks.splice(0, hooks.length, () => { + fired += 1 }) + await parse(['--version'], program) + await parse(['--help'], program) + expect(fired).toBe(0) + + // And the sentinel is not inert: the same hook does fire for a real action, + // which is what makes the assertion above mean something. + const dir = mkdtempSync(join(tmpdir(), 'sim-cli-program-')) + process.env.SIM_CONFIG_DIR = dir try { - await parse(['--version']) - await parse(['--help']) - expect(requests).toEqual([]) - expect(readdirSync(dir)).toEqual([]) + await parse(['configure', '--set-output', 'json'], program) + expect(fired).toBe(1) } finally { - vi.unstubAllGlobals() process.env.SIM_CONFIG_DIR = undefined - Object.defineProperty(stderr, 'isTTY', { configurable: true, value: wasTty }) rmSync(dir, { recursive: true, force: true }) } }) @@ -207,12 +224,7 @@ describe('the update check', () => { */ it('registers the update check as a root preAction hook', async () => { const program = buildProgram() - // Commander keeps lifecycle hooks on a private field and offers no getter, - // the same way `rawArgs` is read elsewhere in this file. - const { _lifeCycleHooks: hooks } = program as Command & { - _lifeCycleHooks?: Record unknown>> - } - const preAction = hooks?.preAction ?? [] + const preAction = preActionHooks(program) expect(preAction).toHaveLength(1) // Invoking it must resolve, never throw: it runs in front of the user's diff --git a/packages/sim-cli/src/update/semver.test.ts b/packages/sim-cli/src/update/semver.test.ts index 6c333557fd0..725cdb163a9 100644 --- a/packages/sim-cli/src/update/semver.test.ts +++ b/packages/sim-cli/src/update/semver.test.ts @@ -34,6 +34,8 @@ describe('parsing a published version', () => { ['01.2.3', 'a leading zero'], ['2.1.2-', 'an empty prerelease'], ['2.1.2-preview..1', 'an empty identifier'], + ['2.1.3-preview.09', 'a zero-padded numeric identifier'], + ['2.1.3-01', 'a zero-padded identifier on its own'], ['', 'nothing at all'], ['latest', 'a dist-tag mistaken for a version'], ])('rejects %s (%s)', (version) => { @@ -63,6 +65,13 @@ describe('precedence', () => { expect(order('2.1.3', '2.1.3-preview.44.1')).toBe(1) }) + it('does not let a malformed identifier outrank every number', () => { + // `09` is not a valid numeric identifier. Accepting it would reclassify it + // as alphanumeric, and alphanumerics outrank numbers — so `preview.010` + // would sort above `preview.2`. + expect(parseVersion('2.1.3-preview.010')).toBeNull() + }) + it('orders two alphanumeric identifiers by ASCII', () => { expect(order('2.1.3-alpha', '2.1.3-beta')).toBe(-1) expect(order('2.1.3-beta', '2.1.3-alpha')).toBe(1) diff --git a/packages/sim-cli/src/update/semver.ts b/packages/sim-cli/src/update/semver.ts index df89273a11a..ea0e451ae9d 100644 --- a/packages/sim-cli/src/update/semver.ts +++ b/packages/sim-cli/src/update/semver.ts @@ -19,6 +19,15 @@ const VERSION_PATTERN = /** A prerelease identifier that is all digits compares as a number. */ const NUMERIC_IDENTIFIER = /^(0|[1-9]\d*)$/ +/** + * A numeric prerelease identifier carrying a leading zero, which the + * specification forbids. It has to be spotted rather than simply failing + * `NUMERIC_IDENTIFIER`: falling through would silently reclassify `09` as an + * alphanumeric identifier, and alphanumerics outrank every number — so + * `preview.010` would sort above `preview.2`. + */ +const LEADING_ZERO_IDENTIFIER = /^0\d+$/ + export interface ParsedVersion { major: number minor: number @@ -58,13 +67,12 @@ export function parseVersion(version: string): ParsedVersion | null { return null } - const prerelease = match[4] - ? match[4] - .split('.') - .map((identifier) => - NUMERIC_IDENTIFIER.test(identifier) ? Number(identifier) : identifier - ) - : [] + const identifiers = match[4] ? match[4].split('.') : [] + if (identifiers.some((identifier) => LEADING_ZERO_IDENTIFIER.test(identifier))) return null + + const prerelease = identifiers.map((identifier) => + NUMERIC_IDENTIFIER.test(identifier) ? Number(identifier) : identifier + ) if ( prerelease.some( (identifier) => typeof identifier === 'number' && !Number.isSafeInteger(identifier) From 1d4e7f500b914a1b026384521955f998c1f83196 Mon Sep 17 00:00:00 2001 From: Marcus Chandra Date: Wed, 2 Sep 2026 15:26:39 -0700 Subject: [PATCH 4/8] fix(cli): make the update-notifier docs match what the code actually does Review round 2. Three findings, all valid. The previous commit's message claimed it had replaced `process.env.SIM_CONFIG_DIR = undefined` with `delete` in the test teardowns. It had not: it added a comment explaining why the assignment is wrong and left the assignment in place, so the teardown still stored the literal string "undefined". Both files now actually delete it. The same pattern exists in profile.test.ts and configure.test.ts, which predate this branch and are left alone. Two documentation claims were stronger than the implementation. "At most once a day" is only true with a writable `~/.sim`. The pace lives in a timestamp file, so a read-only home in a container - or a `~/.sim` left root-owned by an earlier sudo install - means the pace cannot be remembered and the check runs per command. That was already noted in a code comment; it is now in the docs where users read it, along with the fact that it stays bounded by the same one-second timeout. "The tag it was installed from" described behaviour that does not exist. The check only ever queries `latest`, because prerelease installs return before any request. Both docs now say that plainly instead of implying the CLI can ask about the staging or dev channel. --- apps/docs/content/docs/cli/configuration.mdx | 11 +++++++++-- packages/sim-cli/README.md | 8 +++++--- 2 files changed, 14 insertions(+), 5 deletions(-) diff --git a/apps/docs/content/docs/cli/configuration.mdx b/apps/docs/content/docs/cli/configuration.mdx index d4c90404790..f8fe691b352 100644 --- a/apps/docs/content/docs/cli/configuration.mdx +++ b/apps/docs/content/docs/cli/configuration.mdx @@ -124,8 +124,9 @@ shared profile cannot also set its own endpoint or API key. ## Update notices At most once a day, and only when stderr is a terminal, the CLI asks -`registry.npmjs.org` which version is published under the dist-tag it was -installed from. When a newer one exists it prints a single line on stderr naming +`registry.npmjs.org` what is published under the `latest` tag. Prerelease +installs are skipped entirely rather than compared against their own channel, +so a `-preview` or `-dev` build is never told to upgrade. When a newer one exists it prints a single line on stderr naming both versions and the command that upgrades: ``` @@ -154,6 +155,12 @@ The notice is skipped entirely when: deliberately trails the published one - the installed version is a prerelease from the `staging` or `dev` channel +The once-a-day pace comes from a timestamp in `~/.sim/update-check.json`. If +that file cannot be written — a read-only home in a container, or a `~/.sim` +left root-owned by an earlier `sudo` install — the pace cannot be remembered, +so the check runs once per command instead of once per day. It stays bounded by +the same one-second timeout, and `SIM_NO_UPDATE_CHECK=1` still turns it off. + Set `npm_config_registry` to ask a mirror instead; its path and query are preserved, so a token-authenticated Artifactory or Nexus base works. diff --git a/packages/sim-cli/README.md b/packages/sim-cli/README.md index fe6d7be46ca..6d78d5c2ee6 100644 --- a/packages/sim-cli/README.md +++ b/packages/sim-cli/README.md @@ -261,9 +261,11 @@ The main environment variables are: | `SIM_DEBUG` | Print request diagnostics to stderr | | `SIM_NO_UPDATE_CHECK` | Turn off the update notice | -Once a day, at an interactive terminal, `sim` asks `registry.npmjs.org` which -version is published under the tag it was installed from, and prints one line on -stderr when a newer one exists. It sends nothing but its own version and never +Once a day, at an interactive terminal, `sim` asks `registry.npmjs.org` what is +published under the `latest` tag and prints one line on stderr when a newer +version exists. Prerelease installs are skipped entirely. The once-a-day pace +depends on a writable `~/.sim`; without one the check runs per command, still +bounded by a one-second timeout. It sends nothing but its own version and never your Sim API key. If `npm_config_registry` points at a private mirror, the check goes there instead and carries whatever credentials that URL embeds, since the mirror would otherwise refuse it. Set `SIM_NO_UPDATE_CHECK=1` to turn it off; From aba39c580319d38a6ec01098fb1940841dab4fe2 Mon Sep 17 00:00:00 2001 From: Marcus Chandra Date: Wed, 2 Sep 2026 15:32:25 -0700 Subject: [PATCH 5/8] docs(cli): name the update cache path for relocated config dirs Review round 3. The cache is derived from `configDir()`, so it moves with `SIM_CONFIG_DIR` like the config and credentials files do - but the docs named only the `~/.sim` default, sending anyone with a relocated config dir to a file that is not there. --- apps/docs/content/docs/cli/configuration.mdx | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/apps/docs/content/docs/cli/configuration.mdx b/apps/docs/content/docs/cli/configuration.mdx index f8fe691b352..4c4048b1bf2 100644 --- a/apps/docs/content/docs/cli/configuration.mdx +++ b/apps/docs/content/docs/cli/configuration.mdx @@ -155,11 +155,13 @@ The notice is skipped entirely when: deliberately trails the published one - the installed version is a prerelease from the `staging` or `dev` channel -The once-a-day pace comes from a timestamp in `~/.sim/update-check.json`. If -that file cannot be written — a read-only home in a container, or a `~/.sim` -left root-owned by an earlier `sudo` install — the pace cannot be remembered, -so the check runs once per command instead of once per day. It stays bounded by -the same one-second timeout, and `SIM_NO_UPDATE_CHECK=1` still turns it off. +The once-a-day pace comes from a timestamp in `update-check.json`, kept beside +the config and credentials files: `~/.sim/update-check.json` by default, and +under `SIM_CONFIG_DIR` when that is set. If it cannot be written — a read-only +home in a container, or a `~/.sim` left root-owned by an earlier `sudo` install +— the pace cannot be remembered, so the check runs once per command instead of +once per day. It stays bounded by the same one-second timeout, and +`SIM_NO_UPDATE_CHECK=1` still turns it off. Set `npm_config_registry` to ask a mirror instead; its path and query are preserved, so a token-authenticated Artifactory or Nexus base works. From 1573b40a92e93bee33d846cdc28affad7102db89 Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Thu, 3 Sep 2026 10:14:08 -0700 Subject: [PATCH 6/8] fix(cli): harden and simplify update checks --- apps/docs/content/docs/cli/configuration.mdx | 70 +-- .../docs/content/docs/cli/troubleshooting.mdx | 6 +- packages/sim-cli/README.md | 26 +- packages/sim-cli/src/program.test.ts | 8 +- packages/sim-cli/src/program.ts | 3 - .../sim-cli/src/update/check.process.test.ts | 437 ++++++++++++++++++ packages/sim-cli/src/update/check.test.ts | 180 +++++--- packages/sim-cli/src/update/check.ts | 392 +++++++++------- packages/sim-cli/src/update/semver.test.ts | 117 ----- packages/sim-cli/src/update/semver.ts | 144 ------ 10 files changed, 841 insertions(+), 542 deletions(-) create mode 100644 packages/sim-cli/src/update/check.process.test.ts delete mode 100644 packages/sim-cli/src/update/semver.test.ts delete mode 100644 packages/sim-cli/src/update/semver.ts diff --git a/apps/docs/content/docs/cli/configuration.mdx b/apps/docs/content/docs/cli/configuration.mdx index 4c4048b1bf2..b9b9a68c4a7 100644 --- a/apps/docs/content/docs/cli/configuration.mdx +++ b/apps/docs/content/docs/cli/configuration.mdx @@ -114,35 +114,41 @@ shared profile cannot also set its own endpoint or API key. | `SIM_API_KEY` | API key — skips `sim login` entirely | | `SIM_WORKSPACE` | Workspace to target | | `SIM_OUTPUT` | Output format | -| `SIM_CONFIG_DIR` | Relocate both files away from `~/.sim` | +| `SIM_CONFIG_DIR` | Relocate the config directory and update cache; file-specific overrides below still win | | `SIM_CONFIG_FILE` | Relocate only the config file | | `SIM_CREDENTIALS_FILE` | Relocate only the credentials file | | `SIM_TIMEOUT_SECONDS` | Per-request timeout; `0` waits indefinitely. Defaults to `3600`, above every timeout the server itself applies | | `SIM_DEBUG` | Trace each request's method, URL, status and duration to stderr | -| `SIM_NO_UPDATE_CHECK` | Turn off the once-a-day update notice | +| `SIM_NO_UPDATE_CHECK` | Turn off update checks | ## Update notices -At most once a day, and only when stderr is a terminal, the CLI asks +On eligible invocations, the CLI uses a daily cache before asking `registry.npmjs.org` what is published under the `latest` tag. Prerelease -installs are skipped entirely rather than compared against their own channel, -so a `-preview` or `-dev` build is never told to upgrade. When a newer one exists it prints a single line on stderr naming -both versions and the command that upgrades: +installs are skipped entirely, so a `-preview` or `-dev` build is never told to +upgrade. When a newer one exists, it prints a single line on stderr naming both +versions and the command that upgrades: ``` Update available: sim 2.1.2 → 2.1.5. Run: npm install -g sim@latest ``` -The request carries the CLI version and nothing else — no Sim API key, no -workspace, no command — and it never follows a redirect away from the registry -it asked. +Apart from the configured registry URL, the request identifies only the CLI +version — no Sim API key, workspace, or command — and it never follows a +redirect away from the registry it asked. One caveat worth stating plainly: if you point `npm_config_registry` at a -private mirror, the check goes to that mirror instead of npm, and any -credentials embedded in that URL (an Artifactory or Nexus `?token=…`) are sent -with it — they have to be, or the mirror would reject the request. Those are -your registry's credentials, not Sim's, and they go only to the host you -configured. +private mirror, the check goes to that mirror instead of npm. Query-string +credentials (an Artifactory or Nexus `?token=…`, for example) are preserved and +sent as part of the configured registry request — they have to be, or the +mirror would reject it. As with other registry traffic, configured proxies or +TLS inspection can observe what that network setup permits. A registry URL +containing username/password userinfo, such as +`https://user:password@registry.example`, is rejected and no update check is +made. + +Malformed and non-HTTP(S) configured registry values also disable the update +check rather than making an unexpected request to the public registry. The notice is skipped entirely when: @@ -153,27 +159,33 @@ The notice is skipped entirely when: - the CLI is running under `npx`, which resolves the newest version every time - the CLI is running from a checkout of the sim repository, whose version deliberately trails the published one -- the installed version is a prerelease from the `staging` or `dev` channel - -The once-a-day pace comes from a timestamp in `update-check.json`, kept beside -the config and credentials files: `~/.sim/update-check.json` by default, and -under `SIM_CONFIG_DIR` when that is set. If it cannot be written — a read-only -home in a container, or a `~/.sim` left root-owned by an earlier `sudo` install -— the pace cannot be remembered, so the check runs once per command instead of -once per day. It stays bounded by the same one-second timeout, and -`SIM_NO_UPDATE_CHECK=1` still turns it off. - -Set `npm_config_registry` to ask a mirror instead; its path and query are -preserved, so a token-authenticated Artifactory or Nexus base works. +- the installed version is a prerelease + +The daily pace comes from a timestamp in the config directory's +`update-check.json`: `~/.sim/update-check.json` by default, or under +`SIM_CONFIG_DIR` when that is set. `SIM_CONFIG_FILE` and +`SIM_CREDENTIALS_FILE` do not move the cache, so it may not sit beside a file +relocated with either of those variables. + +This throttle is best-effort across processes. Two commands that start together +can both see a stale cache and check. Cache replacement is atomic, so either +complete write can win without leaving a partially interleaved file. If the +cache cannot be written — for example, because the config directory is +read-only — every eligible invocation attempts a check because there is no +timestamp to reuse. + +The registry check has a one-second deadline. On expiry, the CLI terminates its +short-lived request process so stalled DNS, connection, or response work cannot +remain active and delay the command. `SIM_NO_UPDATE_CHECK=1` still turns the +check off. The command the notice prints matches how Sim was installed — `npm install -g`, `pnpm add -g`, `bun add -g`, or `yarn global add` — so running it updates the executable already on your `PATH` rather than installing a second copy under a different package manager. -Node ignores `HTTPS_PROXY` unless you also set `NODE_USE_ENV_PROXY=1`, and only -from Node 22.21 and 24.5. The CLI warns when a proxy is configured but will not -be used. +Node's `fetch` uses `HTTP(S)_PROXY` when opted in with `NODE_USE_ENV_PROXY=1` +(Node 22.21+ or 24.0+) or `--use-env-proxy` (Node 22.21+ or 24.5+). For CI, set `SIM_API_KEY` and `SIM_WORKSPACE` and nothing needs to touch the filesystem at all. diff --git a/apps/docs/content/docs/cli/troubleshooting.mdx b/apps/docs/content/docs/cli/troubleshooting.mdx index 3ae53b624cd..c18a06ff5a2 100644 --- a/apps/docs/content/docs/cli/troubleshooting.mdx +++ b/apps/docs/content/docs/cli/troubleshooting.mdx @@ -118,9 +118,9 @@ one installs a second copy instead of replacing the executable on your `PATH`: -The CLI normally tells you this itself, once a day, on stderr, and the command -it prints already matches your installation. It stays quiet when stderr is -redirected, in CI, and under `npx`. +The CLI can also tell you this through a cached daily check on eligible +invocations, and the command it prints already matches your installation. It +stays quiet when stderr is redirected, in CI, and under `npx`. ## An update notice appears in output I am parsing diff --git a/packages/sim-cli/README.md b/packages/sim-cli/README.md index 6d78d5c2ee6..2a5f4aac220 100644 --- a/packages/sim-cli/README.md +++ b/packages/sim-cli/README.md @@ -256,21 +256,25 @@ The main environment variables are: | `SIM_API_KEY` | API key, usually for CI | | `SIM_WORKSPACE` | Workspace to target | | `SIM_OUTPUT` | `table`, `json`, `yaml`, or `text` | -| `SIM_CONFIG_DIR` | Directory containing CLI config and credentials | +| `SIM_CONFIG_DIR` | Base directory for CLI config, credentials, and the update cache | | `SIM_TIMEOUT_SECONDS` | Per-request timeout; `0` waits indefinitely | | `SIM_DEBUG` | Print request diagnostics to stderr | | `SIM_NO_UPDATE_CHECK` | Turn off the update notice | -Once a day, at an interactive terminal, `sim` asks `registry.npmjs.org` what is -published under the `latest` tag and prints one line on stderr when a newer -version exists. Prerelease installs are skipped entirely. The once-a-day pace -depends on a writable `~/.sim`; without one the check runs per command, still -bounded by a one-second timeout. It sends nothing but its own version and never -your Sim API key. If `npm_config_registry` points at a private mirror, the check -goes there instead and carries whatever credentials that URL embeds, since the -mirror would otherwise refuse it. Set `SIM_NO_UPDATE_CHECK=1` to turn it off; -the full list of cases where it stays quiet is in the -[configuration guide](https://docs.sim.ai/cli/configuration). +On eligible interactive invocations, `sim` uses a daily cache before asking +`registry.npmjs.org` what is published under the `latest` tag and prints one +line on stderr when a newer version exists. Prerelease installs are skipped +entirely. The cache lives in `~/.sim` by default and follows `SIM_CONFIG_DIR`; +without a writable cache, each eligible invocation checks again. Concurrent +invocations can also perform duplicate checks. The registry request has a +one-second deadline; the short-lived request process is terminated on expiry. +Apart from the configured registry URL, it sends only its own version and never +your Sim API key. If `npm_config_registry` points at a private mirror, its query +string is preserved, including any query-string credentials. Registry URLs +containing username/password userinfo are rejected. Set +`SIM_NO_UPDATE_CHECK=1` to turn it off; malformed or non-HTTP(S) configured +registry values also fail closed. The full list of cases where it stays quiet +is in the [configuration guide](https://docs.sim.ai/cli/configuration). ## Documentation diff --git a/packages/sim-cli/src/program.test.ts b/packages/sim-cli/src/program.test.ts index e0f74da335c..8124fc3fb83 100644 --- a/packages/sim-cli/src/program.test.ts +++ b/packages/sim-cli/src/program.test.ts @@ -198,15 +198,15 @@ describe('the update check', () => { await parse(['--help'], program) expect(fired).toBe(0) - // And the sentinel is not inert: the same hook does fire for a real action, - // which is what makes the assertion above mean something. const dir = mkdtempSync(join(tmpdir(), 'sim-cli-program-')) + const previousConfigDir = process.env.SIM_CONFIG_DIR process.env.SIM_CONFIG_DIR = dir try { await parse(['configure', '--set-output', 'json'], program) expect(fired).toBe(1) } finally { - process.env.SIM_CONFIG_DIR = undefined + if (previousConfigDir === undefined) Reflect.deleteProperty(process.env, 'SIM_CONFIG_DIR') + else process.env.SIM_CONFIG_DIR = previousConfigDir rmSync(dir, { recursive: true, force: true }) } }) @@ -227,8 +227,6 @@ describe('the update check', () => { const preAction = preActionHooks(program) expect(preAction).toHaveLength(1) - // Invoking it must resolve, never throw: it runs in front of the user's - // command, and a rejection here would fail the command itself. await expect(preAction[0](program, program)).resolves.toBeUndefined() }) }) diff --git a/packages/sim-cli/src/program.ts b/packages/sim-cli/src/program.ts index f184df5592c..d83cd6afe79 100644 --- a/packages/sim-cli/src/program.ts +++ b/packages/sim-cli/src/program.ts @@ -152,9 +152,6 @@ export function buildProgram(options: { version?: boolean } = {}): Command { program.addHelpText('after', HELP_EPILOGUE) - // Root hooks are inherited by the whole tree, and commander answers `--help` - // and `--version` during parsing without ever reaching an action — so the two - // invocations that must stay instant are excluded by construction. program.hook('preAction', () => announceUpdateIfAvailable()) refuseHelpAfterUnknownCommand(program) diff --git a/packages/sim-cli/src/update/check.process.test.ts b/packages/sim-cli/src/update/check.process.test.ts new file mode 100644 index 00000000000..63ef00597d0 --- /dev/null +++ b/packages/sim-cli/src/update/check.process.test.ts @@ -0,0 +1,437 @@ +/** + * @vitest-environment node + */ +import { execFileSync, spawn } from 'node:child_process' +import { mkdirSync, mkdtempSync, readFileSync, rmSync, writeFileSync } from 'node:fs' +import { createServer, type RequestListener } from 'node:http' +import type { AddressInfo, Socket } from 'node:net' +import { tmpdir } from 'node:os' +import { join } from 'node:path' +import { fileURLToPath, pathToFileURL } from 'node:url' +import { afterAll, beforeAll, expect, it } from 'vitest' + +interface ChildResult { + code: number | null + elapsedMs: number + signal: NodeJS.Signals | null + stderr: string + stdout: string +} + +interface CheckOutput { + elapsedMs: number + notices: string[] +} + +interface RunChildOptions { + env?: NodeJS.ProcessEnv + nodeArgs?: string[] + useProcessEnv?: boolean +} + +let entrypoint: string +let temporaryDir: string + +const [NODE_MAJOR, NODE_MINOR] = process.versions.node.split('.').map(Number) +const SUPPORTS_ENV_PROXY = NODE_MAJOR >= 24 || (NODE_MAJOR === 22 && NODE_MINOR >= 21) +const SUPPORTS_PROXY_FLAG = + NODE_MAJOR > 24 || + (NODE_MAJOR === 24 && NODE_MINOR >= 5) || + (NODE_MAJOR === 22 && NODE_MINOR >= 21) + +function buildUpdateCheck(temporaryDir: string): string { + const entrypoint = join(temporaryDir, 'dist', 'check.mjs') + const sourcePath = fileURLToPath(new URL('./check.ts', import.meta.url)) + mkdirSync(join(temporaryDir, 'dist')) + writeFileSync(join(temporaryDir, 'package.json'), JSON.stringify({ version: '2.1.2' })) + execFileSync( + 'bun', + ['build', sourcePath, '--target=node', '--format=esm', '--outfile', entrypoint], + { stdio: 'pipe' } + ) + return entrypoint +} + +function runChild( + entrypoint: string, + registry: string, + configDir: string, + options: RunChildOptions = {} +): Promise { + const requestEnvironment = options.useProcessEnv + ? 'process.env' + : `{ npm_config_registry: ${JSON.stringify(registry)} }` + const source = ` + import { announceUpdateIfAvailable } from ${JSON.stringify(pathToFileURL(entrypoint).href)} + const started = Date.now() + const notices = [] + await announceUpdateIfAvailable({ + currentVersion: '2.1.2', + env: ${requestEnvironment}, + isTty: true, + modulePath: '/usr/local/lib/node_modules/sim/dist/index.js', + write: (message) => notices.push(message), + }) + process.stdout.write(JSON.stringify({ elapsedMs: Date.now() - started, notices })) + ` + const started = performance.now() + const child = spawn( + process.execPath, + [...(options.nodeArgs ?? []), '--input-type=module', '--eval', source], + { + env: { + ...process.env, + NODE_USE_ENV_PROXY: '0', + NO_PROXY: '127.0.0.1,localhost', + ...options.env, + SIM_CONFIG_DIR: configDir, + ...(options.useProcessEnv ? { npm_config_registry: registry } : {}), + }, + stdio: ['ignore', 'pipe', 'pipe'], + } + ) + let stdout = '' + let stderr = '' + child.stdout.setEncoding('utf8').on('data', (chunk: string) => { + stdout += chunk + }) + child.stderr.setEncoding('utf8').on('data', (chunk: string) => { + stderr += chunk + }) + + return new Promise((resolve, reject) => { + let timedOut = false + const deadline = setTimeout(() => { + timedOut = true + child.kill('SIGKILL') + }, 3500) + child.once('error', reject) + child.once('close', (code, signal) => { + clearTimeout(deadline) + if (timedOut) { + reject(new Error('Update-check child did not exit promptly')) + return + } + resolve({ code, elapsedMs: performance.now() - started, signal, stderr, stdout }) + }) + }) +} + +async function withServer( + listener: RequestListener, + run: (origin: string) => Promise +): Promise { + const server = createServer(listener) + await new Promise((resolve) => server.listen(0, '127.0.0.1', resolve)) + const { port } = server.address() as AddressInfo + + try { + await run(`http://127.0.0.1:${port}`) + } finally { + server.closeAllConnections() + await new Promise((resolve) => server.close(() => resolve())) + } +} + +async function withProxyServer( + run: (origin: string) => Promise +): Promise<{ requests: string[]; result: T }> { + const requests: string[] = [] + const body = JSON.stringify({ latest: '2.1.5' }) + const server = createServer((request, response) => { + requests.push(request.url ?? '') + response.setHeader('content-type', 'application/json') + response.end(body) + }) + server.on('connect', (request, socket, head) => { + requests.push(`CONNECT ${request.url ?? ''}`) + socket.write('HTTP/1.1 200 Connection Established\r\n\r\n') + const respond = () => { + socket.end( + `HTTP/1.1 200 OK\r\nContent-Type: application/json\r\nContent-Length: ${Buffer.byteLength(body)}\r\nConnection: close\r\n\r\n${body}` + ) + } + if (head.length > 0) respond() + else socket.once('data', respond) + }) + + await new Promise((resolve) => server.listen(0, '127.0.0.1', resolve)) + const { port } = server.address() as AddressInfo + try { + const result = await run(`http://127.0.0.1:${port}`) + return { requests, result } + } finally { + server.closeAllConnections() + await new Promise((resolve) => server.close(() => resolve())) + } +} + +beforeAll(() => { + temporaryDir = mkdtempSync(join(tmpdir(), 'sim-cli-update-process-')) + entrypoint = buildUpdateCheck(temporaryDir) +}) + +afterAll(() => { + rmSync(temporaryDir, { recursive: true, force: true }) +}) + +it('destroys a timed-out request so its socket cannot hold the process open', async () => { + await withServer( + () => {}, + async (origin) => { + const result = await runChild(entrypoint, origin, join(temporaryDir, 'config')) + + expect(result).toMatchObject({ code: 0, signal: null, stderr: '' }) + const output = JSON.parse(result.stdout) as CheckOutput + expect(output.elapsedMs).toBeGreaterThanOrEqual(750) + expect(result.elapsedMs).toBeLessThan(3500) + } + ) +}, 10_000) + +it('destroys a response whose body stalls after the headers arrive', async () => { + await withServer( + (_request, response) => { + response.writeHead(200, { 'content-type': 'application/json' }) + response.write('{"latest":') + }, + async (origin) => { + const result = await runChild(entrypoint, origin, join(temporaryDir, 'config-stalled-body')) + + expect(result).toMatchObject({ code: 0, signal: null, stderr: '' }) + const output = JSON.parse(result.stdout) as CheckOutput + expect(output.elapsedMs).toBeGreaterThanOrEqual(750) + expect(result.elapsedMs).toBeLessThan(3500) + } + ) +}, 10_000) + +it('gives the request process its own deadline if the CLI process disappears', async () => { + const server = createServer(() => {}) + let outer: ReturnType | undefined + + try { + await new Promise((resolve) => server.listen(0, '127.0.0.1', resolve)) + const { port } = server.address() as AddressInfo + const source = ` + import { announceUpdateIfAvailable } from ${JSON.stringify(pathToFileURL(entrypoint).href)} + await announceUpdateIfAvailable({ + currentVersion: '2.1.2', + env: { npm_config_registry: 'http://127.0.0.1:${port}' }, + isTty: true, + modulePath: '/usr/local/lib/node_modules/sim/dist/index.js', + }) + ` + const connection = new Promise((resolve, reject) => { + const deadline = setTimeout(() => reject(new Error('Registry probe did not connect')), 2500) + server.once('connection', (socket) => { + clearTimeout(deadline) + resolve(socket) + }) + }) + + outer = spawn(process.execPath, ['--input-type=module', '--eval', source], { + env: { + ...process.env, + NODE_USE_ENV_PROXY: '0', + NO_PROXY: '127.0.0.1,localhost', + SIM_CONFIG_DIR: join(temporaryDir, 'config-orphan'), + }, + stdio: 'ignore', + }) + const socket = await connection + const killedAt = performance.now() + outer.kill('SIGKILL') + await new Promise((resolve) => outer?.once('close', () => resolve())) + await new Promise((resolve, reject) => { + const deadline = setTimeout( + () => reject(new Error('Orphaned registry probe outlived its own deadline')), + 2500 + ) + socket.once('close', () => { + clearTimeout(deadline) + resolve() + }) + }) + + expect(performance.now() - killedAt).toBeLessThan(2500) + } finally { + outer?.kill('SIGKILL') + server.closeAllConnections() + await new Promise((resolve) => server.close(() => resolve())) + } +}, 10_000) + +it('caps a chunked response even when it omits Content-Length', async () => { + await withServer( + (_request, response) => { + response.writeHead(200, { 'content-type': 'application/json' }) + response.write(JSON.stringify({ latest: '2.1.5' })) + response.end(' '.repeat(64 * 1024)) + }, + async (origin) => { + const result = await runChild(entrypoint, origin, join(temporaryDir, 'config-chunked')) + + expect(result).toMatchObject({ code: 0, signal: null, stderr: '' }) + const output = JSON.parse(result.stdout) as CheckOutput + expect(output.notices).toEqual([]) + } + ) +}, 10_000) + +it('preserves a mirror path, query, and reduced request headers', async () => { + let requestHeaders: Record = {} + let requestPath: string | undefined + await withServer( + (request, response) => { + requestHeaders = request.headers + requestPath = request.url + response.setHeader('content-type', 'application/json') + response.end(JSON.stringify({ latest: '2.1.5' })) + }, + async (origin) => { + const result = await runChild( + entrypoint, + `${origin}/api/npm/repo?token=abc`, + join(temporaryDir, 'config-mirror') + ) + + expect(result).toMatchObject({ code: 0, signal: null, stderr: '' }) + const output = JSON.parse(result.stdout) as CheckOutput + expect(output.notices).toEqual([ + 'Update available: sim 2.1.2 → 2.1.5. Run: npm install -g sim@latest\n', + ]) + expect(requestPath).toBe('/api/npm/repo/-/package/sim/dist-tags?token=abc') + expect(requestHeaders).toMatchObject({ + accept: 'application/json', + 'user-agent': 'sim-cli/2.1.2', + }) + expect(requestHeaders.authorization).toBeUndefined() + } + ) +}, 10_000) + +it.skipIf(!SUPPORTS_ENV_PROXY)( + 'preserves built-in environment proxy support inside the request process', + async () => { + const { requests, result } = await withProxyServer((origin) => + runChild(entrypoint, 'http://sim-update.invalid', join(temporaryDir, 'config-proxy'), { + env: { + HTTP_PROXY: origin, + NODE_USE_ENV_PROXY: '1', + NO_PROXY: '', + http_proxy: origin, + no_proxy: '', + }, + }) + ) + + expect(result).toMatchObject({ code: 0, signal: null, stderr: '' }) + expect(requests).not.toEqual([]) + const output = JSON.parse(result.stdout) as CheckOutput + expect(output.notices).toHaveLength(1) + }, + 10_000 +) + +it.skipIf(!SUPPORTS_PROXY_FLAG)( + 'preserves a parent use-env-proxy flag and its command-line precedence', + async () => { + const { requests, result } = await withProxyServer((origin) => + runChild(entrypoint, 'http://sim-update.invalid', join(temporaryDir, 'config-proxy-flag'), { + env: { + HTTP_PROXY: origin, + NODE_OPTIONS: '--no-use-env-proxy', + NODE_USE_ENV_PROXY: '0', + NO_PROXY: '', + http_proxy: origin, + no_proxy: '', + }, + nodeArgs: ['--use-env-proxy'], + }) + ) + + expect(result).toMatchObject({ code: 0, signal: null, stderr: '' }) + expect(requests).not.toEqual([]) + const output = JSON.parse(result.stdout) as CheckOutput + expect(output.notices).toHaveLength(1) + }, + 10_000 +) + +it('keeps a configured registry credential out of probe argv and environment', async () => { + const inspectionPath = join(temporaryDir, 'probe-inspection.json') + const preloadDir = join(temporaryDir, 'probe preload') + const preloadPath = join(preloadDir, 'inspect-probe.cjs') + const sentinel = 'registry-secret-sentinel' + let requestPath: string | undefined + mkdirSync(preloadDir) + writeFileSync( + preloadPath, + ` + const { writeFileSync } = require('node:fs') + if (process.execArgv.some((value) => value.includes('maxResponseBytes'))) { + writeFileSync( + process.env.PROBE_INSPECTION_PATH, + JSON.stringify({ + argv: process.argv, + environmentValues: Object.values(process.env), + execArgv: process.execArgv, + }) + ) + } + ` + ) + + await withServer( + (request, response) => { + requestPath = request.url + response.setHeader('content-type', 'application/json') + response.end(JSON.stringify({ latest: '2.1.5' })) + }, + async (origin) => { + const registry = `${origin}?token=${sentinel}` + const result = await runChild(entrypoint, registry, join(temporaryDir, 'config-credential'), { + env: { + NODE_OPTIONS: `--require="${preloadPath}"`, + NPM_CONFIG_REGISTRY: registry, + PROBE_INSPECTION_PATH: inspectionPath, + }, + useProcessEnv: true, + }) + + expect(result).toMatchObject({ code: 0, signal: null, stderr: '' }) + expect(requestPath).toBe(`/-/package/sim/dist-tags?token=${sentinel}`) + const inspection = JSON.parse(readFileSync(inspectionPath, 'utf8')) as { + argv: string[] + environmentValues: string[] + execArgv: string[] + } + expect(JSON.stringify(inspection)).not.toContain(sentinel) + } + ) +}, 10_000) + +it('refuses redirects without contacting their destination', async () => { + const paths: string[] = [] + await withServer( + (request, response) => { + paths.push(request.url ?? '') + if (request.url === '/redirected') { + response.setHeader('content-type', 'application/json') + response.end(JSON.stringify({ latest: '2.1.5' })) + return + } + response.writeHead(302, { location: '/redirected' }) + response.end() + }, + async (origin) => { + const result = await runChild(entrypoint, origin, join(temporaryDir, 'config-redirect')) + + expect(result).toMatchObject({ code: 0, signal: null, stderr: '' }) + const output = JSON.parse(result.stdout) as CheckOutput + expect(output.notices).toEqual([]) + expect(paths).toEqual(['/-/package/sim/dist-tags']) + } + ) +}, 10_000) diff --git a/packages/sim-cli/src/update/check.test.ts b/packages/sim-cli/src/update/check.test.ts index cfdda5b6331..2eec8e75c90 100644 --- a/packages/sim-cli/src/update/check.test.ts +++ b/packages/sim-cli/src/update/check.test.ts @@ -1,38 +1,47 @@ /** * @vitest-environment node */ -import { mkdtempSync, readFileSync, rmSync, statSync, writeFileSync } from 'node:fs' +import { + linkSync, + mkdtempSync, + readFileSync, + rmSync, + statSync, + symlinkSync, + writeFileSync, +} from 'node:fs' import { tmpdir } from 'node:os' import { join } from 'node:path' -import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import { afterEach, beforeEach, describe, expect, it } from 'vitest' import { CLI_VERSION } from '../version' -import { announceUpdateIfAvailable, resetUpdateCheck, upgradeCommand } from './check' +import { announceUpdateIfAvailable, type UpdateCheckOptions, upgradeCommand } from './check' /** A global install, which is the only shape that gets advised at all. */ const INSTALLED = '/usr/local/lib/node_modules/sim/dist/index.js' let configDir: string +let previousConfigDir: string | undefined let notices: string[] let fetched: URL[] -let inits: RequestInit[] +type RegistryRequest = NonNullable +let inits: Parameters[1][] +let registryRequest: RegistryRequest /** Answers the dist-tags request the way the registry does. */ function stubRegistry( tags: Record | 'reject' | 'not-found' | 'html' | 'oversized' ): void { - vi.stubGlobal('fetch', (input: URL, init: RequestInit) => { + registryRequest = async (input, init) => { fetched.push(input) inits.push(init) if (tags === 'oversized') { - return Promise.resolve( - Response.json({ latest: '2.1.5' }, { headers: { 'content-length': String(1024 * 1024) } }) - ) + return `${JSON.stringify({ latest: '2.1.5' })}${' '.repeat(64 * 1024)}` } - if (tags === 'reject') return Promise.reject(new Error('getaddrinfo ENOTFOUND')) - if (tags === 'not-found') return Promise.resolve(new Response('', { status: 404 })) - if (tags === 'html') return Promise.resolve(new Response('nope', { status: 200 })) - return Promise.resolve(Response.json(tags)) - }) + if (tags === 'reject') throw new Error('getaddrinfo ENOTFOUND') + if (tags === 'not-found') return null + if (tags === 'html') return 'nope' + return JSON.stringify(tags) + } } async function run(overrides: Parameters[0] = {}) { @@ -41,6 +50,7 @@ async function run(overrides: Parameters[0] = env: {}, isTty: true, modulePath: INSTALLED, + registryRequest, write: (message) => notices.push(message), ...overrides, }) @@ -51,7 +61,7 @@ function cachePath(): string { } beforeEach(() => { - resetUpdateCheck() + previousConfigDir = process.env.SIM_CONFIG_DIR configDir = mkdtempSync(join(tmpdir(), 'sim-cli-update-')) process.env.SIM_CONFIG_DIR = configDir notices = [] @@ -61,10 +71,8 @@ beforeEach(() => { }) afterEach(() => { - vi.unstubAllGlobals() - // `= undefined` would store the literal string "undefined", leaving later - // tests pointed at a relative `./undefined` config directory. - process.env.SIM_CONFIG_DIR = undefined + if (previousConfigDir === undefined) Reflect.deleteProperty(process.env, 'SIM_CONFIG_DIR') + else process.env.SIM_CONFIG_DIR = previousConfigDir rmSync(configDir, { recursive: true, force: true }) }) @@ -91,9 +99,16 @@ describe('announcing a newer release', () => { expect(notices).toEqual([]) }) + it.each([ + ['minor', '2.2.0'], + ['major', '3.0.0'], + ])('announces a newer %s version', async (_difference, latest) => { + stubRegistry({ latest }) + await run() + expect(notices.join('')).toContain(`2.1.2 → ${latest}`) + }) + it('writes through the real default: stderr yes, stdout never', async () => { - // Deliberately without the `write` override, so the production default is - // the thing under test. stdout may be a pipeline feeding jq. const realOut = process.stdout.write const realErr = process.stderr.write const seen = { out: [] as string[], err: [] as string[] } @@ -111,6 +126,7 @@ describe('announcing a newer release', () => { env: {}, isTty: true, modulePath: INSTALLED, + registryRequest, }) } finally { process.stdout.write = realOut @@ -120,20 +136,14 @@ describe('announcing a newer release', () => { expect(seen.err.join('')).toContain('Update available: sim 2.1.2 → 2.1.5') }) - it('sends only its own version, and refuses to follow a redirect', async () => { + it('sends only its own version and gives the request a one-second deadline', async () => { await run() - const headers = inits[0]?.headers as Record - // The reduced agent is the privacy property: the exported USER_AGENT in - // version.ts also carries node version, platform and arch. + const headers = inits[0]?.headers expect(headers['user-agent']).toBe(`sim-cli/${CLI_VERSION}`) expect(headers.accept).toBe('application/json') expect(headers.authorization).toBeUndefined() - expect(inits[0]?.redirect).toBe('error') - }) - - it('bounds the request so a hung registry cannot stall the command', async () => { - await run() - expect(inits[0]?.signal).toBeInstanceOf(AbortSignal) + expect(inits[0]?.maxResponseBytes).toBe(64 * 1024) + expect(inits[0]?.timeoutMs).toBe(1000) }) }) @@ -174,9 +184,6 @@ describe('when the notice is suppressed', () => { it.each([ '/Users/x/sim/packages/sim-cli/dist/index.js', - // Windows, and mixed case: a checkout is a checkout on a case-insensitive - // volume too, and this is the guard that stops every Sim engineer being - // nagged daily by their own build. 'C:\\Users\\x\\Sim\\Packages\\Sim-CLI\\dist\\index.js', ])( 'says nothing from a checkout, whose manifest trails npm by design (%s)', @@ -187,43 +194,36 @@ describe('when the notice is suppressed', () => { ) it('says nothing to a prerelease install', async () => { - stubRegistry({ latest: '2.1.5', staging: '2.1.6-preview.812.1' }) + stubRegistry({ latest: '2.1.5' }) await run({ currentVersion: '2.1.3-preview.44.1' }) expect(fetched).toEqual([]) expect(notices).toEqual([]) }) + it('ignores stable build metadata when comparing versions', async () => { + await run({ currentVersion: '2.1.2+local.1' }) + expect(notices).toHaveLength(1) + }) + it('says nothing when the running version cannot be read', async () => { await run({ currentVersion: 'not-a-version' }) expect(notices).toEqual([]) }) - - it('speaks only once per process', async () => { - await run() - rmSync(cachePath(), { force: true }) - await run() - expect(notices).toHaveLength(1) - }) }) describe('the once-a-day cache', () => { - it('records the check, and what it found', async () => { + it('records when the check ran', async () => { const now = new Date('2026-09-02T10:00:00.000Z') await run({ now }) expect(JSON.parse(readFileSync(cachePath(), 'utf8'))).toEqual({ version: 1, checkedAt: '2026-09-02T10:00:00.000Z', - latestVersion: '2.1.5', }) - // Assert the property, not the literal mode: writeFileSync's mode is - // masked by the ambient umask, so an exact comparison fails under - // `umask 077` for reasons that have nothing to do with this code. expect(statSync(cachePath()).mode & 0o022).toBe(0) }) it('does not contact the registry again within the day', async () => { await run({ now: new Date('2026-09-02T10:00:00.000Z') }) - resetUpdateCheck() fetched = [] notices = [] await run({ now: new Date('2026-09-02T22:00:00.000Z') }) @@ -233,17 +233,13 @@ describe('the once-a-day cache', () => { it('checks again once the day is up', async () => { await run({ now: new Date('2026-09-02T10:00:00.000Z') }) - resetUpdateCheck() notices = [] await run({ now: new Date('2026-09-03T11:00:00.000Z') }) expect(notices).toHaveLength(1) }) it('checks again when the clock has moved backwards', async () => { - // A stamp in the future would otherwise suppress the notice until the clock - // caught up, which after a one-off jump forward is permanently. await run({ now: new Date('2026-09-02T10:00:00.000Z') }) - resetUpdateCheck() notices = [] await run({ now: new Date('2026-09-01T10:00:00.000Z') }) expect(notices).toHaveLength(1) @@ -261,6 +257,49 @@ describe('the once-a-day cache', () => { expect(notices).toHaveLength(1) }) + it('re-checks rather than trusting an oversized valid fresh cache', async () => { + const now = new Date('2026-09-02T10:00:00.000Z') + writeFileSync( + cachePath(), + JSON.stringify({ + version: 1, + checkedAt: now.toISOString(), + padding: 'x'.repeat(1024 * 1024), + }) + ) + + await run({ now }) + + expect(fetched).toHaveLength(1) + expect(notices).toHaveLength(1) + }) + + it('replaces a hard-linked cache without modifying its other name', async () => { + const victimPath = join(configDir, 'victim') + writeFileSync(victimPath, 'do not overwrite') + linkSync(victimPath, cachePath()) + + await run() + + expect(readFileSync(victimPath, 'utf8')).toBe('do not overwrite') + expect(JSON.parse(readFileSync(cachePath(), 'utf8'))).toMatchObject({ + version: 1, + checkedAt: expect.any(String), + }) + }) + + it('re-checks rather than following a cache symlink', async () => { + const victimPath = join(configDir, 'victim') + writeFileSync(victimPath, JSON.stringify({ version: 1, checkedAt: new Date().toISOString() })) + symlinkSync(victimPath, cachePath()) + + await run() + + expect(fetched).toHaveLength(1) + expect(notices).toHaveLength(1) + expect(readFileSync(victimPath, 'utf8')).toContain('"version":1') + }) + it('still runs the command when the cache cannot be written', async () => { const wall = join(configDir, 'wall') writeFileSync(wall, 'not a directory') @@ -302,7 +341,6 @@ describe('when the registry does not answer', () => { await run() expect(notices).toEqual([]) - resetUpdateCheck() rmSync(cachePath(), { force: true }) stubRegistry({ latest: 'nonsense' }) await run() @@ -312,7 +350,10 @@ describe('when the registry does not answer', () => { it('still records the attempt, so a dead registry costs one request a day', async () => { stubRegistry('reject') await run({ now: new Date('2026-09-02T10:00:00.000Z') }) - expect(JSON.parse(readFileSync(cachePath(), 'utf8')).latestVersion).toBeNull() + expect(JSON.parse(readFileSync(cachePath(), 'utf8'))).toEqual({ + version: 1, + checkedAt: '2026-09-02T10:00:00.000Z', + }) }) it('asks a configured mirror instead of the default', async () => { @@ -320,18 +361,27 @@ describe('when the registry does not answer', () => { expect(fetched.map(String)).toEqual(['https://npm.internal/api/npm/-/package/sim/dist-tags']) }) + it('refuses registry URLs with username/password userinfo', async () => { + await run({ env: { npm_config_registry: 'https://user:secret@npm.internal/api/npm' } }) + expect(fetched).toEqual([]) + expect(notices).toEqual([]) + }) + it.each([ - ['a value that is not a url', 'not a url'], - ['a non-http protocol', 'file:///var/tmp/registry'], - ['whitespace', ' '], - ])('falls back to the default registry for %s', async (_label, configured) => { + ['a value that is not a URL', 'not a url'], + ['a non-HTTP protocol', 'file:///var/tmp/registry'], + ])('makes no request for %s', async (_label, configured) => { await run({ env: { npm_config_registry: configured } }) + expect(fetched).toEqual([]) + expect(notices).toEqual([]) + }) + + it('uses the default registry when the configured value is only whitespace', async () => { + await run({ env: { npm_config_registry: ' ' } }) expect(fetched.map(String)).toEqual(['https://registry.npmjs.org/-/package/sim/dist-tags']) }) it("keeps a token-authenticated mirror's own path and query", async () => { - // Artifactory and Nexus bases carry both. Resolving the path as a relative - // URL would drop them and ask the mirror a question it answers with a 404. await run({ env: { npm_config_registry: 'https://npm.internal/api/npm/repo?token=abc' } }) expect(fetched.map(String)).toEqual([ 'https://npm.internal/api/npm/repo/-/package/sim/dist-tags?token=abc', @@ -354,16 +404,12 @@ describe('the upgrade command', () => { 'pnpm add -g sim@latest', ], ])('reads %s as the installation it is', (modulePath, expected) => { - expect(upgradeCommand('latest', modulePath, {})).toBe(expected) + expect(upgradeCommand(modulePath, {})).toBe(expected) }) it('falls back to the invoking package manager when the path says nothing', () => { - expect( - upgradeCommand('latest', INSTALLED, { npm_config_user_agent: 'pnpm/9.1.0 npm/? node/v22' }) - ).toBe('pnpm add -g sim@latest') - }) - - it('names the channel it is advising, not always the stable one', () => { - expect(upgradeCommand('staging', INSTALLED, {})).toBe('npm install -g sim@staging') + expect(upgradeCommand(INSTALLED, { npm_config_user_agent: 'pnpm/9.1.0 npm/? node/v22' })).toBe( + 'pnpm add -g sim@latest' + ) }) }) diff --git a/packages/sim-cli/src/update/check.ts b/packages/sim-cli/src/update/check.ts index ce549135d92..330f8b24e75 100644 --- a/packages/sim-cli/src/update/check.ts +++ b/packages/sim-cli/src/update/check.ts @@ -10,48 +10,65 @@ * that writes anything to stdout, is worse than no notice at all. */ -import { mkdirSync, readFileSync, writeFileSync } from 'node:fs' +import { spawn } from 'node:child_process' +import { + closeSync, + constants, + fstatSync, + lstatSync, + mkdirSync, + openSync, + readSync, + renameSync, + unlinkSync, + writeFileSync, +} from 'node:fs' import { dirname } from 'node:path' import { fileURLToPath } from 'node:url' import { updateCachePath } from '../config/paths' import { CLI_VERSION } from '../version' -import { channelOf, compareVersions, parseVersion, type ReleaseChannel } from './semver' -/** How long a check is trusted. The stated contract is one notice per day. */ +/** How long a cached check suppresses another request. */ const CHECK_INTERVAL_MS = 24 * 60 * 60 * 1000 -/** - * Deliberately not `SIM_TIMEOUT_SECONDS`, which defaults to an hour: that bound - * governs work the user asked for, and this is work they did not. - */ +/** Courtesy work gets a short deadline independent of command request timeouts. */ const REGISTRY_TIMEOUT_MS = 1000 const DEFAULT_REGISTRY = 'https://registry.npmjs.org' -/** - * The published package. Named once because it appears in two unrelated places - * — the registry path and the upgrade command — and a rename that updated only - * one would leave the CLI asking about one package while advising another. - */ +/** Published package name used in both the registry path and upgrade command. */ const PACKAGE_NAME = 'sim' /** Relative to the registry root, and about a hundred bytes of response. */ const DIST_TAGS_PATH = `-/package/${PACKAGE_NAME}/dist-tags` -/** - * A ceiling on the response body. The endpoint answers in ~100 bytes, so this - * is three orders of magnitude of headroom; it exists because the host is - * partly environment-controlled through `npm_config_registry`, and an - * unbounded read from a mirror on a fast link can buffer arbitrarily much - * before the timeout fires. - */ +/** Bounds responses from the environment-configurable registry host. */ const MAX_RESPONSE_BYTES = 64 * 1024 -/** - * Set by every CI provider worth naming. `!isTTY` already covers most of them; - * this catches the ones that allocate a terminal anyway, such as a Buildkite - * agent or `docker run -t`. - */ +/** Far above the small timestamp-only cache while still bounding hostile files. */ +const MAX_CACHE_BYTES = 4 * 1024 + +/** Stable SemVer, with optional build metadata that does not affect precedence. */ +const STABLE_VERSION_PATTERN = + /^(0|[1-9]\d*)\.(0|[1-9]\d*)\.(0|[1-9]\d*)(?:\+[0-9A-Za-z-]+(?:\.[0-9A-Za-z-]+)*)?$/ + +type StableVersion = readonly [major: number, minor: number, patch: number] + +/** Parses only stable versions because prerelease installations are never notified. */ +function parseStableVersion(version: string): StableVersion | null { + const match = STABLE_VERSION_PATTERN.exec(version) + if (!match) return null + const parsed = [Number(match[1]), Number(match[2]), Number(match[3])] as const + return parsed.every(Number.isSafeInteger) ? parsed : null +} + +function isNewerVersion(candidate: StableVersion, current: StableVersion): boolean { + if (candidate[0] !== current[0]) return candidate[0] > current[0] + if (candidate[1] !== current[1]) return candidate[1] > current[1] + return candidate[2] > current[2] +} + +/** Covers CI jobs that allocate a terminal despite being non-interactive. */ const CI_VARIABLES = [ 'CI', 'GITHUB_ACTIONS', @@ -60,24 +77,11 @@ const CI_VARIABLES = [ 'BUILDKITE', ] as const -/** The shape written to `~/.sim/update-check.json`. */ +/** The shape written to the update cache. */ interface UpdateCacheEntry { - /** - * Forward compatibility hinge. A reader that does not recognise the number - * treats the file as absent and checks again, so an older CLI can never be - * confused by a newer one's cache — and neither ever has to migrate it. - */ + /** Unknown cache versions are treated as absent. */ version: 1 checkedAt: string - /** - * What the last check found, recorded so `cat ~/.sim/update-check.json` - * answers a support question without re-running anything. - * - * Deliberately NOT served: announcing from the cache would put the notice in - * front of every command for the rest of the day, and the contract is one - * notice per day. Only `checkedAt` decides whether the check runs. - */ - latestVersion: string | null } const CACHE_VERSION = 1 @@ -90,20 +94,22 @@ export interface UpdateCheckOptions { /** Location of the running module, used to recognise npx and local builds. */ modulePath?: string now?: Date + /** Registry transport. Injectable so network behavior can be tested without global state. */ + registryRequest?: RegistryRequest write?: (message: string) => void } -/** - * One notice per process, no matter how the hook is reached. Commander runs a - * single action per parse, so this is belt and braces rather than load-bearing. - */ -let announced = false - -/** Test seam: the guard above is process-global, and each test needs it clear. */ -export function resetUpdateCheck(): void { - announced = false +interface RegistryRequestOptions { + headers: Record + maxResponseBytes: number + timeoutMs: number } +type RegistryRequest = (url: URL, options: RegistryRequestOptions) => Promise + +/** Makes adjacent temporary files unique across writes in this process. */ +let cacheWriteSequence = 0 + /** Anything but unset, empty, `0` or `false` turns a switch on. */ function isEnabled(value: string | undefined): boolean { if (value === undefined) return false @@ -111,30 +117,13 @@ function isEnabled(value: string | undefined): boolean { return normalized !== '' && normalized !== '0' && normalized !== 'false' } -/** - * Whether this installation is one a "please upgrade" line cannot help. - * - * `npx` resolves the dist-tag on every invocation, so its user is by definition - * already current. A checkout is the sharper case: the repo manifest trails npm - * permanently and by design, because the publish workflow bumps the version - * in-job under `permissions: contents: read` and never commits it back. Without - * this, every Sim engineer running a local build would be told daily to upgrade - * to a version their own tree already contains. - */ +/** Skips npx, which resolves latest, and checkouts, whose manifest trails npm. */ function isUnadvisableInstall(modulePath: string): boolean { const normalized = normalizeModulePath(modulePath) return normalized.includes('/_npx/') || normalized.includes('/packages/sim-cli/') } -/** - * One normalisation for every decision made about the module path. - * - * Both readers here match path fragments, and they must agree: normalising - * separators in one and separators-plus-case in the other silently disagrees on - * Windows and on case-insensitive macOS volumes, where a checkout under - * `...\\Packages\\Sim-Cli\\` is a checkout to one reader and a global install to - * the other. - */ +/** Normalizes separators and case before installation-path comparisons. */ function normalizeModulePath(modulePath: string): string { return modulePath.replace(/\\/g, '/').toLowerCase() } @@ -142,63 +131,122 @@ function normalizeModulePath(modulePath: string): string { /** * The full dist-tags URL, honouring a configured mirror. * - * `npm_config_registry` is only set when the CLI is invoked through a - * package-manager script, so this is inconsistent by nature — but the case it - * rescues is real: behind a mirror with `registry.npmjs.org` firewalled, the - * default would fail forever while the mirror holds the right answer. - * - * The mirror's own path and query are preserved rather than resolved away. - * `new URL(relative, base)` would discard both, and a token-authenticated - * Artifactory or Nexus base (`https://host/api/npm/repo?token=...`) is a - * realistic configuration that would otherwise be silently rewritten into a - * request the mirror answers with a 404. - * - * `.npmrc` is deliberately not parsed. That is an INI format with scopes and - * auth tokens, and reading it is where a courtesy check would start growing. + * A configured private registry keeps its path and query. `.npmrc` is not read; + * supporting its scoped configuration and auth is outside this courtesy check. */ -function registryUrl(env: NodeJS.ProcessEnv): URL { +function registryUrl(env: NodeJS.ProcessEnv): URL | null { const fallback = new URL(DIST_TAGS_PATH, DEFAULT_REGISTRY) const configured = env.npm_config_registry?.trim() if (!configured) return fallback try { const base = new URL(configured) - if (base.protocol !== 'http:' && base.protocol !== 'https:') return fallback + if (base.protocol !== 'http:' && base.protocol !== 'https:') return null + if (base.username || base.password) return null base.pathname = `${base.pathname.replace(/\/$/, '')}/${DIST_TAGS_PATH}` return base } catch { - // An unparseable value is not worth reporting; the default still works. - return fallback + return null } } -/** - * Reads a response body under a hard byte budget. - * - * `response.json()` would buffer whatever arrives, bounded only by the abort - * timeout — long enough for a mirror on a fast link to push far more than this - * endpoint could legitimately return. - */ -async function readCapped(response: Response, limit: number): Promise { +const REGISTRY_REQUEST_SCRIPT = ` +let input = '' +process.stdin.setEncoding('utf8') +for await (const chunk of process.stdin) input += chunk + +try { + const { url, headers, maxResponseBytes, timeoutMs } = JSON.parse(input) + const deadline = setTimeout(() => process.exit(1), timeoutMs) + const response = await fetch(url, { headers, redirect: 'error' }) const declared = Number(response.headers.get('content-length')) - if (Number.isFinite(declared) && declared > limit) return null - if (!response.body) return null + + if (!response.ok || !response.body || (Number.isFinite(declared) && declared > maxResponseBytes)) { + process.exit(1) + } const reader = response.body.getReader() - const decoder = new TextDecoder() - let text = '' + const chunks = [] let seen = 0 - try { - while (true) { - const { done, value } = await reader.read() - if (done) break - seen += value.byteLength - if (seen > limit) return null - text += decoder.decode(value, { stream: true }) + + while (true) { + const { done, value } = await reader.read() + if (done) break + seen += value.byteLength + if (seen > maxResponseBytes) { + process.exit(1) } - } finally { - void reader.cancel().catch(() => {}) + chunks.push(Buffer.from(value)) + } + + clearTimeout(deadline) + process.stdout.write(Buffer.concat(chunks), () => process.exit(0)) +} catch { + process.exit(1) +} +` + +/** Preserves proxy/TLS settings without copying the registry credential into the probe. */ +function registryProcessEnv(): NodeJS.ProcessEnv { + const env = { ...process.env } + for (const key of Object.keys(env)) { + if (key.toLowerCase() === 'npm_config_registry') delete env[key] } - return text + decoder.decode() + return env +} + +/** + * Makes one request in a process whose lifetime is owned entirely by this check. + * + * Neither a Fetch abort nor `ClientRequest.destroy()` can cancel every pending + * operation: Undici may retain a connection attempt, and the native client + * cannot cancel an OS `dns.lookup()`. Terminating this child at the deadline + * closes both escape hatches. Input travels over stdin rather than argv or the + * environment so a configured registry credential cannot appear in a process + * listing. + */ +function requestRegistry( + url: URL, + { headers, maxResponseBytes, timeoutMs }: RegistryRequestOptions +): Promise { + return new Promise((resolve, reject) => { + const proxyArguments = process.execArgv.filter( + (argument) => argument === '--use-env-proxy' || argument === '--no-use-env-proxy' + ) + const child = spawn( + process.execPath, + [...proxyArguments, '--input-type=module', '--eval', REGISTRY_REQUEST_SCRIPT], + { + env: registryProcessEnv(), + killSignal: 'SIGKILL', + stdio: ['pipe', 'pipe', 'ignore'], + timeout: timeoutMs, + windowsHide: true, + } + ) + const chunks: Buffer[] = [] + let failed = false + let seen = 0 + + child.stdout.on('data', (chunk: Buffer) => { + seen += chunk.byteLength + if (seen > maxResponseBytes) { + failed = true + child.kill('SIGKILL') + return + } + chunks.push(chunk) + }) + child.stdout.on('error', () => { + failed = true + child.kill('SIGKILL') + }) + child.stdin.on('error', () => {}) + child.once('error', reject) + child.once('close', (code) => { + resolve(code === 0 && !failed ? Buffer.concat(chunks).toString('utf8') : null) + }) + child.stdin.end(JSON.stringify({ headers, maxResponseBytes, timeoutMs, url: url.href })) + }) } /** @@ -212,22 +260,21 @@ async function readCapped(response: Response, limit: number): Promise | null> { +async function fetchDistTags( + env: NodeJS.ProcessEnv, + request: RegistryRequest +): Promise | null> { try { - const response = await fetch(registryUrl(env), { + const url = registryUrl(env) + if (!url) return null + const text = await request(url, { headers: { accept: 'application/json', 'user-agent': `${PACKAGE_NAME}-cli/${CLI_VERSION}` }, - // The endpoint does not redirect in normal operation, so refusing to - // follow costs nothing and keeps the request on the host that was asked. - redirect: 'error', - signal: AbortSignal.timeout(REGISTRY_TIMEOUT_MS), + maxResponseBytes: MAX_RESPONSE_BYTES, + timeoutMs: REGISTRY_TIMEOUT_MS, }) - if (!response.ok) return null - const text = await readCapped(response, MAX_RESPONSE_BYTES) - if (text === null) return null + if (text === null || Buffer.byteLength(text) > MAX_RESPONSE_BYTES) return null const body: unknown = JSON.parse(text) if (typeof body !== 'object' || body === null || Array.isArray(body)) return null - // Some proxies answer with an HTML error page under a 200, so the values are - // checked rather than assumed. const tags: Record = {} for (const [tag, version] of Object.entries(body)) { if (typeof version === 'string') tags[tag] = version @@ -239,8 +286,31 @@ async function fetchDistTags(env: NodeJS.ProcessEnv): Promise MAX_CACHE_BYTES) { + return null + } + + const buffer = Buffer.allocUnsafe(MAX_CACHE_BYTES + 1) + let bytesRead = 0 + while (bytesRead < buffer.byteLength) { + const count = readSync( + descriptor, + buffer, + bytesRead, + buffer.byteLength - bytesRead, + bytesRead + ) + if (count === 0) break + bytesRead += count + } + if (bytesRead > MAX_CACHE_BYTES) return null + + const parsed: unknown = JSON.parse(buffer.subarray(0, bytesRead).toString('utf8')) if (typeof parsed !== 'object' || parsed === null) return null const entry = parsed as Partial if (entry.version !== CACHE_VERSION) return null @@ -249,12 +319,15 @@ function readCache(path: string): UpdateCacheEntry | null { return { version: CACHE_VERSION, checkedAt: entry.checkedAt, - latestVersion: typeof entry.latestVersion === 'string' ? entry.latestVersion : null, } } catch { - // Absent, unreadable, or truncated by an interleaved writer — all of which - // mean the same thing here: check again. return null + } finally { + if (descriptor !== null) { + try { + closeSync(descriptor) + } catch {} + } } } @@ -264,28 +337,40 @@ function readCache(path: string): UpdateCacheEntry | null { * Stamping on failure too is what keeps a blackholed registry costing one second * a day instead of one second per command. * - * There is no temp-file-and-rename. Two concurrent invocations can interleave - * and truncate this file; the reader treats a truncated file as no cache, so the - * whole cost of the race is one extra HTTP request. + * Failures are ignored because the cache is best-effort. An exclusive adjacent + * temporary file makes replacement atomic without modifying a linked target. */ function writeCache(path: string, entry: UpdateCacheEntry): void { + let descriptor: number | null = null + let temporaryCreated = false + const temporaryPath = `${path}.${process.pid}.${Date.now()}.${cacheWriteSequence++}.tmp` try { mkdirSync(dirname(path), { recursive: true, mode: 0o700 }) - writeFileSync(path, `${JSON.stringify(entry, null, 2)}\n`, { mode: 0o644 }) + descriptor = openSync(temporaryPath, 'wx', 0o644) + temporaryCreated = true + writeFileSync(descriptor, `${JSON.stringify(entry, null, 2)}\n`) + closeSync(descriptor) + descriptor = null + renameSync(temporaryPath, path) + temporaryCreated = false } catch { - // A read-only home directory is ordinary in a container, and it must not - // stop the command the user actually ran. The cost is that the throttle - // cannot persist there, so such an installation re-checks once per - // invocation instead of once per day — still bounded by the timeout. + } finally { + if (descriptor !== null) { + try { + closeSync(descriptor) + } catch {} + } + if (temporaryCreated) { + try { + unlinkSync(temporaryPath) + } catch {} + } } } -/** Whether a recorded check is recent enough to skip this one. */ +/** Treats future timestamps as stale in case the clock moved backward. */ function isFresh(entry: UpdateCacheEntry, now: Date): boolean { const age = now.getTime() - Date.parse(entry.checkedAt) - // A negative age means the clock moved backwards since the write. Treating it - // as fresh would suppress the notice until the clock caught up, which after a - // one-off jump forward is forever. return age >= 0 && age < CHECK_INTERVAL_MS } @@ -297,11 +382,10 @@ function isFresh(entry: UpdateCacheEntry, now: Date): boolean { * describes nothing but the shell that happened to invoke it. */ export function upgradeCommand( - channel: ReleaseChannel, modulePath: string = fileURLToPath(import.meta.url), env: NodeJS.ProcessEnv = process.env ): string { - const target = `${PACKAGE_NAME}@${channel}` + const target = `${PACKAGE_NAME}@latest` const normalized = normalizeModulePath(modulePath) if (normalized.includes('.bun/install/global')) return `bun add -g ${target}` @@ -321,7 +405,7 @@ export function upgradeCommand( } /** - * Tells the user once a day when the channel they installed from has moved on. + * Uses a daily cache before telling the user their installation is out of date. * * Wired as a root `preAction` hook rather than a teardown in the entrypoint for * two structural reasons: commander answers `--help` and `--version` during @@ -329,61 +413,43 @@ export function upgradeCommand( * invocations are excluded by construction rather than by a check; and some * commands call `process.exit` directly, which a `finally` would never see. * - * Never throws. The caller is a hook in front of the user's actual command. + * Never throws, writes only plain text to stderr, and stays silent when stderr + * is redirected. The caller runs this before the user's actual command. */ export async function announceUpdateIfAvailable(options: UpdateCheckOptions = {}): Promise { try { - if (announced) return - const env = options.env ?? process.env const isTty = options.isTty ?? process.stderr.isTTY === true const modulePath = options.modulePath ?? fileURLToPath(import.meta.url) const now = options.now ?? new Date() if (isEnabled(env.SIM_NO_UPDATE_CHECK)) return - // stderr is where this goes, so a redirected stderr means it would land in a - // log file or a pipeline rather than in front of a person. if (!isTty) return if (CI_VARIABLES.some((variable) => isEnabled(env[variable]))) return if (isUnadvisableInstall(modulePath)) return const currentVersion = options.currentVersion ?? CLI_VERSION - const current = parseVersion(currentVersion) + const current = parseStableVersion(currentVersion) if (!current) return - const channel = channelOf(current) - // Prereleases publish on every push to their branch, so telling a prerelease - // user to upgrade would be both correct and useless — the advice is stale - // again within the hour, and they opted into moving fast in the first place. - if (channel !== 'latest') return - const cachePath = updateCachePath() const cached = readCache(cachePath) if (cached && isFresh(cached, now)) return - const tags = await fetchDistTags(env) - const latest = tags?.[channel] ?? null - // Parse before persisting: the value came off the network, and nothing - // unvalidated should reach the disk or, later, the terminal. - const available = latest ? parseVersion(latest) : null + const tags = await fetchDistTags(env, options.registryRequest ?? requestRegistry) + const latest = tags?.latest ?? null + const available = latest ? parseStableVersion(latest) : null writeCache(cachePath, { version: CACHE_VERSION, checkedAt: now.toISOString(), - latestVersion: available ? latest : null, }) if (!latest || !available) return - if (compareVersions(available, current) <= 0) return + if (!isNewerVersion(available, current)) return - announced = true const write = options.write ?? ((message: string) => void process.stderr.write(message)) - // Unstyled on purpose: chalk decides on stdout, so `sim workflows list | jq` - // from a terminal would silently drop the colour here even though stderr is - // still a terminal. Plain text is also the whole answer to NO_COLOR. write( - `Update available: sim ${currentVersion} → ${latest}. Run: ${upgradeCommand(channel, modulePath, env)}\n` + `Update available: sim ${currentVersion} → ${latest}. Run: ${upgradeCommand(modulePath, env)}\n` ) - } catch { - // Nothing this function does is worth failing a command over. - } + } catch {} } diff --git a/packages/sim-cli/src/update/semver.test.ts b/packages/sim-cli/src/update/semver.test.ts deleted file mode 100644 index 725cdb163a9..00000000000 --- a/packages/sim-cli/src/update/semver.test.ts +++ /dev/null @@ -1,117 +0,0 @@ -/** - * @vitest-environment node - */ -import { describe, expect, it } from 'vitest' -import { channelOf, compareVersions, parseVersion } from './semver' - -function parsed(version: string) { - const result = parseVersion(version) - if (!result) throw new Error(`fixture "${version}" should parse`) - return result -} - -function order(left: string, right: string): number { - return Math.sign(compareVersions(parsed(left), parsed(right))) -} - -describe('parsing a published version', () => { - it('reads the release triple', () => { - expect(parseVersion('2.1.5')).toEqual({ major: 2, minor: 1, patch: 5, prerelease: [] }) - }) - - it('splits a prerelease into identifiers, keeping numeric ones numeric', () => { - expect(parseVersion('2.1.3-preview.812.1')?.prerelease).toEqual(['preview', 812, 1]) - }) - - it('ignores build metadata, which carries no precedence', () => { - expect(parseVersion('2.1.5+20260902')).toEqual(parseVersion('2.1.5')) - }) - - it.each([ - ['2.1', 'an incomplete triple'], - ['v2.1.2', 'a leading v, which npm does not publish'], - ['2.1.2.3', 'a fourth component'], - ['01.2.3', 'a leading zero'], - ['2.1.2-', 'an empty prerelease'], - ['2.1.2-preview..1', 'an empty identifier'], - ['2.1.3-preview.09', 'a zero-padded numeric identifier'], - ['2.1.3-01', 'a zero-padded identifier on its own'], - ['', 'nothing at all'], - ['latest', 'a dist-tag mistaken for a version'], - ])('rejects %s (%s)', (version) => { - expect(parseVersion(version)).toBeNull() - }) - - it('rejects a component too large to compare exactly', () => { - expect(parseVersion('9007199254740993.0.0')).toBeNull() - }) -}) - -describe('precedence', () => { - it('orders the release triple before anything else', () => { - expect(order('2.1.2', '2.1.3')).toBe(-1) - expect(order('2.1.9', '2.2.0')).toBe(-1) - expect(order('2.9.9', '3.0.0')).toBe(-1) - expect(order('2.1.5', '2.1.5')).toBe(0) - }) - - it('ranks a prerelease below the release it leads to', () => { - expect(order('2.1.3-preview.44.1', '2.1.3')).toBe(-1) - }) - - it('ranks a release above a prerelease of the same triple', () => { - // The mirror of the case above, and a distinct branch: it is the only way - // to reach the comparison with an empty prerelease list on the left. - expect(order('2.1.3', '2.1.3-preview.44.1')).toBe(1) - }) - - it('does not let a malformed identifier outrank every number', () => { - // `09` is not a valid numeric identifier. Accepting it would reclassify it - // as alphanumeric, and alphanumerics outrank numbers — so `preview.010` - // would sort above `preview.2`. - expect(parseVersion('2.1.3-preview.010')).toBeNull() - }) - - it('orders two alphanumeric identifiers by ASCII', () => { - expect(order('2.1.3-alpha', '2.1.3-beta')).toBe(-1) - expect(order('2.1.3-beta', '2.1.3-alpha')).toBe(1) - }) - - it('compares numeric identifiers as numbers, not as text', () => { - // The case a string comparison gets backwards: run 9 precedes run 44, but - // "44" sorts before "9" lexicographically. - expect(order('2.1.3-preview.9.1', '2.1.3-preview.44.1')).toBe(-1) - }) - - it('ranks a numeric identifier below an alphanumeric one', () => { - expect(order('2.1.3-1', '2.1.3-alpha')).toBe(-1) - expect(order('2.1.3-alpha', '2.1.3-1')).toBe(1) - }) - - it('ranks a shorter identifier list below a longer one sharing its prefix', () => { - expect(order('2.1.3-preview.1', '2.1.3-preview.1.2')).toBe(-1) - }) - - it('ranks a stable release below a prerelease of a later patch', () => { - // 2.1.2 really is older than 2.1.3-preview.44.1. The guarantee that this - // never reaches a user as "upgrade" lives in check.ts, which returns before - // comparing anything on a non-stable channel — see check.test.ts's "says - // nothing to a prerelease install". - expect(order('2.1.2', '2.1.3-preview.44.1')).toBe(-1) - }) -}) - -describe('the channel a version was published under', () => { - it('reads a stable release as the latest tag', () => { - expect(channelOf(parsed('2.1.5'))).toBe('latest') - }) - - it('reads the two prerelease tags the publish workflow produces', () => { - expect(channelOf(parsed('2.1.6-preview.812.1'))).toBe('staging') - expect(channelOf(parsed('2.1.6-dev.812.1'))).toBe('dev') - }) - - it('refuses to guess a channel for a prerelease tag we do not publish', () => { - expect(channelOf(parsed('2.1.6-rc.1'))).toBeNull() - }) -}) diff --git a/packages/sim-cli/src/update/semver.ts b/packages/sim-cli/src/update/semver.ts deleted file mode 100644 index ea0e451ae9d..00000000000 --- a/packages/sim-cli/src/update/semver.ts +++ /dev/null @@ -1,144 +0,0 @@ -/** - * The version arithmetic the update check needs, and nothing more. - * - * The package deliberately carries no `semver` dependency: everything here is - * bundled into `dist/index.js`, and a full implementation would be several - * hundred kilobytes to answer one question once a day. What is implemented is - * the precedence half of the specification — enough to order two published - * versions — not ranges, not coercion, not satisfaction. - */ - -/** - * `X.Y.Z`, an optional prerelease, an optional build. Leading zeroes are - * rejected the way the specification rejects them, so a hand-edited `01.2.3` - * reads as unparseable rather than as `1.2.3`. - */ -const VERSION_PATTERN = - /^(0|[1-9]\d*)\.(0|[1-9]\d*)\.(0|[1-9]\d*)(?:-([0-9A-Za-z-]+(?:\.[0-9A-Za-z-]+)*))?(?:\+[0-9A-Za-z-]+(?:\.[0-9A-Za-z-]+)*)?$/ - -/** A prerelease identifier that is all digits compares as a number. */ -const NUMERIC_IDENTIFIER = /^(0|[1-9]\d*)$/ - -/** - * A numeric prerelease identifier carrying a leading zero, which the - * specification forbids. It has to be spotted rather than simply failing - * `NUMERIC_IDENTIFIER`: falling through would silently reclassify `09` as an - * alphanumeric identifier, and alphanumerics outrank every number — so - * `preview.010` would sort above `preview.2`. - */ -const LEADING_ZERO_IDENTIFIER = /^0\d+$/ - -export interface ParsedVersion { - major: number - minor: number - patch: number - /** Dot-separated prerelease identifiers, empty for a stable release. */ - prerelease: readonly (string | number)[] -} - -/** - * The three dist-tags `.github/workflows/publish-sim-cli.yml` publishes under. - * - * A channel is inferred from the version's own prerelease tag rather than - * remembered, because the running CLI knows its version and nothing else about - * how it was installed. - */ -export type ReleaseChannel = 'latest' | 'staging' | 'dev' - -/** - * Parses a published version, or returns null for anything else. - * - * Never throws: every caller is on a path that must stay silent, and a local - * build with a hand-mangled manifest is a normal thing to encounter rather than - * an error to report. - */ -export function parseVersion(version: string): ParsedVersion | null { - const match = VERSION_PATTERN.exec(version) - if (!match) return null - - const major = Number(match[1]) - const minor = Number(match[2]) - const patch = Number(match[3]) - if ( - !Number.isSafeInteger(major) || - !Number.isSafeInteger(minor) || - !Number.isSafeInteger(patch) - ) { - return null - } - - const identifiers = match[4] ? match[4].split('.') : [] - if (identifiers.some((identifier) => LEADING_ZERO_IDENTIFIER.test(identifier))) return null - - const prerelease = identifiers.map((identifier) => - NUMERIC_IDENTIFIER.test(identifier) ? Number(identifier) : identifier - ) - if ( - prerelease.some( - (identifier) => typeof identifier === 'number' && !Number.isSafeInteger(identifier) - ) - ) { - return null - } - - return { major, minor, patch, prerelease } -} - -/** - * Compares two prerelease identifier lists by the specification's rules. - * - * Numeric identifiers compare numerically and sort below alphanumeric ones, and - * a shorter list sorts below a longer one that shares its prefix. The numeric - * rule is the one a string comparison gets wrong: `preview.9` is *older* than - * `preview.44`, which run number ordering depends on. - */ -function comparePrerelease( - left: readonly (string | number)[], - right: readonly (string | number)[] -): number { - // A version carrying a prerelease ranks below the same version without one. - if (left.length === 0) return right.length === 0 ? 0 : 1 - if (right.length === 0) return -1 - - for (let index = 0; index < Math.min(left.length, right.length); index += 1) { - const a = left[index] - const b = right[index] - if (a === b) continue - if (typeof a === 'number' && typeof b === 'number') return a - b - if (typeof a === 'number') return -1 - if (typeof b === 'number') return 1 - return a < b ? -1 : 1 - } - return left.length - right.length -} - -/** - * Semver precedence: negative when left is older, zero when equal, positive - * when left is newer. Build metadata is ignored, as the specification requires. - */ -export function compareVersions(left: ParsedVersion, right: ParsedVersion): number { - if (left.major !== right.major) return left.major - right.major - if (left.minor !== right.minor) return left.minor - right.minor - if (left.patch !== right.patch) return left.patch - right.patch - return comparePrerelease(left.prerelease, right.prerelease) -} - -/** - * The dist-tag a version was published under, or null when its prerelease tag - * is not one this project publishes. - * - * The caller compares a version only against its own channel's tag. Today it - * acts on `latest` alone and returns early for everything else, so a `-preview` - * install is compared against nothing at all — which is what makes it - * impossible to advise "upgrade" to a stable version older than the prerelease - * already installed. Naming the channel rather than answering a bare - * is-this-stable keeps that guarantee legible, and is what a future decision to - * notify prerelease users would extend. - */ -export function channelOf(version: ParsedVersion): ReleaseChannel | null { - if (version.prerelease.length === 0) return 'latest' - const [tag] = version.prerelease - if (tag === 'preview') return 'staging' - if (tag === 'dev') return 'dev' - return null -} From 4c24d3a76e0def3a86ff60bfcec5d7f16ce5c320 Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Thu, 3 Sep 2026 10:17:39 -0700 Subject: [PATCH 7/8] test(cli): isolate update checks from CI markers --- packages/sim-cli/src/update/check.process.test.ts | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/packages/sim-cli/src/update/check.process.test.ts b/packages/sim-cli/src/update/check.process.test.ts index 63ef00597d0..162fe8fd35c 100644 --- a/packages/sim-cli/src/update/check.process.test.ts +++ b/packages/sim-cli/src/update/check.process.test.ts @@ -81,8 +81,13 @@ function runChild( { env: { ...process.env, + BUILDKITE: '0', + CI: '0', + GITHUB_ACTIONS: '0', + JENKINS_URL: '0', NODE_USE_ENV_PROXY: '0', NO_PROXY: '127.0.0.1,localhost', + TEAMCITY_VERSION: '0', ...options.env, SIM_CONFIG_DIR: configDir, ...(options.useProcessEnv ? { npm_config_registry: registry } : {}), From c22df05921eb8e44bac0126ee6b4ee533e551dfe Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Thu, 3 Sep 2026 10:50:05 -0700 Subject: [PATCH 8/8] fix(cli): tighten update check eligibility --- apps/docs/content/docs/cli/configuration.mdx | 8 +++-- .../docs/content/docs/cli/troubleshooting.mdx | 2 +- packages/sim-cli/README.md | 7 ++-- .../sim-cli/src/update/check.process.test.ts | 15 ++++++--- packages/sim-cli/src/update/check.test.ts | 28 +++++++++++++++- packages/sim-cli/src/update/check.ts | 32 +++++++++++++++---- 6 files changed, 73 insertions(+), 19 deletions(-) diff --git a/apps/docs/content/docs/cli/configuration.mdx b/apps/docs/content/docs/cli/configuration.mdx index b9b9a68c4a7..8758ab51859 100644 --- a/apps/docs/content/docs/cli/configuration.mdx +++ b/apps/docs/content/docs/cli/configuration.mdx @@ -147,8 +147,9 @@ containing username/password userinfo, such as `https://user:password@registry.example`, is rejected and no update check is made. -Malformed and non-HTTP(S) configured registry values also disable the update -check rather than making an unexpected request to the public registry. +An empty or whitespace-only `npm_config_registry` is treated as unset, so the +public registry remains the default. Non-empty malformed and non-HTTP(S) values +disable the update check rather than making an unexpected public request. The notice is skipped entirely when: @@ -156,7 +157,8 @@ The notice is skipped entirely when: - stderr is not a terminal, so redirected and piped output is never affected - a CI environment variable is present (`CI`, `GITHUB_ACTIONS`, `JENKINS_URL`, `TEAMCITY_VERSION`, `BUILDKITE`) -- the CLI is running under `npx`, which resolves the newest version every time +- the CLI is running under `npm exec` or `npx`, which may use a project-local or + ephemeral package where global-install advice is inappropriate - the CLI is running from a checkout of the sim repository, whose version deliberately trails the published one - the installed version is a prerelease diff --git a/apps/docs/content/docs/cli/troubleshooting.mdx b/apps/docs/content/docs/cli/troubleshooting.mdx index c18a06ff5a2..b429a455b4a 100644 --- a/apps/docs/content/docs/cli/troubleshooting.mdx +++ b/apps/docs/content/docs/cli/troubleshooting.mdx @@ -120,7 +120,7 @@ one installs a second copy instead of replacing the executable on your `PATH`: The CLI can also tell you this through a cached daily check on eligible invocations, and the command it prints already matches your installation. It -stays quiet when stderr is redirected, in CI, and under `npx`. +stays quiet when stderr is redirected, in CI, and under `npm exec` or `npx`. ## An update notice appears in output I am parsing diff --git a/packages/sim-cli/README.md b/packages/sim-cli/README.md index 2a5f4aac220..ccb6381cc45 100644 --- a/packages/sim-cli/README.md +++ b/packages/sim-cli/README.md @@ -272,9 +272,10 @@ Apart from the configured registry URL, it sends only its own version and never your Sim API key. If `npm_config_registry` points at a private mirror, its query string is preserved, including any query-string credentials. Registry URLs containing username/password userinfo are rejected. Set -`SIM_NO_UPDATE_CHECK=1` to turn it off; malformed or non-HTTP(S) configured -registry values also fail closed. The full list of cases where it stays quiet -is in the [configuration guide](https://docs.sim.ai/cli/configuration). +`SIM_NO_UPDATE_CHECK=1` to turn it off. Empty or whitespace-only registry values +use the public default; non-empty malformed or non-HTTP(S) values fail closed. +The full list of cases where it stays quiet is in the +[configuration guide](https://docs.sim.ai/cli/configuration). ## Documentation diff --git a/packages/sim-cli/src/update/check.process.test.ts b/packages/sim-cli/src/update/check.process.test.ts index 162fe8fd35c..03972c4c5d1 100644 --- a/packages/sim-cli/src/update/check.process.test.ts +++ b/packages/sim-cli/src/update/check.process.test.ts @@ -87,6 +87,7 @@ function runChild( JENKINS_URL: '0', NODE_USE_ENV_PROXY: '0', NO_PROXY: '127.0.0.1,localhost', + npm_command: '', TEAMCITY_VERSION: '0', ...options.env, SIM_CONFIG_DIR: configDir, @@ -364,11 +365,12 @@ it.skipIf(!SUPPORTS_PROXY_FLAG)( 10_000 ) -it('keeps a configured registry credential out of probe argv and environment', async () => { +it('keeps CLI credentials out of probe argv and environment', async () => { const inspectionPath = join(temporaryDir, 'probe-inspection.json') const preloadDir = join(temporaryDir, 'probe preload') const preloadPath = join(preloadDir, 'inspect-probe.cjs') - const sentinel = 'registry-secret-sentinel' + const registrySentinel = 'registry-secret-sentinel' + const apiKeySentinel = 'api-key-secret-sentinel' let requestPath: string | undefined mkdirSync(preloadDir) writeFileSync( @@ -395,24 +397,27 @@ it('keeps a configured registry credential out of probe argv and environment', a response.end(JSON.stringify({ latest: '2.1.5' })) }, async (origin) => { - const registry = `${origin}?token=${sentinel}` + const registry = `${origin}?token=${registrySentinel}` const result = await runChild(entrypoint, registry, join(temporaryDir, 'config-credential'), { env: { NODE_OPTIONS: `--require="${preloadPath}"`, NPM_CONFIG_REGISTRY: registry, PROBE_INSPECTION_PATH: inspectionPath, + SIM_API_KEY: apiKeySentinel, }, useProcessEnv: true, }) expect(result).toMatchObject({ code: 0, signal: null, stderr: '' }) - expect(requestPath).toBe(`/-/package/sim/dist-tags?token=${sentinel}`) + expect(requestPath).toBe(`/-/package/sim/dist-tags?token=${registrySentinel}`) const inspection = JSON.parse(readFileSync(inspectionPath, 'utf8')) as { argv: string[] environmentValues: string[] execArgv: string[] } - expect(JSON.stringify(inspection)).not.toContain(sentinel) + const serializedInspection = JSON.stringify(inspection) + expect(serializedInspection).not.toContain(registrySentinel) + expect(serializedInspection).not.toContain(apiKeySentinel) } ) }, 10_000) diff --git a/packages/sim-cli/src/update/check.test.ts b/packages/sim-cli/src/update/check.test.ts index 2eec8e75c90..9400d1f6a43 100644 --- a/packages/sim-cli/src/update/check.test.ts +++ b/packages/sim-cli/src/update/check.test.ts @@ -177,8 +177,34 @@ describe('when the notice is suppressed', () => { it.each([ '/Users/x/.npm/_npx/a1b2/node_modules/sim/dist/index.js', 'C:\\Users\\x\\AppData\\Local\\npm-cache\\_npx\\a1b2\\node_modules\\sim\\dist\\index.js', - ])('says nothing under npx, which resolves the tag on every run (%s)', async (modulePath) => { + ])('says nothing for an npx cache installation (%s)', async (modulePath) => { await run({ modulePath }) + expect(fetched).toEqual([]) + expect(notices).toEqual([]) + }) + + it.each([ + '/Users/x/project/node_modules/sim/dist/index.js', + 'C:\\Users\\x\\project\\node_modules\\sim\\dist\\index.js', + ])('says nothing when npm exec resolves a project-local dependency (%s)', async (modulePath) => { + await run({ env: { npm_command: 'exec' }, modulePath }) + expect(fetched).toEqual([]) + expect(notices).toEqual([]) + }) + + it.each([ + { + cwd: '/Users/x/project/packages/app', + modulePath: '/Users/x/project/node_modules/sim/dist/index.js', + }, + { + cwd: 'C:\\Users\\x\\project\\packages\\app', + modulePath: + 'C:\\Users\\x\\project\\node_modules\\.pnpm\\sim@2.1.2\\node_modules\\sim\\dist\\index.js', + }, + ])('says nothing from a project-local install at $modulePath', async ({ cwd, modulePath }) => { + await run({ cwd, modulePath }) + expect(fetched).toEqual([]) expect(notices).toEqual([]) }) diff --git a/packages/sim-cli/src/update/check.ts b/packages/sim-cli/src/update/check.ts index 330f8b24e75..900ef4752e7 100644 --- a/packages/sim-cli/src/update/check.ts +++ b/packages/sim-cli/src/update/check.ts @@ -87,6 +87,8 @@ interface UpdateCacheEntry { const CACHE_VERSION = 1 export interface UpdateCheckOptions { + /** Current working directory. Injected so project-local installation detection is testable. */ + cwd?: string currentVersion?: string env?: NodeJS.ProcessEnv /** Whether stderr is a terminal. Injected so the suppression rule is testable. */ @@ -117,10 +119,26 @@ function isEnabled(value: string | undefined): boolean { return normalized !== '' && normalized !== '0' && normalized !== 'false' } -/** Skips npx, which resolves latest, and checkouts, whose manifest trails npm. */ -function isUnadvisableInstall(modulePath: string): boolean { +/** Whether the package is installed in a node_modules tree above the working directory. */ +function isProjectLocalInstall(modulePath: string, cwd: string): boolean { + const normalizedModulePath = normalizeModulePath(modulePath) + const nodeModulesIndex = normalizedModulePath.indexOf('/node_modules/') + if (nodeModulesIndex < 0) return false + + const installRoot = normalizedModulePath.slice(0, nodeModulesIndex) + const workingDirectory = normalizeModulePath(cwd).replace(/\/+$/, '') + return workingDirectory === installRoot || workingDirectory.startsWith(`${installRoot}/`) +} + +/** Skips ephemeral, project-local, and checkout installs that global advice cannot update. */ +function isUnadvisableInstall(modulePath: string, env: NodeJS.ProcessEnv, cwd: string): boolean { const normalized = normalizeModulePath(modulePath) - return normalized.includes('/_npx/') || normalized.includes('/packages/sim-cli/') + return ( + env.npm_command === 'exec' || + normalized.includes('/_npx/') || + normalized.includes('/packages/sim-cli/') || + isProjectLocalInstall(modulePath, cwd) + ) } /** Normalizes separators and case before installation-path comparisons. */ @@ -185,11 +203,12 @@ try { } ` -/** Preserves proxy/TLS settings without copying the registry credential into the probe. */ +/** Preserves proxy/TLS settings without copying CLI credentials into the probe. */ function registryProcessEnv(): NodeJS.ProcessEnv { const env = { ...process.env } for (const key of Object.keys(env)) { - if (key.toLowerCase() === 'npm_config_registry') delete env[key] + const normalized = key.toLowerCase() + if (normalized === 'npm_config_registry' || normalized === 'sim_api_key') delete env[key] } return env } @@ -421,12 +440,13 @@ export async function announceUpdateIfAvailable(options: UpdateCheckOptions = {} const env = options.env ?? process.env const isTty = options.isTty ?? process.stderr.isTTY === true const modulePath = options.modulePath ?? fileURLToPath(import.meta.url) + const cwd = options.cwd ?? process.cwd() const now = options.now ?? new Date() if (isEnabled(env.SIM_NO_UPDATE_CHECK)) return if (!isTty) return if (CI_VARIABLES.some((variable) => isEnabled(env[variable]))) return - if (isUnadvisableInstall(modulePath)) return + if (isUnadvisableInstall(modulePath, env, cwd)) return const currentVersion = options.currentVersion ?? CLI_VERSION const current = parseStableVersion(currentVersion)