fix(cli): move install-state into the config dir so deleting it is a full reset - #2904
Conversation
jrusso1020
left a comment
There was a problem hiding this comment.
Moves install-state into the config dir, with a one-way migration. Correct, well-tested, and the tests are genuinely sensitive — I verified that independently rather than taking the fault-injection claim on trust. Approving.
The question this PR is really answering is "does it address the review feedback on #2854", so I went and read that feedback rather than working from the summary of it. Two things worth separating:
Addressed. The comment at #2854 packages/cli/src/telemetry/config.ts:18 — that deleting the hyperframes config should wipe all hyperframes state, rather than persisting state elsewhere to get around it — is fully addressed. STATE_FILE is now join(CONFIG_DIR, "install-state.json") (config.ts:47), and the migration deletes the pre-move copy (config.ts:102-108), so nothing is left outside CONFIG_DIR. The quote in the PR body is faithful to the original. Together with what already shipped in #2874, rm -rf ~/.hyperframes is a genuine full reset.
Not addressed, and worth saying out loud so it isn't assumed closed. A second comment in that same pass, at #2854 config.ts:43, asked:
should this be in install state? maybe the name is confusing but i feel like we would want their to be "install state" then per feature flag values separate from each other?
InstallState (config.ts:54-59) still carries markerAt (install lineage) and deParallelRouterTrialFired (a per-feature-flag value) in one file, which is the shape that comment questioned. That's outside this PR's stated scope and not a reason to hold it — flagging it only so "does this address your concern" reads as yes, the location concern, not yes, the whole pass. Two further comments on #2854 (canary.ts:10 on conflating flags with long-lived opt-in config, and canary-rollouts.mdx:10 on folding into the telemetry doc plus telemetry-opt-out implying canary-opt-out) are still open there too.
Strengths
readInstallState(config.ts:85-100) gets the ordering right in both directions: current location wins, so a stale legacy file can't resurrect a breaker the user cleared; and the write-then-remove sequence means a failed migration write keeps the legacy copy rather than dropping the state on the floor.- Both locations go through the same
parseInstallState(config.ts:62), so the legacy path can't accept a shape the current path would reject. Easy thing to get wrong by inlining a second, laxer parse. - The comment block at
config.ts:14-45records why the old design existed and why it was rejected. Rationale that explains a reversal is the thing that stops the next person re-litigating it.
Findings
important — sequencing, not a defect here: #2854 is still open and still declares the old location. At its current head, #2854 config.ts:36-37 sets STATE_DIR = join(homedir(), ".local", "state", "hyperframes"), adds bucketSeed to the state file, and asserts the key set ["bucketSeed", "deParallelRouterTrialFired", "markerAt"] at #2854 config.test.ts:187. 232 lines of config.ts differ between the two heads. If #2854 lands after this without adopting CONFIG_DIR, it silently reverts exactly the change being made here, and the tests that pin the new location go with it. Two asks: rebase #2854 onto this, and revisit its rationale — under this PR bucketSeed correctly dies with rm -rf ~/.hyperframes, so any "cohort lineage survives a wipe" framing there needs rewording rather than reinstating.
note — install_predecessor_found changes meaning at this version boundary, with no marker. Same property name, same events (client.ts:81), but pre-move it meant "a previous install existed on this machine, surviving directory deletion" and post-move it means "config.json was lost while install-state survived". A series spanning the boundary is one name with two definitions. It's recoverable at query time because cli_version rides on every event (client.ts:57) — worth stating that in the field's docstring so whoever reads the metric next doesn't have to rediscover why the rate stepped.
nit — a test fixture still encodes the pre-move path. packages/cli/src/commands/telemetry.test.ts:30 mocks STATE_PATH: "/test/.local/state/hyperframes/install-state.json". It passes (the module is mocked), but it now reads as documentation of a location that no longer exists.
nit — the comment describing the file's purpose is scoped to the old layout. packages/cli/src/commands/telemetry.ts:64-65 calls it state that "survives a config wipe". Still true of config.json loss, but it was written when the file lived outside the config dir, so its meaning quietly narrowed. One line, and you're already adjacent to it.
nit — the empty legacy directory outlives the file. removeLegacyStateFile removes the file, leaving ~/.local/state/hyperframes/ behind. config.ts:51's "no file is left behind outside CONFIG_DIR" is literally accurate; take it or leave it, since an rmdir here can race and buys little.
worth naming as a decision, not a surprise: during the mixed-version window, an older CLI still reads and writes the pre-move path, and the new CLI deletes that file whenever a current one exists (config.ts:88). So while both are in use — an older project-pinned version alongside a new global — the old CLI loses its tripped breaker each time the new one runs and can re-enrol into a router path that already failed on that machine. Bounded (an experimental path retried, not data loss) and an unavoidable consequence of "one-way migration, nothing outside CONFIG_DIR", so I'd ship it; just better as a stated trade-off than a discovered one.
Verification
- Full files read, not just the diff. Scope confirmed as exactly the 2 files against current
main— worth noting because a three-dot diff against a stale localmainmisattributes unrelated commits to the branch. - Baseline: 29 pass (21
config.test.ts, 8commands/telemetry.test.ts). - Independent fault injection. Replaced
readInstallState's body withreturn parseInstallState(STATE_FILE), dropping the migration: 2 failed / 27 passed, exactly "adopts a pre-move state file…" and "lets the current location win…". So the migration tests are load-bearing, not decoration, and the other 27 are correctly insensitive to it. Restored clean afterwards. hyperframes telemetry statusreally does print the path (commands/telemetry.ts:66), so the PR body's discoverability claim holds. No docs referenceinstall-state, so nothing there went stale.- CI is terminal-green at
dae1b63d4— every check either success or skipped by path filter, nothing failing or pending. TheBLOCKEDstate was the reviewer gate. - Not verified by me: the 154,964 / 154,964
install_predecessor_foundfigure. That's author-reported, and the PR body is appropriately careful that two days of data can't yet prove wipes are rare.
Verdict: APPROVE
Reasoning: Does what it claims, with tests I confirmed are sensitive to the behaviour they name; the location concern from #2854 is genuinely resolved. Nothing blocking — the one item needing action is on #2854, which must not reinstate the old path when it rebases.
— Rames Jusso
Adopts the #2904 pattern for bucketSeed. The seed rides in install-state.json, which now lives beside config.json in ~/.hyperframes rather than in ~/.local/state/hyperframes/. Review rejected persisting state outside the config dir to defeat a user's reset, and that objection is sharpest for the seed: it is the one field that would turn install-state into a persistent pseudonymous identifier surviving `rm -rf ~/.hyperframes`. The carryover still earns its place, just against the churn that actually happens. config.json is rewritten on every command and every render, and readConfig recovers from any parse/permission/IO failure by minting a fresh identity — so a re-mint would reshuffle cohorts mid-rollout. A no-schema file written once at mint is decoupled from that without leaving the directory. Config re-mint: cohorts hold. Directory deleted: cohorts go too, deliberately. A pre-move seed is adopted by the same migration, so installs already carrying one do not have a live cohort reshuffled under them. Tests: seed survives a re-mint, does NOT survive deleting the config dir, and migrates from the pre-move path. Prose in config.ts, canary.ts and canary-rollouts.mdx corrected — it still claimed cohorts survive a wipe. Docs gain a removal-path section. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
What
Moves the install-state file from
~/.local/state/hyperframes/install-state.jsoninto~/.hyperframes/install-state.json, and migrates any pre-move copy.This adopts the review feedback on #2854 for the code that already merged in #2874.
Why
#2874 put install-state in a second directory specifically so it would survive
rm -rf ~/.hyperframes. Review rejected that premise:That's right, and the measurement agrees: the churn this defends against is not users running
rm -rf.install_predecessor_foundisfalseon 154,964 / 154,964 events since the marker shipped (2026-07-28, v0.7.83–0.7.85). Not one carryover has fired. That number can't yet prove wipes are rare — the marker has only been live two days, and atrueneeds install → wipe → re-run inside that window — but it does mean the mechanism is carrying a contested design cost for no observed benefit.The threat that is real is
config.jsonitself. It's hot and wide — ~20 fields rewritten on every command and every render — andreadConfigrecovers from any parse/permission/IO failure by minting a fresh identity (anonymousId: parsed.anonymousId || randomUUID(), plus mint-on-catch). A separate file with no schema and a single write at first mint is decoupled from that churn without leaving the config dir.So the split is now along the axis that matters:
What changed
install-state.jsonnow lives inCONFIG_DIR.rm -rf ~/.hyperframesis a genuine full reset;hyperframes telemetry statusalready prints the path, so the removal path is discoverable.predecessorFoundnow measures the churn we actually care about — config lost, install-state survived (corruption/re-mint) — rather than directory deletion, which takes both.Behaviour otherwise unchanged: still no identity in the file, still just
markerAt+deParallelRouterTrialFired.Verification
fallow audit --base origin/mainall clean on the changed files.Follow-up (not in this PR)
The
|| randomUUID()inline mint and the mint-on-catch recovery churn identities today for every install, independent of canary. If stable cohorts are the goal, that's the higher-leverage fix and it needs no extra file at all.🤖 Generated with Claude Code