feat(cli): silent auto-update on next run#306
Conversation
- autoUpdate.test.ts unsets CI / HYPERFRAMES_NO_AUTO_INSTALL / HYPERFRAMES_NO_UPDATE_CHECK per-test so GitHub Actions' CI=true env doesn't short-circuit the scheduling policy under test. - installerDetection.test.ts coerces the optional argv[1] restore to '' to satisfy strict TS on CI (process.argv[1] is typed as string; reading + assigning back hits the 'string | undefined' mismatch under exactOptionalPropertyTypes).
TL;DRSolid PR. The "schedule + report on next run" flow is the right shape: parent never blocks, output is silent, banner appears once on N+1. PR description is unusually thorough — includes a live e2e smoke test transcript that exercises the real detached spawn + config writeback + fresh-process banner. Two things worth addressing before merge, plus several nits. Issues to address before merge1. Failed installs retry on every invocation, indefinitely (medium)
if (
config.completedUpdate &&
config.completedUpdate.version === latestVersion &&
config.completedUpdate.ok
) {
return false;
}The "already completed" short-circuit only fires when Two fixes, in order of preference:
The second is one line and almost free. 2. Concurrent config writes between detached child and parent (medium)The detached child does its own exec(CMD, { windowsHide: true, maxBuffer: 4 * 1024 * 1024 }, (err, _stdout, stderr) => {
let cfg = {};
try { cfg = JSON.parse(readFileSync(CFG, "utf-8")); } catch (e) {}
cfg.completedUpdate = { ... };
delete cfg.pendingUpdate;
try { writeFileSync(CFG, JSON.stringify(cfg, null, 2) + "\n", { mode: 0o600 }); } catch (e) {}
});The parent process is still alive when the child runs. The parent already writes to the same file from at least three other places: Smallest fix: write to Nits / smaller issuesShell injection via npm registry version field (low, defense-in-depth)
if (!/^\d+\.\d+\.\d+(?:[-+][0-9A-Za-z.-]+)?$/.test(latestVersion)) return false;Drop that at the top of Windows npm globals are misclassified as
|
jrusso1020
left a comment
There was a problem hiding this comment.
TL;DR
Clean design, excellent PR description, and the "spawn on N, surface on N+1" shape is right. Agreeing with @vanceingalls's two blockers — neither is addressed in the latest commit — and adding one new issue that's in the same "wrong installer → clobber the user's machine" risk class as the detector guardrails this PR is explicitly designed around.
Requesting changes because the new finding (#1 below) is a realistic way to silently install hyperframes globally on a user who never asked for a global install, which inverts the conservative-classifier promise in installerDetection.ts.
New: .pnpm substring match false-positives on project-local pnpm installs
installerDetection.ts:120-125:
if (
realEntry.includes(`${sep}pnpm${sep}global${sep}`) ||
realEntry.includes(`${sep}.pnpm${sep}`)
) {
return { kind: "pnpm", installCommand: (v) => `pnpm add -g hyperframes@${v}`, ... };
}The /.pnpm/ branch is too loose. Every pnpm-managed project has node_modules/.pnpm/ in its hoisting layout. A user who adds hyperframes as a local dep in their pnpm project and runs it via pnpm exec hyperframes or node_modules/.bin/hyperframes will realpathSync through the pnpm hardlink to:
/path/to/their-project/node_modules/.pnpm/hyperframes@0.4.3/node_modules/hyperframes/dist/cli.js
That path matches ${sep}.pnpm${sep} → classified as pnpm → silently runs pnpm add -g hyperframes@0.4.4 on the user's machine. They didn't want a global install; now they have one, competing with the local dep the next time pnpm hoists.
The workspace check above wouldn't save this case — the resolved entry is under node_modules/.pnpm/…/node_modules/hyperframes/…, no /packages/cli/ segment.
Fix: drop the .pnpm fallback and keep only the /pnpm/global/ prefix. The platform-specific globals you actually want to catch are:
- macOS:
~/Library/pnpm/global/5/node_modules/… - Linux:
~/.local/share/pnpm/global/5/node_modules/… - Windows:
%LOCALAPPDATA%\pnpm\global\5\node_modules\…
All three contain /pnpm/global/ as a full segment — no need for the .pnpm fallback. Local installs correctly fall through to the unknown-layout skip, which is exactly the conservative behavior the module docstring promises.
Worth a new test: node_modules/.pnpm/hyperframes@0.4.3/node_modules/hyperframes/dist/cli.js → skip — locks in the precedence and prevents a future "helpful" refactor from re-introducing the loose match.
Agreeing with Vance's blockers
Both are still present in the latest diff:
-
Failed-install retry loop (
autoUpdate.ts:163-170) — still only short-circuits whenok === true. Vance's one-line fix (short-circuit whenversionmatches regardless ofok) is the cheapest version; backoff-with-counter is the more robust one. -
Torn config.json writes from detached child + parent — the child's
writeFileSyncis still non-atomic. At minimum,writeFileSync(tmpPath, …); renameSync(tmpPath, CFG)to turn lost-update into last-writer-wins instead of half-written JSON. This is worth fixing now because the parent already races with itself across three other writers (incrementCommandCount,checkForUpdate, telemetry) — the child just adds a fourth.
One additional nit beyond Vance's list
process.execPath under bun is bun, not node. The detached child does spawn(process.execPath, ["-e", nodeScript]). If a user installed via bun add -g hyperframes, process.execPath will be the bun binary. The inline script uses require("node:child_process") and require("node:fs") which bun supports, but:
bun -esemantics for CJSrequireofnode:*schemes has changed across bun versions; 1.0.x had gaps.- The "ships nothing extra" property in the PR description subtly depends on the runtime that launched the CLI supporting this exact inline script shape.
Low-risk (bun ≥ 1.1 handles it), but worth a one-line doctor note: if process.execPath basename isn't node, log it so install failures have a footprint. Could also use which("node") and fall back to process.execPath.
What's still right about the PR
Everything Vance called out — detector-biased-toward-skip, major-version guard, detached + unref(), separate env knobs, per-user file modes, optional schema fields. The live smoke-test transcript in the description is the kind of verification I want on CLI-plumbing PRs where unit tests can only cover the policy gates.
Verdict
Request changes on #1 (pnpm local-install false-positive) + Vance's #1 (failed-install retry) + Vance's #2 (atomic config write). The rest (bun runtime note, injection defense-in-depth, Windows npm path, maxBuffer, banner UX, log rotation, detection-precedence tests) are all fine as follow-ups.
Happy to re-review once those three are in.
|
Addressed the three blocking review items in Changes:
Verification:
Note:
|
|
Addressed the failing Windows check in Root cause:
Change:
Verification:
|
Summary
Today users have to run
hyperframes upgrade(or the right install command for their package manager) to get a new release — we ship fixes but they don't reach the install until the user remembers. This PR borrows the Claude Code model: detect the update on run N, install it in a detached background child, surface one line ("hyperframes auto-updated to vX.Y.Z") on run N+1. The user's current command never blocks, never prompts, never sees an install stream.Flow across two runs
Installer detection
Walks
realpathSync(process.argv[1])against each package manager's well-known global prefix. Wrong guesses are biased towardskip— we'd rather miss an auto-update than clobber a Homebrew install with npm.…/Cellar/hyperframes/<v>/…brewbrew upgrade hyperframes…/.bun/…bunbun add -g hyperframes@<v>…/pnpm/global/…or…/.pnpm/…pnpmpnpm add -g hyperframes@<v>…/lib/node_modules/hyperframes/…npmnpm install -g hyperframes@<v>…/packages/cli/…(workspace link)skip…/_npx/…,…/bunx-…/…skipskipGuardrails
hyperframes upgradeexplicitly.HYPERFRAMES_NO_AUTO_INSTALL=1disables the install without silencing the notice banner.HYPERFRAMES_NO_UPDATE_CHECK=1silences both (existing knob).~/.hyperframes/auto-update.logfor postmortem — the terminal stays clean.hyperframes upgrademanually.What changed
packages/cli/src/utils/installerDetection.tspackages/cli/src/utils/autoUpdate.tsscheduleBackgroundInstall+reportCompletedUpdate. Spawns a detachednode -e "..."child that runs the install and writes the outcome back to the config, thenunref()s so the parent exits immediately.packages/cli/src/telemetry/config.tspendingUpdate+completedUpdatefields on the config schema.packages/cli/src/cli.tsreportCompletedUpdate()at startup andscheduleBackgroundInstall()aftercheckForUpdate()resolves.Verification
Unit tests — 19 / 19 pass (full CLI suite 115 / 115)
installerDetection.test.ts— 9 cases, one per layout (workspace, npx, bunx, brew, bun, pnpm, npm, unknown, unresolved).autoUpdate.test.ts— 10 scheduling-policy cases:CI=1→ skippedHYPERFRAMES_NO_AUTO_INSTALL=1→ skippedUnit tests mock
spawnand the installer — they verify the policy, not the real detached-child path.Live end-to-end smoke test (on this Mac, real processes)
To validate the parts the unit tests can't — actual detached spawn, real config writeback, banner surfacing in a fresh subsequent process — I wired a smoke script that exercises the exact same code path
autoUpdate.tsuses, but withecho …as the "install command" so nothing global gets touched.Steps exercised:
~/.hyperframes/config.json.pendingUpdatemarker for version0.4.99(likescheduleBackgroundInstalldoes).node -e "..."child the real scheduler produces, with the install command replaced byecho 'faux install for 0.4.99'.unref()d and continued; 800 ms later the parent re-readconfig.json.reportCompletedUpdate()in a fresh subprocess (viabunx tsx -e ...) to match the real "Run N+1" conditions, capturing its stderr.Observed output:
What this proves:
[spawn] pid=49469logged, parent continued immediatelyexec(CMD)completedUpdate = { version: "0.4.99", ok: true, finishedAt: … }pendingUpdate = (cleared)"hyperframes auto-updated to v0.4.99"autoUpdate.ts:reportCompletedUpdateexactlycompletedUpdateabsentBoth the original test-plan checkboxes (fresh install,
HYPERFRAMES_NO_AUTO_INSTALL=1,CI=1) are covered by either the unit-test suite or this smoke test — the scheduling-policy gates are unit-tested underCI=true, and the real detached-spawn path is smoke-tested above.What's still worth doing
npm i -g/brew/bun add -genvironment — the smoke test above replaces the install command withecho, so we've never actually seen npm/bun/brew run the real command. That's the one remaining unknown. Worth one manual run on the maintainer's machine before cutting v0.4.4.Test plan
bunx vitest runonpackages/cli— 115 / 115 pass (incl. 19 new)tsc --noEmitcleantsupbuild cleannpm i -g hyperframes@0.4.2install to confirm the realnpm install -g hyperframes@0.4.3command actually runs whenautoUpdate.tsdelegates to it (the smoke test stopped short of executingnpm)Notes
checkForUpdate+printUpdateNoticestill work unchanged; this PR adds a second stage that applies the update rather than just telling the user about it.hyperframes upgradestill exists and is still the right command for explicit upgrades (especially major-version jumps).