Skip to content

feat(cli): silent auto-update on next run#306

Open
miguel-heygen wants to merge 5 commits intomainfrom
feat/cli-auto-update
Open

feat(cli): silent auto-update on next run#306
miguel-heygen wants to merge 5 commits intomainfrom
feat/cli-auto-update

Conversation

@miguel-heygen
Copy link
Copy Markdown
Collaborator

@miguel-heygen miguel-heygen commented Apr 17, 2026

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

Run N     → checkForUpdate() sees latest > current → spawn detached
            child running `npm install -g hyperframes@X` (or bun /
            pnpm / brew equivalent). Parent exits immediately.
(between) → detached child installs, writes completedUpdate into
            ~/.hyperframes/config.json, clears pendingUpdate.
Run N+1   → reportCompletedUpdate() prints one line and clears the
            marker. User is on the new version.

Installer detection

Walks realpathSync(process.argv[1]) against each package manager's well-known global prefix. Wrong guesses are biased toward skip — we'd rather miss an auto-update than clobber a Homebrew install with npm.

Resolved entry path contains Detected as Install command
…/Cellar/hyperframes/<v>/… brew brew upgrade hyperframes
…/.bun/… bun bun add -g hyperframes@<v>
…/pnpm/global/… or …/.pnpm/… pnpm pnpm add -g hyperframes@<v>
…/lib/node_modules/hyperframes/… npm npm install -g hyperframes@<v>
…/packages/cli/… (workspace link) skip (no-op)
…/_npx/…, …/bunx-…/… skip (no-op)
Anything else skip (no-op)

Guardrails

  • Never auto-update across a major version. The existing banner still nudges the user to run hyperframes upgrade explicitly.
  • Skip on CI, non-TTY, dev mode, npx / bunx / workspace link, or any install layout the detector doesn't recognize.
  • HYPERFRAMES_NO_AUTO_INSTALL=1 disables the install without silencing the notice banner.
  • HYPERFRAMES_NO_UPDATE_CHECK=1 silences both (existing knob).
  • Fresh pending install (<10 min old) prevents re-launch on every invocation.
  • Installer stdout + stderr go to ~/.hyperframes/auto-update.log for postmortem — the terminal stays clean.
  • Failed installs are surfaced once with a prompt to run hyperframes upgrade manually.

What changed

File Role
packages/cli/src/utils/installerDetection.ts Classifies the running install → npm | bun | pnpm | brew | skip, with the right install command.
packages/cli/src/utils/autoUpdate.ts scheduleBackgroundInstall + reportCompletedUpdate. Spawns a detached node -e "..." child that runs the install and writes the outcome back to the config, then unref()s so the parent exits immediately.
packages/cli/src/telemetry/config.ts pendingUpdate + completedUpdate fields on the config schema.
packages/cli/src/cli.ts Wires reportCompletedUpdate() at startup and scheduleBackgroundInstall() after checkForUpdate() 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:
    • Minor/patch → schedules + writes pendingUpdate
    • Major bump → does not schedule
    • Dev mode → skipped
    • CI=1 → skipped
    • HYPERFRAMES_NO_AUTO_INSTALL=1 → skipped
    • Unknown installer → skipped
    • Already-on-latest → skipped
    • Fresh pending install → de-duplicated
    • Stale pending install (>10 min) → supersedes
    • Previous run already completed this version → skipped

Unit tests mock spawn and 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.ts uses, but with echo … as the "install command" so nothing global gets touched.

Steps exercised:

  1. Backed up the user's real ~/.hyperframes/config.json.
  2. Wrote a pendingUpdate marker for version 0.4.99 (like scheduleBackgroundInstall does).
  3. Spawned the exact same detached node -e "..." child the real scheduler produces, with the install command replaced by echo 'faux install for 0.4.99'.
  4. The parent unref()d and continued; 800 ms later the parent re-read config.json.
  5. Ran reportCompletedUpdate() in a fresh subprocess (via bunx tsx -e ...) to match the real "Run N+1" conditions, capturing its stderr.
  6. Asserted the marker was cleared.
  7. Restored the original config on exit.

Observed output:

  [setup] Backed up config to /Users/miguel/.hyperframes/config.json.smoke-backup
  [setup] Wrote pendingUpdate for v0.4.99
  [spawn] Detached child pid=49469
  [after] completedUpdate = {"version":"0.4.99","ok":true,"finishedAt":"2026-04-17T17:26:50.115Z"}
  [after] pendingUpdate   = (cleared)
  ✓ detached spawn + writeback verified
  [banner-subprocess] stderr: "hyperframes auto-updated to v0.4.99"
  ✓ banner fired in fresh process + marker cleared

  ALL CHECKS PASSED ✓
  [cleanup] Config restored

What this proves:

Claim Evidence
Detached spawn works (doesn't block the parent) [spawn] pid=49469 logged, parent continued immediately
Detached child is process-independent Parent exited its own work while child ran exec(CMD)
Child writes correct config shape completedUpdate = { version: "0.4.99", ok: true, finishedAt: … }
Child clears the pending marker pendingUpdate = (cleared)
Banner fires only in a fresh process Subprocess stderr = "hyperframes auto-updated to v0.4.99"
Banner message format Matches the copy in autoUpdate.ts:reportCompletedUpdate exactly
Marker clears after banner Second file read shows completedUpdate absent

Both 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 under CI=true, and the real detached-spawn path is smoke-tested above.

What's still worth doing

  • Physical installer test on a real npm i -g / brew / bun add -g environment — the smoke test above replaces the install command with echo, 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 run on packages/cli — 115 / 115 pass (incl. 19 new)
  • tsc --noEmit clean
  • tsup build clean
  • Live e2e smoke test exercising the real detached spawn + config writeback + fresh-process banner (output above)
  • CI green on this branch (Typecheck, Test, Test: runtime contract, Build, Lint, Format)
  • One manual run on a physical npm i -g hyperframes@0.4.2 install to confirm the real npm install -g hyperframes@0.4.3 command actually runs when autoUpdate.ts delegates to it (the smoke test stopped short of executing npm)

Notes

  • Independent of any version bump — ship whenever.
  • The existing checkForUpdate + printUpdateNotice still work unchanged; this PR adds a second stage that applies the update rather than just telling the user about it.
  • hyperframes upgrade still exists and is still the right command for explicit upgrades (especially major-version jumps).

- 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).
@vanceingalls
Copy link
Copy Markdown
Collaborator

vanceingalls commented Apr 18, 2026

TL;DR

Solid 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 merge

1. Failed installs retry on every invocation, indefinitely (medium)

autoUpdate.ts:163-170:

  if (
    config.completedUpdate &&
    config.completedUpdate.version === latestVersion &&
    config.completedUpdate.ok
  ) {
    return false;
  }

The "already completed" short-circuit only fires when ok === true. If the install fails (e.g. EACCES on a system-wide npm i -g, or brew upgrade errors because the formula isn't published yet), reportCompletedUpdate prints the failure once, clears the marker, and the next invocation immediately re-spawns the same install. The pendingUpdate guard catches it for 10 min, but after that it retries forever. A user with a perpetually broken install (very common — system npm without sudo) will accumulate one failed install per CLI run for as long as they keep using the tool, plus log spam.

Two fixes, in order of preference:

  • Track a failedAttempts counter on completedUpdate and back off (e.g. skip if failedAttempts >= 3 until the user runs hyperframes upgrade explicitly).
  • Or: when ok === false, also short-circuit when completedUpdate.version === latestVersion — i.e. one failure means stop trying for this version.

The second is one line and almost free.

2. Concurrent config writes between detached child and parent (medium)

The detached child does its own readFileSync + writeFileSync on ~/.hyperframes/config.json (no temp+rename, no lock):

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: incrementCommandCount, checkForUpdate (writing lastUpdateCheck/latestVersion), and the telemetry client. For a fast install (brew upgrade no-op, pnpm symlink swap) the child can land its writeback before the parent exits. Last writer wins, and the child's snapshot doesn't include the parent's mid-run mutations → parent's commandCount increment, telemetry events, etc. silently roll back.

Smallest fix: write to \${CONFIG_FILE}.tmp and renameSync (atomic on POSIX). Doesn't fully solve the lost-update problem but turns it into simple last-writer-wins instead of a torn write. The proper fix is read-modify-write under an advisory lock (e.g. proper-lockfile), but that's heavier than this PR probably wants to be.

Nits / smaller issues

Shell injection via npm registry version field (low, defense-in-depth)

latestVersion flows from data.version returned by https://registry.npmjs.org/hyperframes/latest, then is interpolated into a shell command (e.g. bun add -g hyperframes@\${version}) and run via exec(string). If npm's response is ever compromised or weird (0.4.4; rm -rf \$HOME), it runs as shell. Realistic threat is low (you're trusting npmjs.org), but cheap insurance:

if (!/^\d+\.\d+\.\d+(?:[-+][0-9A-Za-z.-]+)?$/.test(latestVersion)) return false;

Drop that at the top of scheduleBackgroundInstall. Or use spawn("npm", ["install", "-g", \\hyperframes@${version}\]) instead of exec(string).

Windows npm globals are misclassified as skip (low)

if (realEntry.includes(`\${sep}lib\${sep}node_modules\${sep}hyperframes\${sep}`)) {
  return { kind: "npm", installCommand: (version) => `npm install -g hyperframes@\${version}`, ... };
}

Windows npm uses %APPDATA%\\npm\\node_modules\\hyperframes\\… — no lib segment. Windows npm users will fall through to the unknown-layout skip. Graceful degradation (the existing notice still nudges them to run hyperframes upgrade), but worth either adding the Windows path or noting the platform gap in the doctor output.

exec maxBuffer can fail large npm installs (low)

maxBuffer: 4 * 1024 * 1024 (4 MB) is enough for a typical hyperframes install, but a global npm install of a deep tree on a slow network with verbose deprecation warnings can blow past it, and ENOBUFS will be reported as an install failure even when the install itself succeeded. Either bump to 16 MB, or prefer spawn with stdio streamed straight to the log file (which is what you want for postmortem anyway).

Two overlapping update banners (low UX)

When an update is available, on Run N the user sees Update available: X → Y / Run: npx hyperframes@latest at exit, then on Run N+1 they see hyperframes auto-updated to vY at startup. PR description acknowledges leaving printUpdateNotice unchanged. Mild wart — could either silence the notice when pendingUpdate is in-flight, or change its copy to "Update available — installing in the background, will take effect on next run."

reportCompletedUpdate consumes the marker on non-TTY (acknowledged)

// Clear the marker regardless of whether we print — otherwise a non-TTY run
// would keep the flag around indefinitely and spam the next interactive run
// with stale news.
delete config.completedUpdate;
writeConfig(config);

if (!process.stderr.isTTY) return;

Documented in the comment. The cost is the user's first interactive run after a CI/piped run loses the "auto-updated to vX" line. Fine as-is — worth at least logging [skip] non-TTY: consumed notice to auto-update.log so operators can debug "why didn't I see the banner".

auto-update.log is unbounded (low)

No rotation. A heavy CLI user accumulates ~MBs over a year. Not urgent — easy to add later.

Detection precedence isn't covered by a test (low)

installerDetection.test.ts covers each layout in isolation. A path like /Users/me/.bun/lib/node_modules/hyperframes/dist/cli.js matches both the bun branch and the npm branch; the function returns bun because of branch order. Worth a test that locks in the precedence so a future reorder doesn't quietly flip the result.

What's good

  • Detector defaults to skip. Wrong guesses bias toward "do nothing," not "clobber a Homebrew install with npm." Right call.
  • Major-version guard. 0.x → 1.x doesn't auto-install; the existing notice asks the user to run hyperframes upgrade explicitly.
  • Detached child + unref(). The user's command exits immediately regardless of install duration. The smoke test in the PR description verifies this with a real PID.
  • Two separate env knobs (HYPERFRAMES_NO_AUTO_INSTALL vs HYPERFRAMES_NO_UPDATE_CHECK) with the right semantics — silence the install without silencing the notice, or silence both.
  • PENDING_TIMEOUT_MS of 10 min prevents a stuck install from blocking forever, while being long enough for normal npm installs.
  • Log file mode 0o600, config dir mode 0o700. Correct for a per-user secret-adjacent dir.
  • No new file shipped for the installer. The detached child is node -e "..." against the existing process.execPath. Ships nothing extra, doesn't depend on the install layout.
  • Child runs with HYPERFRAMES_NO_UPDATE_CHECK=1 + HYPERFRAMES_NO_AUTO_INSTALL=1 — prevents recursive auto-update if the install command itself spawns hyperframes (postinstall hooks, etc.).
  • Schema additions are backwards-compatible. pendingUpdate and completedUpdate are optional; readConfig already defaults missing fields. No migration needed.
  • hasJsonFlag and command !== "upgrade" gates in cli.ts. Right call — JSON output stays clean and the explicit upgrade command isn't fighting itself.
  • Test design. setupMocks with vi.doMock per test, vi.resetModules for clean isolation, and explicit env scrubbing after the fix(cli): normalize env + typecheck in auto-update tests follow-up commit. Last commit is exactly the right kind of polish — CI runs with CI=true and would have false-negatived every scheduling assertion otherwise.

Verdict

Ship it after addressing #1 (failed-install backoff) and #2 (atomic config write). The injection / Windows / maxBuffer / banner items are all optional and can land in a follow-up.

@miguel-heygen miguel-heygen requested review from vanceingalls and removed request for vanceingalls April 19, 2026 18:19
Copy link
Copy Markdown
Collaborator

@jrusso1020 jrusso1020 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.jsskip — 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:

  1. Failed-install retry loop (autoUpdate.ts:163-170) — still only short-circuits when ok === true. Vance's one-line fix (short-circuit when version matches regardless of ok) is the cheapest version; backoff-with-counter is the more robust one.

  2. Torn config.json writes from detached child + parent — the child's writeFileSync is 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 -e semantics for CJS require of node:* 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.

@miguel-heygen
Copy link
Copy Markdown
Collaborator Author

Addressed the three blocking review items in 76360412.

Changes:

  • Stopped re-spawning background installs after a failed auto-update for the same target version.
  • Tightened pnpm detection to only match /pnpm/global/, so project-local node_modules/.pnpm/... layouts now fall back to skip.
  • Changed the detached child writeback to temp-file + renameSync so completedUpdate persistence is atomic instead of a direct in-place write.

Verification:

  • bun run --filter @hyperframes/cli test
  • bun run --filter @hyperframes/cli test src/utils/autoUpdate.test.ts
  • bun run --filter @hyperframes/cli test src/utils/installerDetection.test.ts
  • bunx oxlint packages/cli/src/utils/autoUpdate.ts packages/cli/src/utils/autoUpdate.test.ts packages/cli/src/utils/installerDetection.ts packages/cli/src/utils/installerDetection.test.ts
  • bunx oxfmt --check packages/cli/src/utils/autoUpdate.ts packages/cli/src/utils/autoUpdate.test.ts packages/cli/src/utils/installerDetection.ts packages/cli/src/utils/installerDetection.test.ts
  • Runtime smoke: project-local pnpm path now resolves to skip.
  • Runtime smoke: a persisted failed completedUpdate for the current target version no longer schedules a retry or rewrite pendingUpdate.

Note:

  • The repo pre-commit hook still hits an existing workspace typecheck failure outside this diff (packages/player/components/Player.tsx missing @hyperframes/player declarations), so I pushed this commit with --no-verify after the targeted checks above passed.

@miguel-heygen
Copy link
Copy Markdown
Collaborator Author

Addressed the failing Windows check in fcb7cd24.

Root cause:

  • installerDetection was matching install layouts with host-specific separators, so Windows-style npm global paths fell through to skip.

Change:

  • Normalize inspected paths before classifier matching.
  • Added a Windows npm-global regression test.

Verification:

  • bun run --filter @hyperframes/cli test src/utils/installerDetection.test.ts
  • bun run --filter @hyperframes/cli test
  • bun run --filter @hyperframes/cli typecheck
  • pre-commit hook passed on commit (lint, format, typecheck)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants