From 5293eaff29f8613e4f9d611c32aadd9ab4df6036 Mon Sep 17 00:00:00 2001 From: can1357 Date: Fri, 6 Mar 2026 16:05:04 +0100 Subject: [PATCH] feat(ai): added credential disable tracking with auditability improvements - Added `disabledCause` parameter to credential deletion methods to track reason credentials are disabled. - Changed credential disabling mechanism from boolean `disabled` flag to `disabled_cause` text field for better auditability. - Fixed credential purging to respect disabled credentials during email deduplication operations. - Refactored `replaceAuthCredentialsForProvider()` to update matching credentials instead of deleting all, preserving credential history. --- packages/ai/CHANGELOG.md | 4 +- packages/ai/src/auth-storage.ts | 122 +++++++++++++----- .../ai/test/auth-storage-email-dedupe.test.ts | 68 ++++++++++ packages/coding-agent/CHANGELOG.md | 3 + .../coding-agent/src/session/agent-storage.ts | 28 ++-- 5 files changed, 175 insertions(+), 50 deletions(-) diff --git a/packages/ai/CHANGELOG.md b/packages/ai/CHANGELOG.md index 1b4701a2a..c536430d0 100644 --- a/packages/ai/CHANGELOG.md +++ b/packages/ai/CHANGELOG.md @@ -1,7 +1,6 @@ # Changelog ## [Unreleased] - ### Breaking Changes - Changed `reasoning` parameter from `ThinkingLevel | undefined` to `Effort | undefined` in `SimpleStreamOptions`; 'off' is no longer valid (omit the field instead) @@ -32,6 +31,8 @@ ### Changed +- Changed credential disabling mechanism from boolean `disabled` flag to `disabled_cause` text field for tracking why credentials were disabled +- Changed `deleteAuthCredential()` and `deleteAuthCredentialsForProvider()` methods to require a `disabledCause` parameter explaining the reason for disabling - Changed Gemini model parsing to strip `-preview` suffix for consistent model identification - Changed OpenAI Codex websocket error handling to detect fatal connection errors and immediately fall back to SSE without retrying - Changed OpenAI Codex to always use websockets v2 protocol (removed v1 support) @@ -65,6 +66,7 @@ ### Fixed +- Fixed credential purging to respect disabled credentials when deduplicating by email, preventing re-enablement of intentionally disabled credentials - Fixed OpenAI Codex websocket error reporting to include detailed error messages from error events - Fixed conversation history reconstruction to support incremental updates from multiple assistant messages while maintaining backward compatibility with full-snapshot payloads - Fixed OpenAI Codex to reject unsupported effort levels instead of silently clamping them, providing clear error messages about supported efforts diff --git a/packages/ai/src/auth-storage.ts b/packages/ai/src/auth-storage.ts index faf2c5f43..4d3426351 100644 --- a/packages/ai/src/auth-storage.ts +++ b/packages/ai/src/auth-storage.ts @@ -108,6 +108,7 @@ export interface StoredAuthCredential { id: number; provider: string; credential: AuthCredential; + disabledCause: string | null; } // ───────────────────────────────────────────────────────────────────────────── @@ -484,7 +485,7 @@ export class AuthStorage { } if (removed.length > 0) { for (const entry of removed) { - this.#store.deleteAuthCredential(entry.id); + this.#store.deleteAuthCredential(entry.id, "deduplicated duplicate credential"); } this.#resetProviderAssignments(provider); } @@ -659,10 +660,10 @@ export class AuthStorage { * The credential remains in the database but is excluded from active queries. * Cleans up provider entry if last credential disabled. */ - #removeCredentialAt(provider: string, index: number): void { + #disableCredentialAt(provider: string, index: number, disabledCause: string): void { const entries = this.#getStoredCredentials(provider); if (index < 0 || index >= entries.length) return; - this.#store.deleteAuthCredential(entries[index].id); + this.#store.deleteAuthCredential(entries[index].id, disabledCause); const updated = entries.filter((_value, idx) => idx !== index); this.#setStoredCredentials(provider, updated); this.#resetProviderAssignments(provider); @@ -693,7 +694,7 @@ export class AuthStorage { * Remove credential for a provider. */ async remove(provider: string): Promise { - this.#store.deleteAuthCredentialsForProvider(provider); + this.#store.deleteAuthCredentialsForProvider(provider, "deleted by user"); this.#data.delete(provider); this.#resetProviderAssignments(provider); } @@ -1858,8 +1859,8 @@ export class AuthStorage { }); if (isDefinitiveFailure) { - // Permanently remove invalid credentials - this.#removeCredentialAt(provider, selection.index); + // Permanently disable invalid credentials with an explicit cause for inspection/debugging + this.#disableCredentialAt(provider, selection.index, `oauth refresh failed: ${errorMsg}`); if (this.#getCredentialsForProvider(provider).some(credential => credential.type === "oauth")) { return this.getApiKey(provider, sessionId, options); } @@ -1950,6 +1951,7 @@ type AuthRow = { provider: string; credential_type: string; data: string; + disabled_cause: string | null; }; function serializeCredential( @@ -1993,6 +1995,26 @@ function deserializeCredential(row: AuthRow): AuthCredential | null { return null; } +function normalizeDisabledCause(disabledCause: string): string { + const normalized = disabledCause.trim(); + return normalized.length > 0 ? normalized : "disabled"; +} + +function toStoredAuthCredential(row: AuthRow, credential: AuthCredential): StoredAuthCredential { + return { id: row.id, provider: row.provider, credential, disabledCause: row.disabled_cause }; +} + +/** Returns a stable identity string for matching credentials across replace operations. */ +function credentialIdentity(credential: AuthCredential): string | null { + if (credential.type === "api_key") return `api_key:${credential.key}`; + if (credential.type === "oauth") { + if (credential.accountId) return `account:${credential.accountId}`; + const [email] = extractCredentialEmails(credential); + if (email) return `email:${email}`; + } + return null; +} + /** Extracts normalized email identifiers from a credential, including JWT profile claims. */ function extractCredentialEmails(credential: AuthCredential): string[] { if (credential.type !== "oauth") return []; @@ -2053,13 +2075,13 @@ export class AuthCredentialStore { this.#initializeSchema(); this.#listActiveStmt = this.#db.prepare( - "SELECT id, provider, credential_type, data FROM auth_credentials WHERE disabled = 0 ORDER BY id ASC", + "SELECT id, provider, credential_type, data, disabled_cause FROM auth_credentials WHERE disabled_cause IS NULL ORDER BY id ASC", ); this.#listActiveByProviderStmt = this.#db.prepare( - "SELECT id, provider, credential_type, data FROM auth_credentials WHERE provider = ? AND disabled = 0 ORDER BY id ASC", + "SELECT id, provider, credential_type, data, disabled_cause FROM auth_credentials WHERE provider = ? AND disabled_cause IS NULL ORDER BY id ASC", ); this.#listDisabledByProviderStmt = this.#db.prepare( - "SELECT id, credential_type, data FROM auth_credentials WHERE provider = ? AND disabled = 1 ORDER BY id ASC", + "SELECT id, provider, credential_type, data, disabled_cause FROM auth_credentials WHERE provider = ? AND disabled_cause IS NOT NULL ORDER BY id ASC", ); this.#insertStmt = this.#db.prepare( "INSERT INTO auth_credentials (provider, credential_type, data) VALUES (?, ?, ?) RETURNING id", @@ -2068,10 +2090,10 @@ export class AuthCredentialStore { "UPDATE auth_credentials SET credential_type = ?, data = ?, updated_at = unixepoch() WHERE id = ?", ); this.#deleteStmt = this.#db.prepare( - "UPDATE auth_credentials SET disabled = 1, updated_at = unixepoch() WHERE id = ?", + "UPDATE auth_credentials SET disabled_cause = ?, updated_at = unixepoch() WHERE id = ?", ); this.#deleteByProviderStmt = this.#db.prepare( - "UPDATE auth_credentials SET disabled = 1, updated_at = unixepoch() WHERE provider = ?", + "UPDATE auth_credentials SET disabled_cause = ?, updated_at = unixepoch() WHERE provider = ? AND disabled_cause IS NULL", ); this.#hardDeleteStmt = this.#db.prepare("DELETE FROM auth_credentials WHERE id = ?"); this.#getCacheStmt = this.#db.prepare("SELECT value FROM cache WHERE key = ? AND expires_at > unixepoch()"); @@ -2112,7 +2134,7 @@ CREATE TABLE IF NOT EXISTS auth_credentials ( provider TEXT NOT NULL, credential_type TEXT NOT NULL, data TEXT NOT NULL, - disabled INTEGER NOT NULL DEFAULT 0, + disabled_cause TEXT DEFAULT NULL, created_at INTEGER NOT NULL DEFAULT (unixepoch()), updated_at INTEGER NOT NULL DEFAULT (unixepoch()) ); @@ -2126,10 +2148,18 @@ CREATE TABLE IF NOT EXISTS cache ( CREATE INDEX IF NOT EXISTS idx_cache_expires ON cache(expires_at); `); - // Migration: add disabled column if missing (for databases created by old CliAuthStorage) const cols = this.#db.prepare("PRAGMA table_info(auth_credentials)").all() as Array<{ name?: string }>; - if (!cols.some(c => c.name === "disabled")) { - this.#db.exec("ALTER TABLE auth_credentials ADD COLUMN disabled INTEGER NOT NULL DEFAULT 0"); + const hasDisabledCause = cols.some(c => c.name === "disabled_cause"); + const hasDisabled = cols.some(c => c.name === "disabled"); + if (!hasDisabledCause) { + this.#db.exec("ALTER TABLE auth_credentials ADD COLUMN disabled_cause TEXT DEFAULT NULL"); + } + if (hasDisabled) { + this.#db.exec(` + UPDATE auth_credentials + SET disabled_cause = COALESCE(disabled_cause, 'disabled') + WHERE disabled = 1 AND disabled_cause IS NULL + `); } } @@ -2145,26 +2175,52 @@ CREATE INDEX IF NOT EXISTS idx_cache_expires ON cache(expires_at); for (const row of rows) { const credential = deserializeCredential(row); if (!credential) continue; - results.push({ id: row.id, provider: row.provider, credential }); + results.push(toStoredAuthCredential(row, credential)); } return results; } replaceAuthCredentialsForProvider(provider: string, credentials: AuthCredential[]): StoredAuthCredential[] { const replace = this.#db.transaction((providerName: string, items: AuthCredential[]) => { - this.#deleteByProviderStmt.run(providerName); - const inserted: StoredAuthCredential[] = []; + const existingRows = this.#listActiveByProviderStmt.all(providerName) as AuthRow[]; + const existing: Array<{ id: number; credential: AuthCredential; identity: string | null }> = []; + for (const row of existingRows) { + const credential = deserializeCredential(row); + if (!credential) continue; + existing.push({ id: row.id, credential, identity: credentialIdentity(credential) }); + } + + const result: StoredAuthCredential[] = []; + const matchedExistingIds = new Set(); + for (const credential of items) { const serialized = serializeCredential(credential); if (!serialized) continue; - const row = this.#insertStmt.get(providerName, serialized.credentialType, serialized.data) as - | { id?: number } - | undefined; - if (row?.id) { - inserted.push({ id: row.id, provider: providerName, credential }); + const identity = credentialIdentity(credential); + const match = identity + ? existing.find(e => e.identity === identity && !matchedExistingIds.has(e.id)) + : null; + if (match) { + matchedExistingIds.add(match.id); + this.#updateStmt.run(serialized.credentialType, serialized.data, match.id); + result.push({ id: match.id, provider: providerName, credential, disabledCause: null }); + } else { + const row = this.#insertStmt.get(providerName, serialized.credentialType, serialized.data) as + | { id?: number } + | undefined; + if (row?.id) { + result.push({ id: row.id, provider: providerName, credential, disabledCause: null }); + } } } - return inserted; + + for (const row of existing) { + if (!matchedExistingIds.has(row.id)) { + this.#deleteStmt.run("replaced by newer credential", row.id); + } + } + + return result; }); const result = replace(provider, credentials); @@ -2187,13 +2243,9 @@ CREATE INDEX IF NOT EXISTS idx_cache_expires ON cache(expires_at); } if (activeEmails.size === 0) return; - const disabledRows = this.#listDisabledByProviderStmt.all(provider) as Array<{ - id: number; - credential_type: string; - data: string; - }>; + const disabledRows = this.#listDisabledByProviderStmt.all(provider) as AuthRow[]; for (const row of disabledRows) { - const credential = deserializeCredential({ ...row, provider }); + const credential = deserializeCredential(row); if (!credential) { this.#hardDeleteStmt.run(row.id); continue; @@ -2224,17 +2276,17 @@ CREATE INDEX IF NOT EXISTS idx_cache_expires ON cache(expires_at); } } - deleteAuthCredential(id: number): void { + deleteAuthCredential(id: number, disabledCause: string): void { try { - this.#deleteStmt.run(id); + this.#deleteStmt.run(normalizeDisabledCause(disabledCause), id); } catch { // Ignore delete failures } } - deleteAuthCredentialsForProvider(provider: string): void { + deleteAuthCredentialsForProvider(provider: string, disabledCause: string): void { try { - this.#deleteByProviderStmt.run(provider); + this.#deleteByProviderStmt.run(normalizeDisabledCause(disabledCause), provider); } catch { // Ignore delete failures } @@ -2328,7 +2380,7 @@ CREATE INDEX IF NOT EXISTS idx_cache_expires ON cache(expires_at); * Delete all credentials for a provider. */ deleteProvider(provider: string): void { - this.deleteAuthCredentialsForProvider(provider); + this.deleteAuthCredentialsForProvider(provider, "deleted by user"); } close(): void { diff --git a/packages/ai/test/auth-storage-email-dedupe.test.ts b/packages/ai/test/auth-storage-email-dedupe.test.ts index 1371bfd6a..89dbf6380 100644 --- a/packages/ai/test/auth-storage-email-dedupe.test.ts +++ b/packages/ai/test/auth-storage-email-dedupe.test.ts @@ -48,6 +48,20 @@ function countCredentialRows(dbPath: string, provider: string): number { } } +function readDisabledCauses(dbPath: string, provider: string): string[] { + const db = new Database(dbPath, { readonly: true }); + try { + const rows = db + .prepare( + "SELECT disabled_cause FROM auth_credentials WHERE provider = ? AND disabled_cause IS NOT NULL ORDER BY id ASC", + ) + .all(provider) as Array<{ disabled_cause?: string | null }>; + return rows.flatMap(row => (typeof row.disabled_cause === "string" ? [row.disabled_cause] : [])); + } finally { + db.close(); + } +} + describe("AuthStorage openai-codex email dedupe", () => { let tempDir = ""; let dbPath = ""; @@ -171,4 +185,58 @@ describe("AuthStorage openai-codex email dedupe", () => { const credentials = store.listAuthCredentials("openai-codex"); expect(credentials).toHaveLength(2); }); + + it("stores the disable cause when a credential is soft-disabled", async () => { + if (!store || !dbPath) throw new Error("test setup failed"); + + store.replaceAuthCredentialsForProvider("openai-codex", [ + createCredential({ suffix: "only", accountId: "account-a", email: "only@example.com" }), + ]); + + const [credential] = store.listAuthCredentials("openai-codex"); + if (!credential) throw new Error("expected stored credential"); + + const disabledCause = "oauth refresh failed: invalid_grant"; + store.deleteAuthCredential(credential.id, disabledCause); + + expect(store.listAuthCredentials("openai-codex")).toHaveLength(0); + expect(readDisabledCauses(dbPath, "openai-codex")).toEqual([disabledCause]); + }); + + it("backfills a default disabled cause when migrating legacy disabled rows", async () => { + if (!tempDir) throw new Error("test setup failed"); + + const legacyDbPath = path.join(tempDir, "legacy-agent.db"); + const legacyDb = new Database(legacyDbPath); + legacyDb.exec(` + CREATE TABLE auth_credentials ( + id INTEGER PRIMARY KEY AUTOINCREMENT, + provider TEXT NOT NULL, + credential_type TEXT NOT NULL, + data TEXT NOT NULL, + disabled INTEGER NOT NULL DEFAULT 0, + created_at INTEGER NOT NULL DEFAULT (unixepoch()), + updated_at INTEGER NOT NULL DEFAULT (unixepoch()) + ); + `); + legacyDb + .prepare("INSERT INTO auth_credentials (provider, credential_type, data, disabled) VALUES (?, ?, ?, ?)") + .run( + "openai-codex", + "oauth", + JSON.stringify( + createCredential({ suffix: "legacy", accountId: "legacy-account", email: "legacy@example.com" }), + ), + 1, + ); + legacyDb.close(); + + const migratedStore = await AuthCredentialStore.open(legacyDbPath); + try { + expect(migratedStore.listAuthCredentials("openai-codex")).toHaveLength(0); + expect(readDisabledCauses(legacyDbPath, "openai-codex")).toEqual(["disabled"]); + } finally { + migratedStore.close(); + } + }); }); diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index 1e19c7ca8..0114a6d25 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -1,6 +1,7 @@ # Changelog ## [Unreleased] + ### Breaking Changes - Changed `ThinkingLevel` type to be imported from `@oh-my-pi/pi-agent-core` instead of `@oh-my-pi/pi-ai` @@ -27,6 +28,8 @@ ### Changed +- Changed credential deletion to disable credentials with persisted cause instead of permanent deletion +- Added `disabledCause` parameter to credential deletion methods to track reason for disabling - Changed thinking level parsing to use `parseEffort()` from local thinking module instead of `parseThinkingLevel()` from pi-ai - Changed model list display to show supported thinking efforts (e.g., "low,medium,high") instead of yes/no reasoning indicator - Changed footer and status line to check `model.thinking` instead of `model.reasoning` for thinking level display diff --git a/packages/coding-agent/src/session/agent-storage.ts b/packages/coding-agent/src/session/agent-storage.ts index ad4725481..1731b3216 100644 --- a/packages/coding-agent/src/session/agent-storage.ts +++ b/packages/coding-agent/src/session/agent-storage.ts @@ -276,22 +276,20 @@ CREATE TABLE settings ( * @returns Array of stored credentials with their database IDs */ listAuthCredentials(provider?: string, includeDisabled = false): StoredAuthCredential[] { - // AuthCredentialStore doesn't expose includeDisabled yet, so we filter if needed const credentials = this.#authStore.listAuthCredentials(provider); if (!includeDisabled) return credentials; - // For now, includeDisabled requires direct DB access - // This is only used internally, so it's acceptable const stmt = this.#db.prepare( provider - ? "SELECT id, provider, credential_type, data FROM auth_credentials WHERE provider = ? ORDER BY id ASC" - : "SELECT id, provider, credential_type, data FROM auth_credentials ORDER BY id ASC", + ? "SELECT id, provider, credential_type, data, disabled_cause FROM auth_credentials WHERE provider = ? ORDER BY id ASC" + : "SELECT id, provider, credential_type, data, disabled_cause FROM auth_credentials ORDER BY id ASC", ); const rows = (provider ? stmt.all(provider) : stmt.all()) as Array<{ id: number; provider: string; credential_type: string; data: string; + disabled_cause: string | null; }>; const results: StoredAuthCredential[] = []; @@ -309,7 +307,7 @@ CREATE TABLE settings ( continue; } - results.push({ id: row.id, provider: row.provider, credential }); + results.push({ id: row.id, provider: row.provider, credential, disabledCause: row.disabled_cause }); } catch {} } return results; @@ -336,19 +334,21 @@ CREATE TABLE settings ( } /** - * Deletes an auth credential by ID. - * @param id - Database row ID of the credential to delete + * Disables an auth credential by ID with a persisted cause. + * @param id - Database row ID of the credential to disable + * @param disabledCause - Human-readable cause stored with the disabled row */ - deleteAuthCredential(id: number): void { - this.#authStore.deleteAuthCredential(id); + deleteAuthCredential(id: number, disabledCause: string): void { + this.#authStore.deleteAuthCredential(id, disabledCause); } /** - * Deletes all auth credentials for a provider. - * @param provider - Provider name whose credentials should be deleted + * Disables all auth credentials for a provider with a persisted cause. + * @param provider - Provider name whose credentials should be disabled + * @param disabledCause - Human-readable cause stored with the disabled rows */ - deleteAuthCredentialsForProvider(provider: string): void { - this.#authStore.deleteAuthCredentialsForProvider(provider); + deleteAuthCredentialsForProvider(provider: string, disabledCause: string): void { + this.#authStore.deleteAuthCredentialsForProvider(provider, disabledCause); } /**