Merge pull request #2904 from heygen-com/07-30-fix_cli_move_install-state_into_the_config_dir_so_deleting_it_is_a_full_reset

fix(cli): move install-state into the config dir so deleting it is a full reset

Author: vanceingalls

packages/cli/src/telemetry/config.test.ts

@@ -1,4 +1,6 @@
 import { beforeEach, describe, expect, it, vi } from "vitest";
+import { homedir } from "node:os";
+import { join } from "node:path";
 
 // In-memory fake filesystem so these tests exercise the REAL config.ts
 // module (parsing, caching, readConfigFresh's cache-bypass) without ever
@@ -28,6 +30,10 @@ vi.mock("node:fs", () => ({
     fsState.files.set(to, content);
     fsState.files.delete(from);
   }),
+  // Legacy install-state cleanup after the migration to CONFIG_DIR.
+  rmSync: vi.fn((path: string) => {
+    fsState.files.delete(path);
+  }),
 }));
 
 describe("config.ts — readConfig / readConfigFresh / writeConfig (real module, faked fs)", () => {
@@ -130,13 +136,18 @@ describe("config.ts — readConfig / readConfigFresh / writeConfig (real module,
   });
 });
 
-describe("install-state rollover (breaker survives a config wipe)", () => {
+describe("install-state rollover (breaker survives a config re-mint)", () => {
   let readConfig: typeof import("./config.js").readConfig;
   let readConfigFresh: typeof import("./config.js").readConfigFresh;
   let writeConfig: typeof import("./config.js").writeConfig;
   let CONFIG_PATH: typeof import("./config.js").CONFIG_PATH;
   let STATE_PATH: typeof import("./config.js").STATE_PATH;
 
+  // Derived here rather than exported from config.ts: the pre-move path is
+  // frozen history, so pinning the literal is the point — an export would
+  // just let a rename pass silently, and it has no non-test consumer.
+  const LEGACY_STATE_PATH = join(homedir(), ".local", "state", "hyperframes", "install-state.json");
+
   beforeEach(async () => {
     fsState.files.clear();
     vi.resetModules();
@@ -245,4 +256,39 @@ describe("install-state rollover (breaker survives a config wipe)", () => {
     fsState.files.set(CONFIG_PATH, JSON.stringify(legacy));
     expect(readConfigFresh().predecessorFound).toBeUndefined();
   });
+
+  // The move: state used to live in ~/.local/state/hyperframes/ so it would
+  // survive `rm -rf ~/.hyperframes`. Review rejected persisting state outside
+  // the config dir to defeat the user's reset, so it now shares CONFIG_DIR.
+  it("keeps install-state inside the config dir, so deleting that dir is a full reset", () => {
+    readConfig();
+    expect(STATE_PATH.startsWith(CONFIG_PATH.replace(/config\.json$/, ""))).toBe(true);
+    expect(STATE_PATH).not.toContain(".local");
+  });
+
+  it("adopts a pre-move state file so an upgrading install keeps its tripped breaker", () => {
+    fsState.files.set(
+      LEGACY_STATE_PATH,
+      JSON.stringify({ markerAt: "2026-07-28T00:00:00.000Z", deParallelRouterTrialFired: true }),
+    );
+    // Config absent => mintConfig consults install state; without the
+    // migration this install silently re-enrols in the failed trial.
+    const config = readConfig();
+    expect(config.deParallelRouterTrialFired).toBe(true);
+    expect(config.predecessorFound).toBe(true);
+    expect(fsState.files.has(STATE_PATH), "state migrated into CONFIG_DIR").toBe(true);
+    expect(fsState.files.has(LEGACY_STATE_PATH), "legacy copy removed").toBe(false);
+  });
+
+  it("lets the current location win over a stale legacy file, and deletes the legacy copy", () => {
+    // A user who cleared the breaker must not have it resurrected by a
+    // leftover file from the old scheme.
+    fsState.files.set(
+      LEGACY_STATE_PATH,
+      JSON.stringify({ markerAt: "2026-07-28T00:00:00.000Z", deParallelRouterTrialFired: true }),
+    );
+    fsState.files.set(STATE_PATH, JSON.stringify({ markerAt: "2026-07-30T00:00:00.000Z" }));
+    expect(readConfig().deParallelRouterTrialFired).toBeUndefined();
+    expect(fsState.files.has(LEGACY_STATE_PATH)).toBe(false);
+  });
 });

packages/cli/src/telemetry/config.ts

@@ -1,4 +1,4 @@
-import { existsSync, mkdirSync, readFileSync, renameSync, writeFileSync } from "node:fs";
+import { existsSync, mkdirSync, readFileSync, renameSync, rmSync, writeFileSync } from "node:fs";
 import { join } from "node:path";
 import { homedir } from "node:os";
 import { randomUUID } from "node:crypto";
@@ -12,30 +12,44 @@ const CONFIG_DIR = join(homedir(), ".hyperframes");
 const CONFIG_FILE = join(CONFIG_DIR, "config.json");
 
 // ---------------------------------------------------------------------------
-// Install-state file: ~/.local/state/hyperframes/install-state.json
+// Install-state file: ~/.hyperframes/install-state.json
 //
-// A second, deliberately separate location from CONFIG_DIR, so it survives
-// the most common identity reset — deleting or reinstalling ~/.hyperframes.
-// It exists to carry exactly two facts across that reset, and nothing else:
+// A separate FILE, but deliberately the same DIRECTORY as config.json, so
+// `rm -rf ~/.hyperframes` really is a full reset. It previously lived in
+// ~/.local/state/hyperframes/ specifically to survive that delete; review
+// rejected that ("if someone is deleting their hyperframes config it should
+// wipe all hyperframes state — I'm not sure we should try and persist state
+// elsewhere to get around this"), and the measurement agreed: the churn this
+// actually defends against is not users running `rm -rf`.
 //
-//   1. `markerAt` — "a hyperframes install existed on this machine". Written
-//      unconditionally, so the fraction of fresh installs that find it is a
-//      direct measurement of recoverable id churn (config wiped, machine
-//      persisted) vs unrecoverable (fresh machine/container/new user).
+// The threat it does defend against is config.json itself. That file is hot
+// and wide — ~20 fields rewritten on every command and every render — and
+// readConfig recovers from ANY parse/permission/IO failure by minting a fresh
+// identity. Splitting these two facts into their own file decouples them from
+// that churn: no shared schema to migrate on upgrade, and one write at first
+// mint instead of one per command.
+//
+// It carries exactly two facts, and no identity — no anonymousId, no
+// counters, nothing linking the old install to the new one:
+//
+//   1. `markerAt` — "a hyperframes install existed on this machine". Now that
+//      it shares CONFIG_DIR, `predecessorFound` measures the churn we care
+//      about (config.json lost, install-state survived => corruption/re-mint)
+//      rather than deliberate directory deletion, which takes both.
 //   2. `deParallelRouterTrialFired` — the DE parallel-router circuit
-//      breaker's tripped state. Without this, a config wipe re-enrols the
-//      install into an experimental path that already FAILED on this exact
-//      machine; the breaker's whole point is that a real failure turns the
-//      trial off for good.
+//      breaker's tripped state, so a config re-mint does not re-enrol an
+//      install into an experimental path that already FAILED on this machine.
 //
-// It intentionally holds NO identity: no anonymousId, no counters, nothing
-// that could link the old install to the new one. A user who wipes their
-// config gets a fresh id unconditionally — this file only stops the wipe
-// from also discarding a safety fact about the machine.
+// Removal path: delete ~/.hyperframes (or just this file). `hyperframes
+// telemetry status` prints its exact location.
 // ---------------------------------------------------------------------------
 
-const STATE_DIR = join(homedir(), ".local", "state", "hyperframes");
-const STATE_FILE = join(STATE_DIR, "install-state.json");
+const STATE_FILE = join(CONFIG_DIR, "install-state.json");
+
+// Pre-move location. Read once, migrated, then deleted — an install that
+// wrote state under the old scheme keeps its tripped breaker instead of
+// silently re-enrolling, and no file is left behind outside CONFIG_DIR.
+const LEGACY_STATE_FILE = join(homedir(), ".local", "state", "hyperframes", "install-state.json");
 
 interface InstallState {
   /** ISO timestamp of when the marker was first written. */
@@ -44,11 +58,11 @@ interface InstallState {
   deParallelRouterTrialFired?: boolean;
 }
 
-/** Read the install-state file; any parse/shape failure reads as absent. */
-function readInstallState(): InstallState | null {
+/** Parse one state file; any parse/shape failure reads as absent. */
+function parseInstallState(file: string): InstallState | null {
   try {
-    if (!existsSync(STATE_FILE)) return null;
-    const parsed = JSON.parse(readFileSync(STATE_FILE, "utf-8")) as Partial<InstallState>;
+    if (!existsSync(file)) return null;
+    const parsed = JSON.parse(readFileSync(file, "utf-8")) as Partial<InstallState>;
     if (typeof parsed.markerAt !== "string") return null;
     return {
       markerAt: parsed.markerAt,
@@ -59,6 +73,40 @@ function readInstallState(): InstallState | null {
   }
 }
 
+/**
+ * Read the install-state file, adopting the pre-move copy if this machine
+ * still has one.
+ *
+ * Migration is one-way and best-effort: the current location always wins (a
+ * stale legacy file must never resurrect a breaker the user has since
+ * cleared), and failing to delete the legacy copy is not an error — it is
+ * re-read harmlessly next time.
+ */
+function readInstallState(): InstallState | null {
+  const current = parseInstallState(STATE_FILE);
+  if (current !== null) {
+    removeLegacyStateFile();
+    return current;
+  }
+  const legacy = parseInstallState(LEGACY_STATE_FILE);
+  if (legacy === null) return null;
+  try {
+    writeInstallState(legacy);
+    removeLegacyStateFile();
+  } catch {
+    // Keep the legacy copy; the value is still returned below either way.
+  }
+  return legacy;
+}
+
+function removeLegacyStateFile(): void {
+  try {
+    if (existsSync(LEGACY_STATE_FILE)) rmSync(LEGACY_STATE_FILE, { force: true });
+  } catch {
+    // Best-effort cleanup — never break the CLI over a leftover file.
+  }
+}
+
 // Sync bookkeeping, so the existsSync+read doesn't run on every writeConfig:
 // `stateMarkerSynced` = the marker is known present; `stateFiredSynced` = the
 // state file is known to already carry fired=true.
@@ -76,7 +124,7 @@ export function __resetInstallStateSyncForTests(): void {
  * since a corrupted state file silently reads as absent.
  */
 function writeInstallState(next: InstallState): void {
-  mkdirSync(STATE_DIR, { recursive: true, mode: 0o700 });
+  mkdirSync(CONFIG_DIR, { recursive: true, mode: 0o700 });
   const tmpFile = `${STATE_FILE}.${process.pid}.tmp`;
   writeFileSync(tmpFile, JSON.stringify(next, null, 2) + "\n", { mode: 0o600 });
   renameSync(tmpFile, STATE_FILE);