From e2676ab6e5bf2b86099fafb9d10ef2a609d9b15c Mon Sep 17 00:00:00 2001 From: vreshch Date: Sun, 21 Jun 2026 12:31:32 +0200 Subject: [PATCH] fix(robustness): clear dead session on 4xx refresh; chooser shows load errors; surface unmergeable/push-retry conflicts; disconnect clears memory --- src/auth/auth-flow.test.ts | 22 +++++++++++++++ src/auth/auth-flow.ts | 11 +++++++- src/auth/oauth.ts | 15 +++++++++- src/git/git-client.ts | 23 +++++++++++++--- src/main.ts | 32 ++++++++++++++++------ src/memory-chooser.ts | 56 +++++++++++++++++++++++++++----------- src/settings.ts | 4 +++ src/sync-controller.ts | 39 ++++++++++++++++++++++---- 8 files changed, 165 insertions(+), 37 deletions(-) diff --git a/src/auth/auth-flow.test.ts b/src/auth/auth-flow.test.ts index 400d1ff..f43dff9 100644 --- a/src/auth/auth-flow.test.ts +++ b/src/auth/auth-flow.test.ts @@ -102,4 +102,26 @@ describe('auth-flow', () => { expect(post).not.toHaveBeenCalled(); expect(await flow(fakeStore(), post).f.getValidToken()).toBeNull(); }); + + it('getValidToken clears the dead session (and notifies onChange) on a 4xx refresh', async () => { + const store = fakeStore({ accessToken: 'AT0', refreshToken: 'RT0', expiresAt: NOW - 1 }, 'cid'); + const post = vi.fn(async () => ({ status: 400, json: { error: 'invalid_grant' } })); + const onChange = vi.fn(); + const notify = vi.fn(); + const tok = await flow(store, post, { onChange, notify }).f.getValidToken(); + expect(tok).toBeNull(); + expect(store.load()).toBeNull(); // session cleared → UI flips to signed-out + expect(onChange).toHaveBeenCalled(); + expect(notify).toHaveBeenCalledWith(expect.stringContaining('session expired')); + }); + + it('getValidToken keeps tokens on a transient 5xx refresh (retryable, not a sign-out)', async () => { + const store = fakeStore({ accessToken: 'AT0', refreshToken: 'RT0', expiresAt: NOW - 1 }, 'cid'); + const post = vi.fn(async () => ({ status: 503, json: null })); + const onChange = vi.fn(); + const tok = await flow(store, post, { onChange }).f.getValidToken(); + expect(tok).toBeNull(); + expect(store.load()).not.toBeNull(); // tokens preserved + expect(onChange).not.toHaveBeenCalled(); + }); }); diff --git a/src/auth/auth-flow.ts b/src/auth/auth-flow.ts index 4d21783..b65fef3 100644 --- a/src/auth/auth-flow.ts +++ b/src/auth/auth-flow.ts @@ -11,6 +11,7 @@ import { import { exchangeAuthCode, isTokenExpired, + OAuthHttpError, refreshTokens, registerClient, revokeToken, @@ -143,7 +144,15 @@ export function createAuthFlow(deps: AuthFlowDeps): AuthFlow { ); await deps.store.save(refreshed); return refreshed.accessToken; - } catch { + } catch (e) { + // A rejected refresh (4xx: invalid/expired/revoked refresh token) means the session is + // dead — clear it so the UI flips to signed-out instead of a green dot that fails every + // sync. Keep tokens on transient/server errors (5xx/network) so a blip isn't a sign-out. + if (e instanceof OAuthHttpError && e.status >= 400 && e.status < 500) { + await deps.store.clear(); + deps.onChange?.(); + deps.notify('Your Agentage session expired — sign in again.'); + } return null; } }; diff --git a/src/auth/oauth.ts b/src/auth/oauth.ts index b1d5a84..13e4997 100644 --- a/src/auth/oauth.ts +++ b/src/auth/oauth.ts @@ -19,6 +19,18 @@ export interface HttpResponse { status: number; json: unknown; } + +// Carries the HTTP status so callers can tell a rejected grant (4xx — clear the session) +// from a transient/server error (5xx/network — keep tokens, retry later). +export class OAuthHttpError extends Error { + constructor( + readonly status: number, + message: string + ) { + super(message); + this.name = 'OAuthHttpError'; + } +} export type HttpPost = ( url: string, init: { headers: Record; body: string } @@ -70,7 +82,8 @@ async function postToken( const res = await post(tokenEndpoint, { headers: FORM, body: form(body) }); if (res.status < 200 || res.status >= 300) { const d = res.json as { error_description?: string; error?: string } | null; - throw new Error( + throw new OAuthHttpError( + res.status, `Token request failed: ${d?.error_description ?? d?.error ?? `HTTP ${res.status}`}` ); } diff --git a/src/git/git-client.ts b/src/git/git-client.ts index 8400499..17183dd 100644 --- a/src/git/git-client.ts +++ b/src/git/git-client.ts @@ -25,6 +25,9 @@ export interface RepoCtx { export interface PullResult { conflicted: string[]; + // True when the histories can't be 3-way merged automatically (criss-cross / multiple merge + // bases — MergeNotSupportedError). No filepaths; the caller surfaces an actionable note. + unmergeable?: boolean; } const AUTHOR = { name: 'Agentage Memory', email: 'memory@agentage.io' }; @@ -103,23 +106,35 @@ export function createGitClient({ fs, http }: GitClientDeps, mergeDriver: MergeD // (abortOnConflict:false) — do NOT checkout, that would discard them. return { conflicted: e.data.filepaths }; } - throw e; // MergeNotSupportedError (criss-cross) handled by the caller (backup + LWW) + if (e instanceof Errors.MergeNotSupportedError) { + // Criss-cross / multiple merge bases: iso-git can't auto-merge and the worktree is + // left intact. Signal the caller to surface a clear "resolve manually" note rather + // than bubbling a raw error. (No auto LWW fallback — it would risk silent data loss.) + return { conflicted: [], unmergeable: true }; + } + throw e; } }, // No force. A non-ff push throws PushRejectedError → re-pull + re-push (still no force). - async push(c: RepoCtx): Promise { + // Returns the conflict outcome: an empty result = pushed; a conflicted/unmergeable result + // means the re-pull hit a conflict and we did NOT push — the caller surfaces it. + async push(c: RepoCtx): Promise { const ref = c.ref ?? 'main'; const doPush = () => wrapFS(git.push({ ...base(c), ...auth(c), http, url: c.url, ref })); try { const r = await doPush(); if (!r.ok) throw new Error('push not ok'); + return { conflicted: [] }; } catch (e) { if (e instanceof Errors.PushRejectedError) { - await client.pull(c); + // Someone pushed between our pull and push. Re-pull (merge); if THAT conflicts, + // propagate it instead of blindly re-pushing into a "not ok after rebase" error. + const pulled = await client.pull(c); + if (pulled.conflicted.length > 0 || pulled.unmergeable) return pulled; const r2 = await doPush(); if (!r2.ok) throw new Error('push not ok after rebase'); - return; + return { conflicted: [] }; } throw e; } diff --git a/src/main.ts b/src/main.ts index 3b8497d..ef7a4f5 100644 --- a/src/main.ts +++ b/src/main.ts @@ -3,6 +3,7 @@ import type { FsClient, MergeDriverCallback } from 'isomorphic-git'; import { type AgentageMemorySettings, type VaultInfo, + type VaultListResult, DEFAULT_SETTINGS, normalizeVaultName, } from './settings'; @@ -355,10 +356,11 @@ export default class AgentageMemoryPlugin extends Plugin implements SettingsHost } // --- memory selection (SettingsHost) --- - /** Existing server memories + their info (files/folders/updated) via GET /vaults. */ - async listVaults(): Promise { + /** Existing server memories + their info (files/folders/updated) via GET /vaults. Returns a + * result so the chooser can show a real error (with Retry) vs an empty account. */ + async listVaults(): Promise { const token = await this.auth.getValidToken(); - if (!token) return []; + if (!token) return { ok: false, error: 'Sign in to Agentage first.' }; try { const r = await requestUrl({ url: `${SYNC_ORIGIN}/vaults`, @@ -366,13 +368,17 @@ export default class AgentageMemoryPlugin extends Plugin implements SettingsHost headers: { Authorization: `Bearer ${token}` }, throw: false, }); - if (r.status < 200 || r.status >= 300) return []; + if (r.status < 200 || r.status >= 300) { + const body = safeJson(r.text) as { error?: string } | null; + return { ok: false, error: body?.error ?? `Couldn't load memories (HTTP ${r.status})` }; + } const raw = (safeJson(r.text) as { vaults?: unknown })?.vaults; - return Array.isArray(raw) + const vaults = Array.isArray(raw) ? raw.map(parseVaultInfo).filter((v): v is VaultInfo => v !== null) : []; - } catch { - return []; + return { ok: true, vaults }; + } catch (e) { + return { ok: false, error: (e as Error).message }; } } @@ -441,8 +447,14 @@ export default class AgentageMemoryPlugin extends Plugin implements SettingsHost isSignedIn(): boolean { return this.auth.isSignedIn(); } - disconnect(): Promise { - return this.auth.disconnect(); + async disconnect(): Promise { + await this.auth.disconnect(); + // Drop the selected memory so the next session starts in the amber "Choose Memory" state + // and re-validates the pick — never resurface a stale (possibly deleted) memory as active. + this.settings.vault = ''; + this.lastVault = undefined; + await this.saveSettings(); + this.refreshStatus(); } // Git fs runs over Obsidian's vault adapter on BOTH desktop and mobile (the proven @@ -503,6 +515,8 @@ export default class AgentageMemoryPlugin extends Plugin implements SettingsHost ok: false, message: `Conflicts in ${r.conflicted.length} file(s) — see "Agentage Sync Conflicts".`, }; + // Unmergeable history (criss-cross): not pushed, no per-file markers — surface the reason. + if (!r.pushed && r.message) return { ok: false, message: r.message }; const bits = [r.action, r.committed ? 'committed' : '', r.pushed ? 'pushed' : ''].filter( Boolean ); diff --git a/src/memory-chooser.ts b/src/memory-chooser.ts index d166ec7..2228e7d 100644 --- a/src/memory-chooser.ts +++ b/src/memory-chooser.ts @@ -1,9 +1,9 @@ import { type App, Modal, Notice, Setting } from 'obsidian'; -import type { VaultInfo } from './settings'; +import type { VaultInfo, VaultListResult } from './settings'; // What the chooser needs from the plugin. Kept narrow so it doesn't depend on main. export interface MemoryChooserHost { - listVaults(): Promise; + listVaults(): Promise; createVault(name: string): Promise<{ ok: boolean; vault?: string; error?: string }>; defaultVaultName(): string; selectVault(name: string): Promise; @@ -78,29 +78,34 @@ class MemoryModal extends Modal { } private async fillList(): Promise { - const [vaults, cur] = [await this.host.listVaults(), this.host.currentVault()]; + const [res, cur] = [await this.host.listVaults(), this.host.currentVault()]; this.listEl.empty(); - if (!vaults.length) { + if (!res.ok) { + this.listEl.createEl('p', { + text: `Couldn't load your memories: ${res.error}`, + cls: 'ams-hint', + }); + new Setting(this.listEl).addButton((b) => + b.setButtonText('Retry').onClick(() => { + this.listEl.empty(); + this.listEl.createEl('p', { text: 'Loading your memories…', cls: 'ams-hint' }); + void this.fillList(); + }) + ); + return; + } + if (!res.vaults.length) { this.listEl.createEl('p', { text: 'No memories yet — create one below.', cls: 'ams-hint' }); return; } - for (const v of vaults) { + for (const v of res.vaults) { const isCur = v.name === cur; const meta = describeVault(v); const row = new Setting(this.listEl) .setName(v.name) - .setDesc(isCur ? `current · ${meta}` : meta) - .addButton((b) => - b - .setButtonText(isCur ? 'In use' : 'Use') - .setDisabled(isCur) - .onClick(async () => { - await this.host.selectVault(v.name); - this.close(); - }) - ); - // Sync now only for the memory in use; switch to another with Use first. + .setDesc(isCur ? `current · ${meta}` : meta); if (isCur) { + // Already in use → offer Sync now. row.addButton((b) => b .setCta() @@ -111,6 +116,25 @@ class MemoryModal extends Modal { this.close(); }) ); + } else { + // Switch to it: Use (select only) or Use & sync (select + sync immediately). + row + .addButton((b) => + b.setButtonText('Use').onClick(async () => { + await this.host.selectVault(v.name); + this.close(); + }) + ) + .addButton((b) => + b + .setCta() + .setButtonText('Use & sync') + .onClick(async () => { + b.setDisabled(true).setButtonText('Syncing…'); + await this.host.syncVault(v.name); + this.close(); + }) + ); } } } diff --git a/src/settings.ts b/src/settings.ts index b24ecfb..94c610f 100644 --- a/src/settings.ts +++ b/src/settings.ts @@ -47,6 +47,10 @@ export interface VaultInfo { updated: string | null; empty: boolean; } + +// Result of listing memories: lets the chooser tell a genuinely empty account ({ok,vaults:[]}) +// from a server/network failure ({ok:false}), instead of both rendering as "No memories yet". +export type VaultListResult = { ok: true; vaults: VaultInfo[] } | { ok: false; error: string }; export const DEFAULT_REMOTE_HOST = 'https://sync.agentage.io'; export const VAULTS_SCHEMA_URL = 'https://memory.agentage.io/schema/vaults.json'; // The managed remote alias. memory-core resolves "agentage" to the cloud git URL diff --git a/src/sync-controller.ts b/src/sync-controller.ts index b46e885..940f928 100644 --- a/src/sync-controller.ts +++ b/src/sync-controller.ts @@ -177,14 +177,41 @@ export function createSyncController(deps: SyncControllerDeps) { const staged = await stageChanges(ctx, new Set()); if (staged > 0) await deps.client.commit(ctx, `Sync ${deps.now()}`); - const { conflicted } = await deps.client.pull(ctx); - if (conflicted.length > 0) { - await writeConflictNote(conflicted); - status('conflict', `${conflicted.length} file(s) need resolution`); - return { action: 'synced', committed: staged > 0, pushed: false, conflicted }; + const pulled = await deps.client.pull(ctx); + if (pulled.unmergeable) { + const message = + "Automatic merge isn't possible for this history (diverged too far). " + + 'Resolve in another client or re-clone, then sync again.'; + status('conflict', message); + return { action: 'synced', committed: staged > 0, pushed: false, conflicted: [], message }; + } + if (pulled.conflicted.length > 0) { + await writeConflictNote(pulled.conflicted); + status('conflict', `${pulled.conflicted.length} file(s) need resolution`); + return { + action: 'synced', + committed: staged > 0, + pushed: false, + conflicted: pulled.conflicted, + }; } - await deps.client.push(ctx); + const pushed = await deps.client.push(ctx); + if (pushed.unmergeable || pushed.conflicted.length > 0) { + // A concurrent push landed; the re-pull conflicted — surface it instead of erroring. + if (pushed.conflicted.length > 0) await writeConflictNote(pushed.conflicted); + const message = pushed.unmergeable + ? "Automatic merge isn't possible for this history (diverged too far). Resolve and sync again." + : `${pushed.conflicted.length} file(s) need resolution`; + status('conflict', message); + return { + action: 'synced', + committed: staged > 0, + pushed: false, + conflicted: pushed.conflicted, + message, + }; + } status('idle'); return { action: 'synced', committed: staged > 0, pushed: true, conflicted: [] }; }