Skip to content

refactor(cli-core): remove configstore dependency, use sync config I/O#752

Open
rexxars wants to merge 7 commits intomainfrom
rexxars/explore-telemetry
Open

refactor(cli-core): remove configstore dependency, use sync config I/O#752
rexxars wants to merge 7 commits intomainfrom
rexxars/explore-telemetry

Conversation

@rexxars
Copy link
Member

@rexxars rexxars commented Mar 20, 2026

Description

Removes the configstore npm dependency from @sanity/cli-core and replaces it with a minimal sync implementation that reads/writes ~/.config/sanity/config.json directly.

The motivation: configstore uses async I/O internally, and the logout command ran two concurrent async read-modify-write cycles via Promise.all (clearing auth token + clearing telemetry consent). This created an intra-process race condition where one write could overwrite the other, leaving stale values in the config file - most notably, telemetry consent status ("denied") surviving a logout/re-login cycle.

Switching to synchronous I/O makes each read-modify-write atomic within a single event loop tick, eliminating this class of bug. Cross-process races (two CLI processes writing simultaneously) are still theoretically possible but unchanged from before and unlikely in practice since config writes are rare and user-initiated.

Also moves telemetry utilities (getCliTelemetry, readNDJSON) from src/util/ to src/telemetry/ where they belong.

What to review

  • packages/@sanity/cli-core/src/services/cliUserConfig.ts - core change. The new getUserConfig() returns a ConfigStore with sync get/set/delete methods. This is a subset of what configstore exposed (no .all, .path, .size), but those were unused.
  • packages/@sanity/cli/src/commands/logout.ts - the bug fix. clearConfig() now uses sequential sync calls instead of Promise.all with async configstore.
  • packages/@sanity/cli-core/src/util/readJsonFileSync.ts and writeJsonFileSync.ts - new sync file utilities.
  • Test files - verify the mocking approach is reasonable for the new sync API.

Testing

The cliUserConfig.test.ts suite was rewritten with 19 tests covering the sync API and ConfigStore interface. All existing tests for callers (logout, login, telemetry, debug, etc.) were updated to mock the new getUserConfig export path.

Run pnpm test from the repo root to verify.

@claude
Copy link

claude bot commented Mar 20, 2026

Claude finished @rexxars's task in 2m 31s —— View job


PR Review

  • Gather context
  • Read key changed files
  • Identify issues
  • Post review

1. setCliUserConfig('authToken', undefined) creates empty config file on logout

packages/@sanity/cli-core/src/services/cliUserConfig.ts:35-49

When logging out with no existing config, readConfig() returns {}, but setCliUserConfig('authToken', undefined) still calls mkdirSync + writeJsonFileSync, creating ~/.config/sanity/config.json containing {}. The getUserConfig().delete() path avoids this with the early-return on line 118, but setCliUserConfig has no equivalent guard. After logout, users without a prior config end up with a new empty config file.

Fix: add an early return when the key doesn't exist in the read config:

if (value === undefined) {
  if (!(prop in config)) return   // <-- add this
  const {[prop]: _, ...rest} = config
  writeJsonFileSync(configPath, rest, {pretty: true})
}

Fix this →


2. getUserConfig().set doesn't invalidate the token cache

packages/@sanity/cli-core/src/services/cliUserConfig.ts:109-113

If anyone calls getUserConfig().set('authToken', token), the module-level token cache in cliTokenCache.ts will not be invalidated, causing getCliToken() to return a stale value. Currently no code does this — all authToken writes go through setCliUserConfig. But the ConfigStore interface gives no indication this key is special. Worth adding a note to the getUserConfig JSDoc or the ConfigStore interface that authToken must be managed via setCliUserConfig to ensure cache invalidation.


3. writeJsonFileSync adds a trailing newline that the original writeJsonFile didn't

packages/@sanity/cli-core/src/util/writeJsonFileSync.ts:17

The original wrote stringified directly; the new version writes `${stringified}\n`. This is good practice, but any existing config.json files will gain a trailing newline on the next write, which could matter for checksum-based tools or audit logs. Not a bug, but worth noting as an intentional behavior change.

@rexxars rexxars force-pushed the rexxars/explore-telemetry branch from 7505e1b to 5f5c293 Compare March 20, 2026 18:33
@github-actions
Copy link

github-actions bot commented Mar 20, 2026

Coverage Delta

File Statements
packages/@sanity/cli-core/src/SanityCommand.ts 86.0% (±0%)
packages/@sanity/cli-core/src/services/cliTokenCache.ts 100.0% (new)
packages/@sanity/cli-core/src/services/cliUserConfig.ts 100.0% (±0%)
packages/@sanity/cli-core/src/services/getCliToken.ts 100.0% (±0%)
packages/@sanity/cli-core/src/telemetry/getCliTelemetry.ts 100.0% (new)
packages/@sanity/cli-core/src/telemetry/readNDJSON.ts 100.0% (new)
packages/@sanity/cli-core/src/util/readJsonFileSync.ts 100.0% (new)
packages/@sanity/cli-core/src/util/writeJsonFileSync.ts 100.0% (new)
packages/@sanity/cli/src/actions/auth/login/login.ts 100.0% (±0%)
packages/@sanity/cli/src/actions/debug/gatherDebugInfo.ts 96.5% (±0%)
packages/@sanity/cli/src/actions/telemetry/setConsent.ts 96.7% (±0%)
packages/@sanity/cli/src/commands/logout.ts 100.0% (±0%)
packages/@sanity/cli/src/hooks/prerun/setupTelemetry.ts 95.2% (±0%)
packages/@sanity/cli/src/services/telemetry.ts 70.6% (±0%)
packages/@sanity/cli/src/util/createExpiringConfig.ts 94.1% (±0%)

Comparing 15 changed files against main @ 7af617a3f19983756f93edca8f939e19bdbb4d2b

Overall Coverage

Metric Coverage
Statements 83.0% (+ 0.0%)
Branches 72.7% (+ 0.1%)
Functions 83.3% (+ 0.1%)
Lines 83.5% (+ 0.0%)

@rexxars rexxars force-pushed the rexxars/explore-telemetry branch 2 times, most recently from b00e512 to 2ba9bda Compare March 20, 2026 18:49
Replace the configstore-backed getUserConfig() with a dependency-free
implementation that reads/writes the CLI config JSON file directly
using sync I/O. This fixes a race condition in logout where concurrent
async read-modify-write cycles on the same config file could silently
lose data (e.g. telemetry consent surviving a logout).

The getUserConfig() public API keeps its name. The return type narrows
from configstore's ConfigStore class to a minimal ConfigStore interface
with get/set/delete. Moves telemetry utilities from src/util/ to
src/telemetry/ in cli-core. Removes 6 transitive packages.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@rexxars rexxars force-pushed the rexxars/explore-telemetry branch from 2ba9bda to bdac322 Compare March 20, 2026 18:51
rexxars and others added 4 commits March 20, 2026 12:02
…il tests

Make setCliUserConfig explicitly remove the key when called with
undefined instead of relying on JSON.stringify silently dropping it.
Add unit tests for readJsonFileSync and writeJsonFileSync error paths.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- ConfigStore.delete now skips the write when the key doesn't exist,
  avoiding creating an empty config file as a side effect.
- writeJsonFileSync appends a trailing newline for POSIX compatibility
  and to avoid spurious diffs with files written by configstore.
- Cleaned up dead code in readJsonFileSync tests and tightened
  writeJsonFileSync test assertions.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The flush worker stdio was gated on `debug.enabled` (sanity:cli
namespace), but the worker logs using `telemetryDebug`
(telemetry:sanity:cli namespace). Running with
DEBUG=telemetry:sanity:cli:* would not inherit stdio, so worker
output went nowhere.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The previous tests spied on getCliToken and re-implemented its logic
(incorrectly using async getCliUserConfig). Now uses vi.resetModules()
+ dynamic import to get a fresh module per test, exercises the real
sync code path, and uses vi.stubEnv for cleaner env management.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@rexxars rexxars marked this pull request as ready for review March 21, 2026 16:58
@rexxars rexxars requested a review from a team as a code owner March 21, 2026 16:58
@rexxars rexxars requested review from binoy14, gu-stav and mttdnt and removed request for a team March 21, 2026 16:58
rexxars and others added 2 commits March 21, 2026 10:01
…t mkdirSync

setCliUserConfig now calls clearCliTokenCache() so that subsequent
getCliToken() calls in the same process pick up the new value.

Also removes the redundant mkdirSync from ConfigStore.delete() - if
readConfig() succeeded, the directory already exists.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…tCliToken

Extract token cache into a separate cliTokenCache module that both
cliUserConfig.ts and getCliToken.ts can import without creating a
cycle. Fixes the import-x/no-cycle lint error.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@rexxars rexxars removed the request for review from gu-stav March 21, 2026 17:49
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.

1 participant