feat: wizard-owned OAuth session and project binding storage#516
Merged
Conversation
- Store OAuth session in ~/.amplitude/wizard/oauth-session.json (0o600) with dual-read/migrate from ~/.ampli.json and dual-write for transition - Canonical project binding: .amplitude/project-binding.json; merge with legacy ampli.json; writeAmpliConfig returns whether any sink persisted - Gitignore: drop blanket .amplitude/; ignore .amplitude/dashboard.json and legacy dotfiles; wizard block replace upgrades old .amplitude/ entries - reset: clear auth before rm .amplitude to avoid readAmpliConfig migration recreating the directory - Diagnostics, logout copy, oauth/store messaging, CI hint paths updated Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Contributor
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
Bugbot Autofix prepared fixes for both issues found in the latest run.
- ✅ Fixed: Credential clearing defeated by legacy re-migration on next read
- Changed readConfig to only migrate from legacy when the primary file doesn't exist on disk (fs.existsSync check), so writing {} to primary via clearStoredCredentials is respected and legacy data is never resurrected.
- ✅ Fixed: Non-ENOENT errors misreported as
not_foundtriggers false migration- Added a 'read_error' variant to AmpliConfigParseResult and made readAmpliConfigFile return it for non-ENOENT filesystem errors (e.g. EACCES), so the migration condition at line 271 no longer falsely triggers for unreadable files.
Or push these changes by commenting:
@cursor push 780ec25898
Preview (780ec25898)
diff --git a/src/lib/ampli-config.ts b/src/lib/ampli-config.ts
--- a/src/lib/ampli-config.ts
+++ b/src/lib/ampli-config.ts
@@ -90,7 +90,10 @@
export type AmpliConfigParseResult =
| { ok: true; config: AmpliConfig }
- | { ok: false; error: 'not_found' | 'invalid_json' | 'merge_conflicts' };
+ | {
+ ok: false;
+ error: 'not_found' | 'read_error' | 'invalid_json' | 'merge_conflicts';
+ };
// ── Pure logic (unit-testable) ────────────────────────────────────────────────
@@ -232,13 +235,14 @@
raw = fs.readFileSync(filePath, 'utf-8');
} catch (err) {
const code = (err as NodeJS.ErrnoException).code;
- if (code !== 'ENOENT') {
- log.debug('readAmpliConfigFile: read failed', {
- path: filePath,
- 'error message': err instanceof Error ? err.message : String(err),
- });
+ if (code === 'ENOENT') {
+ return { ok: false, error: 'not_found' };
}
- return { ok: false, error: 'not_found' };
+ log.debug('readAmpliConfigFile: read failed', {
+ path: filePath,
+ 'error message': err instanceof Error ? err.message : String(err),
+ });
+ return { ok: false, error: 'read_error' };
}
return parseAmpliConfig(raw);
}
diff --git a/src/utils/ampli-settings.ts b/src/utils/ampli-settings.ts
--- a/src/utils/ampli-settings.ts
+++ b/src/utils/ampli-settings.ts
@@ -79,7 +79,11 @@
if (oauthEntriesPresent(primary)) {
return primary;
}
- if (oauthEntriesPresent(legacy)) {
+ // Only migrate from legacy when the primary file doesn't exist on disk.
+ // If primary exists (even as `{}`), it was written intentionally (e.g. by
+ // clearStoredCredentials) and legacy data must not be resurrected.
+ const primaryFileExists = fs.existsSync(primaryPath);
+ if (!primaryFileExists && oauthEntriesPresent(legacy)) {
const merged: Record<string, unknown> = { ...legacy, ...primary };
try {
ensureDir(getCacheRoot());You can send follow-ups to the cloud agent here.
Reviewed by Cursor Bugbot for commit f46ca02. Configure here.
Member
Author
…inguish read errors from not-found
Bug 1: clearStoredCredentials writes {} to primary, but if legacy write fails,
readConfig would see no OAuth entries in primary, find them in legacy, and
re-migrate them back — effectively un-clearing credentials. Fix: only migrate
from legacy when the primary file doesn't exist on disk at all.
Bug 2: readAmpliConfigFile returned 'not_found' for all FS errors including
EACCES. This caused readAmpliConfig to treat an unreadable binding file as
absent and attempt migration with stale legacy data. Fix: return 'read_error'
for non-ENOENT failures so the migration condition doesn't trigger falsely.
Applied via @cursor push command
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.


Summary
Rebases and ships the storage consolidation work from
feat/storage-consolidation-resilienceonto currentmain.Not duplicative of recent main:
mainalready landed atomic JSON persistence and.amplitude/event/dashboard paths (#498, #503, #504). This branch adds wizard-owned OAuth (~/.amplitude/wizard/oauth-session.json,0o600) with migrate/dual-read from~/.ampli.json, canonical project binding at.amplitude/project-binding.jsonwith legacyampli.jsonmerge, narrowed gitignore (.amplitude/dashboard.jsoninstead of blanket.amplitude/), reset/diagnostics copy updates, and a small type-narrowing fix forAmpliConfigParseResult.Merge / rebase notes
origin/main(includes feat: AuthOnboardingPath, replace --signup with --auth-onboarding #514 auth onboarding, etc.).wizard-tools/wizard-tools.testconflicts by keeping narrowed-gitignore behavior and aligning comments withpersistEventPlan(canonical events only; legacy dotfiles still gitignored for migration reads).Testing
pnpm test(2786 tests) — green locally.Supersedes closed PR #512 (same branch, updated after rebase).
Made with Cursor
Note
Medium Risk
Touches credential persistence and project binding resolution (OAuth token storage, migration/merge logic, gitignore/reset behavior), which can affect login reuse and project scoping if the migration paths misbehave.
Overview
Moves OAuth token persistence from legacy
~/.ampli.jsonto a wizard-owned~/.amplitude/wizard/oauth-session.json(mode0o600), with read-time migration/dual-write to keep older workflows working.Introduces a canonical per-project binding file at
.amplitude/project-binding.jsonand updatesreadAmpliConfig/writeAmpliConfigto merge binding over legacyampli.json, forward-migrate on read, and return a boolean success signal for persistence failures.Narrows gitignore behavior to ignore only
.amplitude/dashboard.json(not the whole.amplitude/dir), updates/diagnostics,reset, logout messaging, and adds/adjusts tests around migration, merging precedence, and failure handling.Reviewed by Cursor Bugbot for commit 71a16d7. Bugbot is set up for automated code reviews on this repo. Configure here.