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
|
miguel-heygen
requested review from
vanceingalls
and removed request for
vanceingalls
jrusso1020
left a comment
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:
|
jrusso1020
left a comment
jrusso1020
left a comment
There was a problem hiding this comment.
Re-review
Three blockers from the previous round — status:
| Item | Status | Where |
|---|---|---|
pnpm .pnpm false-positive (my #1) |
✅ Fixed | installerDetection.ts:134-140 — only /pnpm/global/ now; node_modules/.pnpm/… paths fall through to skip, regression test at installerDetection.test.ts:75-80 |
| Atomic config writeback (Vance #2) | ✅ Fixed | autoUpdate.ts:94-107 — child writes to ${CFG}.tmp, then renameSync |
| Failed-install retry loop (Vance #1) | autoUpdate.ts:163-166 |
Bonus: Windows npm paths now handled via normalizePath (new test at installerDetection.test.ts:107-111). 👍
Remaining blocker: failed-install short-circuit is defeated by the call order in cli.ts
The new check in scheduleBackgroundInstall (autoUpdate.ts:163-166):
if (config.completedUpdate && config.completedUpdate.version === latestVersion) {
return false;
}…is correct in isolation, but reportCompletedUpdate clears the marker before scheduleBackgroundInstall gets a chance to read it. In cli.ts:85-94:
import("./utils/autoUpdate.js").then((mod) => mod.reportCompletedUpdate()).catch(() => {}); // ← fires first, clears marker
import("./utils/updateCheck.js").then(async (mod) => {
_printUpdateNotice = mod.printUpdateNotice;
const result = await mod.checkForUpdate().catch(() => null); // ← awaits network (hundreds of ms)
if (result?.updateAvailable) {
const auto = await import("./utils/autoUpdate.js").catch(() => null);
auto?.scheduleBackgroundInstall(result.latest, result.current); // ← completedUpdate is already gone
}
});reportCompletedUpdate (autoUpdate.ts:199-202) clears the marker unconditionally — failure or success — "so the flag doesn't spam the next interactive run."
Race on Run N+1:
reportCompletedUpdatereadscompletedUpdate{ok:false, version:X}→ clears it → prints failure banner.- ~few hundred ms later,
checkForUpdatereturns →scheduleBackgroundInstall(X, current)runs. - The short-circuit at line 163 sees
config.completedUpdate === undefined(just cleared) → falls through → spawns install again. - Detached child fails again → writes
completedUpdate{ok:false, version:X}→ next run repeats.
The "one failure means stop trying" guarantee never fires in practice — the marker is always cleared before the scheduler sees it. A user on a perpetually-broken install (system npm without sudo, offline mirror, EACCES, brew formula not published yet, etc.) will see the failure banner and re-spawn an install on every single hyperframes invocation. That's the exact infinite-retry loop the short-circuit was supposed to prevent.
Suggested fix
Cheapest: only clear completedUpdate when ok === true. On failure, leave the marker in place so the next run's scheduleBackgroundInstall sees it and bails. Adjust reportCompletedUpdate to suppress the banner after it's been surfaced once, by stamping reported: true:
// autoUpdate.ts: reportCompletedUpdate
const done = config.completedUpdate;
if (!done) return;
if (done.ok) {
delete config.completedUpdate; // success: clear, user will never re-see
writeConfig(config);
} else if (!done.reported) {
config.completedUpdate = { ...done, reported: true }; // failure: keep + mark surfaced
writeConfig(config);
} else {
return; // already surfaced once, stay silent
}
if (!process.stderr.isTTY) return;
if (done.ok) { /* success banner */ }
else if (!done.reported) { /* failure banner */ }With this, scheduleBackgroundInstall's existing version-match check at line 163 correctly keeps short-circuiting for as long as latestVersion on npm matches the failed marker. When npm advances to a new version, the version mismatch lets the next retry proceed naturally.
(Alternative shape if you want a counter instead: failedAttempts: number on completedUpdate; bail when >= 3. Strictly more forgiving but also more state. The reported flag is the minimum change.)
Unchanged nits (all fine as follow-ups)
- Shell injection defense-in-depth on
latestVersion(validate against/^\d+\.\d+\.\d+(?:[-+][0-9A-Za-z.-]+)?$/before interpolating intoexec). maxBuffer: 4 * 1024 * 1024can stillENOBUFSon verbose npm installs.auto-update.logis still unbounded (no rotation).- Two overlapping update banners on the transition run (notice on Run N + auto-updated banner on Run N+1).
- Detection-precedence test (a path matching both bun + npm signatures — locks in branch ordering against a future refactor).
Verdict
Request changes for the one blocker. Everything else in this round is solid — the pnpm tightening, atomic writeback, and Windows normalization are exactly right. Happy to re-review once the reportCompletedUpdate marker-clearing behavior is adjusted.
|
One item still looks unresolved: the failed auto-update retry loop. The guard added in
But in the actual startup flow it gets defeated by the call order:
So on a failed auto-update:
That means the "stop retrying a broken install for the same version" behavior still does not work end-to-end. I think this still needs a real fix before calling the retry-loop issue resolved. The smallest path seems to be preserving failure state long enough for the scheduler to see it, instead of clearing |
|
Addressed the remaining retry-loop blocker in Change:
Verification:
|
jrusso1020
left a comment
jrusso1020
left a comment
There was a problem hiding this comment.
LGTM. Retry-loop fix in e3368d9a is correct.
Verified end-to-end:
| Scenario | Before fix | After fix |
|---|---|---|
| Run N+1 after a failed install, same version | reportCompletedUpdate cleared marker → scheduleBackgroundInstall re-spawned every run |
reportCompletedUpdate stamps reported:true; marker persists; scheduler short-circuits on version match ✅ |
| Run N+2 after failure surfaced | banner re-printed + re-spawn | silent early-return, marker untouched ✅ |
npm publishes vX+1 after failed vX |
already broken (was stuck on same version) | marker version ≠ latest → falls through, schedules install for X+1 ✅ |
| Successful install on N+1 | marker cleared, banner printed once | same (unchanged path) ✅ |
The new regression test at autoUpdate.test.ts:264-304 hits the exact call ordering from cli.ts — reportCompletedUpdate() first, then scheduleBackgroundInstall(sameVersion) — and asserts both that the banner prints exactly once and that spawn is never re-called. Good coverage for the race I flagged.
Overall on this PR: three blockers across two reviewers all addressed (pnpm false-positive, atomic config writeback, failed-install retry loop), Windows npm paths fixed bonus, and the smoke test in the PR description is the right level of verification for CLI-plumbing work.
Nits still open as follow-ups (none blocking):
- Shell injection defense-in-depth on
latestVersion— cheap semver regex at the top ofscheduleBackgroundInstallor switchexec(string)→spawn(argv) maxBuffer: 4 * 1024 * 1024canENOBUFSon verbose npm installs — either bump to 16 MB or stream stdio directly to the logauto-update.logunbounded — easy rotation later- Two overlapping banners on the transition run (existing update notice on N + auto-updated line on N+1) — minor UX
- Detection-precedence test locking in bun-before-npm branch ordering
- Minor: on non-TTY + failed-install runs, the
reported:truestamp is still applied without the user ever seeing the banner, which consumes the notice for a subsequent interactive run. Probably fine — matches the pre-existing success-path philosophy documented in the old comment — but worth a mental note if anyone files a "never saw the update-failed message" bug.
Ship it whenever. 🚢
3 participants
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).