From 9e9a8bce8d93013b92f5a399a49f9571574b903d Mon Sep 17 00:00:00 2001 From: Haider Date: Thu, 1 Oct 2026 06:03:26 +0530 Subject: [PATCH 1/3] fix(warehouse): reopen closed Snowflake connections, and keep saved passwords Two causes of warehouse connections failing after working earlier: 1. A Snowflake connection Snowflake had closed (idle session expiry, VPN drop, laptop sleep) stayed cached, and every later statement failed with "Unable to perform operation using terminated connection" until restart. The driver now: - sets `clientSessionKeepAlive` (default on; `client_session_keep_alive: false` opts out), - checks `isUp()` before each statement and reopens a dead connection with the same options (concurrent callers share one reconnect), - retries once on errors raised before the statement reached Snowflake (407002, 390111, 390112, 390114), so nothing can run twice. 2. Passwords, keys and tokens added through `warehouse_add` were never saved. The credential store only used keytar, an optional external nothing installs, so every sensitive field was stripped from the saved connection: it worked in the session that added it, then failed after a restart with the driver's "A password must be specified". The store now falls back to Bun's built-in `Bun.secrets` (macOS Keychain, Windows Credential Manager, libsecret), keeps the same service and account names, and reports a failed write instead of claiming success. `warehouse_add` now shows the warning when a credential could not be stored. The test preload disables the OS store so the suite never touches a real keychain. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_018fJ3X7pcGT4R9yzjsJnqsV --- docs/docs/configure/warehouses.md | 4 + packages/drivers/src/snowflake.ts | 124 ++++++++++++--- .../drivers/test/snowflake-reconnect.test.ts | 142 ++++++++++++++++++ .../native/connections/credential-store.ts | 132 ++++++++++++---- .../altimate/native/connections/registry.ts | 2 +- .../opencode/src/altimate/native/types.ts | 2 + .../src/altimate/tools/warehouse-add.ts | 8 + .../altimate/credential-store-backend.test.ts | 85 +++++++++++ packages/opencode/test/preload.ts | 4 + 9 files changed, 447 insertions(+), 56 deletions(-) create mode 100644 packages/drivers/test/snowflake-reconnect.test.ts create mode 100644 packages/opencode/test/altimate/credential-store-backend.test.ts diff --git a/docs/docs/configure/warehouses.md b/docs/docs/configure/warehouses.md index 02bdff8949..99b1748033 100644 --- a/docs/docs/configure/warehouses.md +++ b/docs/docs/configure/warehouses.md @@ -23,6 +23,9 @@ Each warehouse has a key (the connection name) and a config object: !!! tip Use `{env:...}` substitution for passwords and tokens so you never commit secrets to version control. +!!! note "Where saved passwords go" + When the agent adds a connection for you, passwords, keys and tokens are kept in your OS credential store (macOS Keychain, Windows Credential Manager, or libsecret on Linux), never in `connections.json`. If no credential store is available, the credential is not saved: the connection works for that session only, and Altimate Code says so. Use an `ALTIMATE_CODE_CONN_` environment variable instead in that case. + ## Snowflake ```json @@ -50,6 +53,7 @@ Each warehouse has a key (the connection name) and a config object: | `database` | No | Database name | | `schema` | No | Schema name | | `role` | No | User role | +| `client_session_keep_alive` | No | Keep the session alive while Altimate Code is running (default `true`). A connection Snowflake closes anyway is reopened automatically. | ### Key-pair authentication diff --git a/packages/drivers/src/snowflake.ts b/packages/drivers/src/snowflake.ts index 9cafa5a0f5..113757742f 100644 --- a/packages/drivers/src/snowflake.ts +++ b/packages/drivers/src/snowflake.ts @@ -51,9 +51,32 @@ export function suppressSnowflakeLogging(snowflake: any): void { }) } -export async function connect(config: ConnectionConfig): Promise { +/** + * Errors meaning the statement never reached Snowflake because the session was + * already gone, so reconnecting and sending it again cannot run it twice: + * 407002 is raised client-side for a terminated connection; 390111 (session no + * longer exists), 390112 (session expired) and 390114 (session token expired) + * are the server refusing the request before executing anything. + */ +const CLOSED_SESSION_CODES = new Set(["407002", "390111", "390112", "390114"]) + +export function isClosedConnectionError(err: unknown): boolean { + const code = String((err as { code?: unknown } | null)?.code ?? "") + if (CLOSED_SESSION_CODES.has(code)) return true + return /unable to perform operation using terminated connection/i.test(String((err as Error)?.message ?? err)) +} + +/** `client_session_keep_alive` / `clientSessionKeepAlive`; on unless explicitly false. */ +export function keepAliveSetting(config: ConnectionConfig): boolean { + const value = config.client_session_keep_alive ?? config.clientSessionKeepAlive + if (value === false || value === "false" || value === 0 || value === "0") return false + return true +} + +/** `sdk` replaces the installed snowflake-sdk in tests. */ +export async function connect(config: ConnectionConfig, sdk?: unknown): Promise { let snowflake: any - snowflake = await loadOptionalDriver("snowflake", "snowflake-sdk") + snowflake = sdk ?? (await loadOptionalDriver("snowflake", "snowflake-sdk")) snowflake = snowflake.default || snowflake // Suppress snowflake-sdk's Winston console logging as early as possible — it @@ -62,12 +85,78 @@ export async function connect(config: ConnectionConfig): Promise { suppressSnowflakeLogging(snowflake) let connection: any + /** Kept from connect() so a dead connection can be reopened the same way. */ + let connectOptions: Record | undefined + let connectViaBrowser = false + let reconnecting: Promise | undefined + + function openConnection(): Promise { + return new Promise((resolve, reject) => { + const conn = snowflake.createConnection(connectOptions) + if (connectViaBrowser) { + if (typeof conn.connectAsync !== "function") { + reject(new Error("Snowflake browser/SSO auth requires snowflake-sdk with connectAsync support. Upgrade snowflake-sdk.")) + return + } + conn.connectAsync((err: Error | null) => { + if (err) reject(err) + else resolve(conn) + }).catch(reject) + } else { + conn.connect((err: Error | null) => { + if (err) reject(err) + else resolve(conn) + }) + } + }) + } + + /** Replace a connection Snowflake has closed. Concurrent callers share one reconnect. */ + function reconnect(): Promise { + if (!reconnecting) { + const previous = connection + reconnecting = openConnection() + .then((conn) => { + connection = conn + suppressSnowflakeLogging(snowflake) + try { + previous?.destroy?.(() => {}) + } catch { + // already terminated — nothing to release + } + }) + .finally(() => { + reconnecting = undefined + }) + } + return reconnecting + } + + /** Reopen before use when the SDK already knows the connection is gone (idle timeout, network drop, sleep). */ + async function ensureLive(): Promise { + if (reconnecting) return reconnecting + if (connection && connectOptions && typeof connection.isUp === "function" && !connection.isUp()) { + await reconnect() + } + } function escapeSqlIdentifier(value: string): string { return value.replace(/"/g, '""') } - function executeQuery(sql: string, binds?: any[]): Promise<{ columns: string[]; rows: any[][] }> { + /** Run a statement, reopening the connection and retrying once if it was already closed when the statement was sent. */ + async function executeQuery(sql: string, binds?: any[]): Promise<{ columns: string[]; rows: any[][] }> { + await ensureLive() + try { + return await runQuery(sql, binds) + } catch (err) { + if (!connectOptions || !isClosedConnectionError(err)) throw err + await reconnect() + return runQuery(sql, binds) + } + } + + function runQuery(sql: string, binds?: any[]): Promise<{ columns: string[]; rows: any[][] }> { return new Promise((resolve, reject) => { const options: Record = { sqlText: sql, @@ -98,6 +187,11 @@ export async function connect(config: ConnectionConfig): Promise { schema: config.schema, warehouse: config.warehouse, role: config.role, + // Without it Snowflake ends an idle session (the master token lasts + // four hours), and every later statement fails with "terminated + // connection" until the process restarts. The heartbeat does not + // resume a suspended warehouse. + clientSessionKeepAlive: keepAliveSetting(config), } // --------------------------------------------------------------- @@ -242,26 +336,10 @@ export async function connect(config: ConnectionConfig): Promise { // Use connectAsync for browser-based auth (SSO/Okta), connect for everything else const isOktaUrl = authenticator && /^https?:\/\/.+\.okta\.com/i.test(authenticator) - const useBrowserAuth = authUpper === "EXTERNALBROWSER" || isOktaUrl - - connection = await new Promise((resolve, reject) => { - const conn = snowflake.createConnection(options) - if (useBrowserAuth) { - if (typeof conn.connectAsync !== "function") { - reject(new Error("Snowflake browser/SSO auth requires snowflake-sdk with connectAsync support. Upgrade snowflake-sdk.")) - return - } - conn.connectAsync((err: Error | null) => { - if (err) reject(err) - else resolve(conn) - }).catch(reject) - } else { - conn.connect((err: Error | null) => { - if (err) reject(err) - else resolve(conn) - }) - } - }) + connectViaBrowser = Boolean(authUpper === "EXTERNALBROWSER" || isOktaUrl) + connectOptions = options + + connection = await openConnection() // Re-apply suppression: Snowflake "Easy Logging" reads a client-config file // during connect() and can re-raise the log level (and re-attach a console diff --git a/packages/drivers/test/snowflake-reconnect.test.ts b/packages/drivers/test/snowflake-reconnect.test.ts new file mode 100644 index 0000000000..ca4b2622a8 --- /dev/null +++ b/packages/drivers/test/snowflake-reconnect.test.ts @@ -0,0 +1,142 @@ +/** + * A Snowflake connection that the SDK reports as closed (idle session expiry, + * VPN drop, laptop sleep) is reopened instead of failing every later statement + * with "Unable to perform operation using terminated connection" until restart. + */ +import { describe, test, expect } from "bun:test" +import { connect, isClosedConnectionError, keepAliveSetting } from "../src/snowflake" + +type Behaviour = { up: boolean; failNextWith?: { code: unknown; message: string } } + +/** A minimal stand-in for snowflake-sdk: records connections and lets a test close them. */ +function fakeSdk() { + const created: Array<{ options: any; state: Behaviour; destroyed: boolean; executed: string[] }> = [] + const sdk = { + configure() {}, + createConnection(options: any) { + const entry = { options, state: { up: true } as Behaviour, destroyed: false, executed: [] as string[] } + created.push(entry) + return { + connect(cb: (err: Error | null) => void) { + setTimeout(() => cb(null), 1) + }, + isUp: () => entry.state.up, + execute(opts: { sqlText: string; complete: (err: any, stmt: any, rows: any[]) => void }) { + if (entry.state.failNextWith) { + const f = entry.state.failNextWith + entry.state.failNextWith = undefined + const err = Object.assign(new Error(f.message), { code: f.code }) + setTimeout(() => opts.complete(err, null, []), 1) + return + } + entry.executed.push(opts.sqlText) + setTimeout(() => opts.complete(null, null, [{ N: created.indexOf(entry) }]), 1) + }, + destroy(cb: (err: Error | null) => void) { + entry.destroyed = true + cb(null) + }, + } + }, + } + return { sdk, created } +} + +const config = { type: "snowflake", account: "acct", user: "u", password: "p" } as any + +describe("Snowflake connection lifecycle", () => { + test("keep-alive is on by default and can be turned off", async () => { + const { sdk, created } = fakeSdk() + const c = await connect(config, sdk) + await c.connect() + expect(created[0].options.clientSessionKeepAlive).toBe(true) + + const off = fakeSdk() + const c2 = await connect({ ...config, client_session_keep_alive: false }, off.sdk) + await c2.connect() + expect(off.created[0].options.clientSessionKeepAlive).toBe(false) + }) + + test("a connection the SDK reports as down is reopened before the statement runs", async () => { + const { sdk, created } = fakeSdk() + const c = await connect(config, sdk) + await c.connect() + created[0].state.up = false + + const result = await c.execute("SELECT 1") + expect(created.length).toBe(2) + expect(created[0].destroyed).toBe(true) + expect(created[0].executed).toEqual([]) + expect(created[1].executed).toEqual(["SELECT 1 LIMIT 1001"]) + expect(result.rows).toEqual([[1]]) + // The reopened connection keeps the original options, keep-alive included. + expect(created[1].options).toEqual(created[0].options) + }) + + test("a statement refused because the session was already closed is retried once on a new connection", async () => { + const { sdk, created } = fakeSdk() + const c = await connect(config, sdk) + await c.connect() + // isUp() still says yes, but the SDK refuses the statement: the race isUp() cannot close. + created[0].state.failNextWith = { code: 407002, message: "Unable to perform operation using terminated connection." } + + const result = await c.execute("SELECT 1") + expect(created.length).toBe(2) + expect(created[1].executed).toEqual(["SELECT 1 LIMIT 1001"]) + expect(result.rows).toEqual([[1]]) + }) + + test("an expired session token (390114) is also retried", async () => { + const { sdk, created } = fakeSdk() + const c = await connect(config, sdk) + await c.connect() + created[0].state.failNextWith = { code: "390114", message: "Authentication token has expired." } + await c.execute("SELECT 1") + expect(created.length).toBe(2) + }) + + test("any other error is not retried, so a statement is never sent twice", async () => { + const { sdk, created } = fakeSdk() + const c = await connect(config, sdk) + await c.connect() + created[0].state.failNextWith = { code: "002003", message: "SQL compilation error: Object 'X' does not exist" } + await expect(c.execute("INSERT INTO x VALUES (1)")).rejects.toThrow("does not exist") + expect(created.length).toBe(1) + }) + + test("concurrent statements on a dead connection share one reconnect", async () => { + const { sdk, created } = fakeSdk() + const c = await connect(config, sdk) + await c.connect() + created[0].state.up = false + await Promise.all([c.execute("SELECT 1"), c.execute("SELECT 2"), c.listSchemas()]) + expect(created.length).toBe(2) + expect(created[1].executed.length).toBe(3) + }) +}) + +describe("isClosedConnectionError", () => { + test("matches only errors raised before the statement reached Snowflake", () => { + expect(isClosedConnectionError({ code: 407002 })).toBe(true) + expect(isClosedConnectionError({ code: "407002" })).toBe(true) + expect(isClosedConnectionError({ code: "390114" })).toBe(true) + expect(isClosedConnectionError({ code: "390111" })).toBe(true) + expect(isClosedConnectionError({ code: 390112 })).toBe(true) + expect(isClosedConnectionError(new Error("Unable to perform operation using terminated connection."))).toBe(true) + for (const e of [null, undefined, "boom", { code: 390100 }, { code: "002003" }, new Error("Network error"), new Error("Request timed out")]) { + expect(isClosedConnectionError(e)).toBe(false) + } + }) +}) + +describe("keepAliveSetting", () => { + test("only an explicit false turns keep-alive off", () => { + expect(keepAliveSetting({} as any)).toBe(true) + expect(keepAliveSetting({ client_session_keep_alive: true } as any)).toBe(true) + expect(keepAliveSetting({ clientSessionKeepAlive: "true" } as any)).toBe(true) + for (const off of [false, "false", 0, "0"]) { + expect(keepAliveSetting({ client_session_keep_alive: off } as any)).toBe(false) + expect(keepAliveSetting({ clientSessionKeepAlive: off } as any)).toBe(false) + } + }) +}) diff --git a/packages/opencode/src/altimate/native/connections/credential-store.ts b/packages/opencode/src/altimate/native/connections/credential-store.ts index 0074d6369b..feeaec5847 100644 --- a/packages/opencode/src/altimate/native/connections/credential-store.ts +++ b/packages/opencode/src/altimate/native/connections/credential-store.ts @@ -1,10 +1,18 @@ /** * Credential management for connection configs. * - * 3-tier fallback: - * 1. keytar (OS Keychain) — preferred, secure + * Fallback order: + * 1. The OS credential store — macOS Keychain, Windows Credential Manager, or + * libsecret on Linux — through keytar when it is installed, otherwise + * through Bun's built-in `Bun.secrets` * 2. ALTIMATE_CODE_CONN_* env vars — for headless/CI environments * 3. Refuse — never store plaintext credentials in config JSON + * + * keytar is an optional external that nothing installs, so in a released + * binary it is almost never present. Before `Bun.secrets` was used as the + * fallback, every credential was stripped from the saved connection: it + * worked for the session that added it, then failed after a restart with the + * driver's own "password must be specified". */ import { Log } from "@/altimate/util/log" @@ -36,62 +44,120 @@ const SENSITIVE_FIELDS = new Set([ "tls_ca_cert", ]) -/** Cached keytar module (or null if unavailable). */ -let keytarModule: any | null | undefined = undefined +/** One OS credential store. `account` is `/` under {@link SERVICE_NAME}. */ +export interface SecretBackend { + name: string + set(account: string, value: string): Promise + get(account: string): Promise + delete(account: string): Promise +} + +/** Turns the OS credential store off; set by the test preload so the suite never touches a real keychain. */ +const DISABLE_ENV = "ALTIMATE_CODE_DISABLE_OS_CREDENTIAL_STORE" + +/** Cached backend: `null` once probed and found unavailable. */ +let backend: SecretBackend | null | undefined = undefined -async function getKeytar(): Promise { - if (keytarModule !== undefined) return keytarModule +async function loadKeytar(): Promise { try { // @ts-expect-error — optional dependency, loaded at runtime - keytarModule = await import("keytar") - return keytarModule + const keytar = await import("keytar") + const k = keytar.default ?? keytar + if (typeof k?.setPassword !== "function") return null + return { + name: "keytar", + set: (account, value) => k.setPassword(SERVICE_NAME, account, value), + get: (account) => k.getPassword(SERVICE_NAME, account), + delete: (account) => k.deletePassword(SERVICE_NAME, account), + } } catch { + return null + } +} + +function loadBunSecrets(): SecretBackend | null { + const secrets = (globalThis as { Bun?: { secrets?: any } }).Bun?.secrets + if (typeof secrets?.set !== "function" || typeof secrets?.get !== "function") return null + return { + name: "Bun.secrets", + set: (account, value) => secrets.set({ service: SERVICE_NAME, name: account, value }), + get: async (account) => (await secrets.get({ service: SERVICE_NAME, name: account })) ?? null, + delete: async (account) => Boolean(await secrets.delete({ service: SERVICE_NAME, name: account })), + } +} + +async function getBackend(): Promise { + if (backend !== undefined) return backend + if (process.env[DISABLE_ENV]) { + backend = null + return backend + } + backend = (await loadKeytar()) ?? loadBunSecrets() + if (!backend) { Log.Default.warn( - "keytar not available — use ALTIMATE_CODE_CONN_* env vars for secure credential storage", + "no OS credential store available — use ALTIMATE_CODE_CONN_* env vars for secure credential storage", ) - keytarModule = null - return null } + return backend +} + +/** Swap the credential backend in tests. `undefined` re-probes on next use. */ +export function setSecretBackendForTests(next: SecretBackend | null | undefined): void { + backend = next } -/** Store a single credential in the OS keychain (or return false if unavailable). */ +/** Store a single credential in the OS credential store (or return false if unavailable). */ export async function storeCredential( connectionName: string, field: string, value: string, ): Promise { - const keytar = await getKeytar() - if (!keytar) return false - const account = `${connectionName}/${field}` - await keytar.setPassword(SERVICE_NAME, account, value) - return true + const store = await getBackend() + if (!store) return false + try { + await store.set(`${connectionName}/${field}`, value) + return true + } catch (e) { + // e.g. no libsecret on Linux, or a locked keychain: report "not stored" so + // the caller warns, rather than claiming success. + Log.Default.warn(`could not store '${field}' for connection '${connectionName}' in ${store.name}`, { + error: String(e), + }) + return false + } } -/** Retrieve a single credential from the OS keychain (or return null). */ +/** Retrieve a single credential from the OS credential store (or return null). */ export async function getCredential( connectionName: string, field: string, ): Promise { - const keytar = await getKeytar() - if (!keytar) return null - const account = `${connectionName}/${field}` - return keytar.getPassword(SERVICE_NAME, account) + const store = await getBackend() + if (!store) return null + try { + return await store.get(`${connectionName}/${field}`) + } catch { + return null + } } -/** Delete a single credential from the OS keychain. */ +/** Delete a single credential from the OS credential store. */ export async function deleteCredential( connectionName: string, field: string, ): Promise { - const keytar = await getKeytar() - if (!keytar) return false - const account = `${connectionName}/${field}` - return keytar.deletePassword(SERVICE_NAME, account) + const store = await getBackend() + if (!store) return false + try { + return await store.delete(`${connectionName}/${field}`) + } catch { + return false + } } /** - * Resolve a connection config by pulling sensitive fields from the keychain. - * If keytar is unavailable, returns the config as-is (credentials stay in JSON). + * Resolve a connection config by pulling sensitive fields from the OS credential store. + * If no store is available, returns the config as-is (credentials stay in JSON). */ export async function resolveConfig( name: string, @@ -125,10 +191,12 @@ export async function saveConnection( if (stored) { delete sanitized[field] } else { - // keytar unavailable — strip sensitive field from config to prevent + // No OS credential store — strip the sensitive field to prevent // plaintext storage. Users should use ALTIMATE_CODE_CONN_* env vars. - const warning = `Cannot securely store '${field}' for connection '${name}'. ` + - `Set ALTIMATE_CODE_CONN_${name.toUpperCase()} env var with full config JSON instead.` + const warning = + `Could not store '${field}' for connection '${name}' securely, so it was not saved: ` + + `the connection works in this session but will fail after a restart. ` + + `Set ALTIMATE_CODE_CONN_${name.toUpperCase()} to the connection's full config as JSON instead.` Log.Default.warn(warning) warnings.push(warning) delete sanitized[field] diff --git a/packages/opencode/src/altimate/native/connections/registry.ts b/packages/opencode/src/altimate/native/connections/registry.ts index 5b091ca95c..3542ec8ea4 100644 --- a/packages/opencode/src/altimate/native/connections/registry.ts +++ b/packages/opencode/src/altimate/native/connections/registry.ts @@ -616,7 +616,7 @@ export async function test( export async function add( name: string, config: ConnectionConfig, -): Promise<{ success: boolean; name: string; type: string; error?: string }> { +): Promise<{ success: boolean; name: string; type: string; error?: string; warnings?: string[] }> { try { ensureLoaded() diff --git a/packages/opencode/src/altimate/native/types.ts b/packages/opencode/src/altimate/native/types.ts index f69f72e712..0e04f0b575 100644 --- a/packages/opencode/src/altimate/native/types.ts +++ b/packages/opencode/src/altimate/native/types.ts @@ -360,6 +360,8 @@ export interface WarehouseAddResult { name: string type: string error?: string + /** Credentials that could not be stored securely, and so were not saved. */ + warnings?: string[] } export interface WarehouseRemoveParams { diff --git a/packages/opencode/src/altimate/tools/warehouse-add.ts b/packages/opencode/src/altimate/tools/warehouse-add.ts index 8636e14936..54adc86657 100644 --- a/packages/opencode/src/altimate/tools/warehouse-add.ts +++ b/packages/opencode/src/altimate/tools/warehouse-add.ts @@ -65,6 +65,14 @@ IMPORTANT: For private key file paths, always use "private_key_path" (not "priva // instead, at the point where it can still be acted on. output += driverReadinessNote(result.type) + // A credential that could not be stored securely was left out of the + // saved connection. It still works now (the session keeps it in + // memory), so without saying so here the first sign is a failed + // connection after the next restart. + if (result.warnings?.length) { + output += `\n\nWarning — not every credential was saved:\n${result.warnings.map((w) => `- ${w}`).join("\n")}` + } + // Run suggestion gathering concurrently with a timeout to avoid // adding noticeable latency to the warehouse add response. try { diff --git a/packages/opencode/test/altimate/credential-store-backend.test.ts b/packages/opencode/test/altimate/credential-store-backend.test.ts new file mode 100644 index 0000000000..8cc251740f --- /dev/null +++ b/packages/opencode/test/altimate/credential-store-backend.test.ts @@ -0,0 +1,85 @@ +/** + * The warehouse credential store falls back to an OS store when keytar is not + * installed (it never is in a released binary). Before that fallback every + * credential was stripped from the saved connection, so a connection worked in + * the session that added it and failed after a restart. + */ +import { afterEach, describe, expect, test } from "bun:test" +import * as CredentialStore from "../../src/altimate/native/connections/credential-store" + +function memoryBackend(opts: { failSet?: boolean } = {}) { + const store = new Map() + return { + store, + backend: { + name: "memory", + async set(account: string, value: string) { + if (opts.failSet) throw new Error("keychain locked") + store.set(account, value) + }, + async get(account: string) { + return store.get(account) ?? null + }, + async delete(account: string) { + return store.delete(account) + }, + }, + } +} + +afterEach(() => CredentialStore.setSecretBackendForTests(undefined)) + +describe("credential store with an OS backend", () => { + test("a saved password leaves the file and comes back on the next load", async () => { + const { store, backend } = memoryBackend() + CredentialStore.setSecretBackendForTests(backend) + const { sanitized, warnings } = await CredentialStore.saveConnection("pg", { + type: "postgres", + host: "h", + user: "u", + password: "s3cret", + } as any) + expect(warnings).toEqual([]) + expect(sanitized.password).toBeUndefined() + expect(store.get("pg/password")).toBe("s3cret") + + // A later process reads the sanitized file and resolves the password back. + const resolved = await CredentialStore.resolveConfig("pg", sanitized) + expect(resolved.password).toBe("s3cret") + }) + + test("a store that refuses the write reports it rather than claiming success", async () => { + const { backend } = memoryBackend({ failSet: true }) + CredentialStore.setSecretBackendForTests(backend) + const { sanitized, warnings } = await CredentialStore.saveConnection("pg", { + type: "postgres", + password: "s3cret", + } as any) + expect(sanitized.password).toBeUndefined() + expect(warnings.length).toBe(1) + expect(warnings[0]).toContain("will fail after a restart") + expect(warnings[0]).toContain("ALTIMATE_CODE_CONN_PG") + }) + + test("a read or delete that throws is treated as absent", async () => { + CredentialStore.setSecretBackendForTests({ + name: "broken", + set: async () => {}, + get: async () => { + throw new Error("no libsecret") + }, + delete: async () => { + throw new Error("no libsecret") + }, + }) + expect(await CredentialStore.getCredential("pg", "password")).toBeNull() + expect(await CredentialStore.deleteCredential("pg", "password")).toBe(false) + }) + + test("with no backend the test preload keeps the real keychain untouched", async () => { + // The preload sets the disable switch; re-probing must respect it. + CredentialStore.setSecretBackendForTests(undefined) + expect(process.env.ALTIMATE_CODE_DISABLE_OS_CREDENTIAL_STORE).toBe("1") + expect(await CredentialStore.storeCredential("pg", "password", "x")).toBe(false) + }) +}) diff --git a/packages/opencode/test/preload.ts b/packages/opencode/test/preload.ts index 14bfffd55d..b19592376b 100644 --- a/packages/opencode/test/preload.ts +++ b/packages/opencode/test/preload.ts @@ -39,6 +39,10 @@ process.env["OPENCODE_MODELS_PATH"] = path.join(import.meta.dir, "tool", "fixtur process.env["OPENCODE_EXPERIMENTAL_EVENT_SYSTEM"] = "true" process.env["OPENCODE_EXPERIMENTAL_WORKSPACES"] = "true" process.env["OPENCODE_DISABLE_PROJECT_COPY_REFRESH"] = "1" +// altimate_change start — the warehouse credential store falls back to Bun.secrets (the real OS +// keychain); tests must never write there. Credential-store tests inject a backend instead. +process.env["ALTIMATE_CODE_DISABLE_OS_CREDENTIAL_STORE"] = "1" +// altimate_change end // Set test home directory to isolate tests from user's actual home directory // This prevents tests from picking up real user configs/skills from ~/.claude/skills From e2920fb8dab1b3877a49e22082eb5660aa87cd5d Mon Sep 17 00:00:00 2001 From: Haider Date: Mon, 5 Oct 2026 04:45:50 +0530 Subject: [PATCH 2/3] fix(warehouse): show Snowflake browser sign-in, bound silent logins, restore the terminal on an unclean exit With `authenticator: externalbrowser`, the Snowflake SDK opens a sign-in page and waits up to two minutes with nothing on screen. A tab that opened behind other windows, or never opened, looked exactly like a hang; after two minutes the attempt failed with "Browser action timed out" and the agent retried into another silent wait. - The driver passes its own `openExternalBrowserCallback`: it announces the sign-in page (with the account and the two-minute limit), opens it, and reports completion or failure. A sign-in nobody completed fails with a message that tells the agent to ask the user rather than retry. - The TUI shows "Waiting for sign-in" for the whole wait, with the link; headless `run` prints one stderr line. IdP text is stripped of control characters, and logs keep the URL's host and path only. - Non-browser logins fail after 120 s with a network/VPN/proxy hint; the SDK's own login retry cannot be set below 300 s. - Every connection attempt is logged (connecting / connected / connect failed with category) and a `warehouse_connect_started` event is sent before connecting, so an attempt that hangs or ends the process still leaves a trace. New categories `sso_not_completed` and `connect_timeout`. - Connection logging used `Log.create`, which writes to stderr only with print-logs on and never reached opencode.log. A small synchronous `fileLog` writes these lines to opencode.log in the same format. - When the process exits without the renderer being torn down, the TUI switches off the terminal modes OpenTUI enables (mouse, focus, paste, colour-scheme reports, kitty keyboard, modifyOtherKeys), so the shell is not left typing mouse reports. A clean exit writes nothing. - Docs: browser single sign-on section. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_018fJ3X7pcGT4R9yzjsJnqsV --- docs/docs/configure/warehouses.md | 20 +++ packages/drivers/src/index.ts | 4 + packages/drivers/src/sign-in.ts | 84 ++++++++++++ packages/drivers/src/snowflake.ts | 97 +++++++++++-- .../drivers/test/snowflake-sign-in.test.ts | 127 ++++++++++++++++++ .../altimate/native/connections/registry.ts | 33 +++++ .../native/connections/sign-in-notice.ts | 99 ++++++++++++++ .../opencode/src/altimate/telemetry/index.ts | 8 ++ .../opencode/src/altimate/util/file-log.ts | 38 ++++++ .../altimate/warehouse-sign-in-notice.test.ts | 96 +++++++++++++ packages/tui/src/app.tsx | 9 ++ packages/tui/src/util/terminal-restore.ts | 51 +++++++ packages/tui/test/terminal-restore.test.ts | 45 +++++++ 13 files changed, 698 insertions(+), 13 deletions(-) create mode 100644 packages/drivers/src/sign-in.ts create mode 100644 packages/drivers/test/snowflake-sign-in.test.ts create mode 100644 packages/opencode/src/altimate/native/connections/sign-in-notice.ts create mode 100644 packages/opencode/src/altimate/util/file-log.ts create mode 100644 packages/opencode/test/altimate/warehouse-sign-in-notice.test.ts create mode 100644 packages/tui/src/util/terminal-restore.ts create mode 100644 packages/tui/test/terminal-restore.test.ts diff --git a/docs/docs/configure/warehouses.md b/docs/docs/configure/warehouses.md index 99b1748033..b754aac7e8 100644 --- a/docs/docs/configure/warehouses.md +++ b/docs/docs/configure/warehouses.md @@ -72,6 +72,26 @@ Each warehouse has a key (the connection name) and a config object: } ``` +### Browser single sign-on + +Set `"authenticator": "externalbrowser"` (and no password) to sign in through your identity provider. + +```json +{ + "snowflake-sso": { + "type": "snowflake", + "account": "xy12345.us-east-1", + "user": "jane.doe@example.com", + "authenticator": "externalbrowser", + "warehouse": "COMPUTE_WH" + } +} +``` + +When a connection opens, Snowflake's sign-in page opens in your default browser, and Altimate Code shows **Waiting for sign-in** with a link to the same page in case no browser tab appeared. Snowflake waits up to two minutes for the sign-in. If it isn't completed in that time, the connection fails with a message saying so; complete the sign-in, then try again. If you'd rather not sign in through the browser, use [key-pair authentication](#key-pair-authentication). + +A connection that does not use the browser fails after two minutes without an answer from Snowflake, with a message pointing at network, VPN or proxy settings, instead of retrying silently. + ## BigQuery ```json diff --git a/packages/drivers/src/index.ts b/packages/drivers/src/index.ts index 2e10ca75d0..0c35bb4d82 100644 --- a/packages/drivers/src/index.ts +++ b/packages/drivers/src/index.ts @@ -4,6 +4,10 @@ export type { Connector, ConnectorResult, SchemaColumn, ConnectionConfig } from // Re-export config normalization export { normalizeConfig, sanitizeConnectionString } from "./normalize" +// Re-export interactive sign-in notices +export { onBrowserSignIn, redactSignInUrl } from "./sign-in" +export type { BrowserSignInNotice } from "./sign-in" + // Re-export file-backed store guards export { allowsCreate, assertStoreExists, isLocalFilePath, requireStorePath } from "./file-store" diff --git a/packages/drivers/src/sign-in.ts b/packages/drivers/src/sign-in.ts new file mode 100644 index 0000000000..c8fa248097 --- /dev/null +++ b/packages/drivers/src/sign-in.ts @@ -0,0 +1,84 @@ +import { spawn } from "child_process" + +/** + * Interactive sign-in a driver is waiting on (today: Snowflake `externalbrowser` + * SSO). The SDK opens the browser and waits silently; without this the user sees + * only a spinner and cannot tell a pending sign-in from a hang. + */ +export interface BrowserSignInNotice { + warehouse: string + /** Identifies which account is asking, e.g. the Snowflake account locator. */ + account?: string + phase: "waiting" | "completed" | "failed" + /** The sign-in page. Only on `waiting`. */ + url?: string + /** How long the driver waits for the sign-in before failing. Only on `waiting`. */ + timeoutMs?: number +} + +type Listener = (notice: BrowserSignInNotice) => void + +// Process-global: the driver and its subscriber can reach this file through +// different module graphs (the bundle and a dynamic driver import), and a +// per-module set would silently drop every notice. +const LISTENERS_KEY = Symbol.for("altimate.drivers.browserSignInListeners") +const listeners: Set = ((globalThis as Record)[LISTENERS_KEY] as Set) ?? + ((globalThis as Record)[LISTENERS_KEY] = new Set()) + +/** Subscribe to sign-in notices from every driver. Returns the unsubscribe. */ +export function onBrowserSignIn(listener: Listener): () => void { + listeners.add(listener) + return () => listeners.delete(listener) +} + +export function emitBrowserSignIn(notice: BrowserSignInNotice): void { + for (const listener of listeners) { + try { + listener(notice) + } catch { + // a broken listener must not fail the connection + } + } +} + +let opener: (url: string) => void = launchBrowser + +/** Opens a URL in the default browser, the way the SDK's own opener does. Failure is silent: the URL is also surfaced to the user. */ +export function openInBrowser(url: string): void { + opener(url) +} + +/** Replaces the browser launcher; tests pass a recorder so nothing opens. Returns the restore. */ +export function setBrowserOpenerForTests(fn: (url: string) => void): () => void { + opener = fn + return () => { + opener = launchBrowser + } +} + +function launchBrowser(url: string): void { + const [command, args] = + process.platform === "win32" + ? // No shell, so `&` in the SAML query needs no escaping (unlike `cmd /c start`). + ["rundll32", ["url.dll,FileProtocolHandler", url]] + : process.platform === "darwin" + ? ["open", [url]] + : ["xdg-open", [url]] + try { + const child = spawn(command, args as string[], { detached: true, stdio: "ignore", windowsHide: true }) + child.on("error", () => {}) + child.unref() + } catch { + // no browser launcher — the notice still carries the URL + } +} + +/** The sign-in URL without its query string, for logs. The query carries the SAML request. */ +export function redactSignInUrl(url: string): string { + try { + const u = new URL(url) + return `${u.protocol}//${u.host}${u.pathname}` + } catch { + return "" + } +} diff --git a/packages/drivers/src/snowflake.ts b/packages/drivers/src/snowflake.ts index 113757742f..dc81404fb7 100644 --- a/packages/drivers/src/snowflake.ts +++ b/packages/drivers/src/snowflake.ts @@ -5,6 +5,7 @@ import * as fs from "fs" import type { ConnectionConfig, Connector, ConnectorResult, ExecuteOptions, SchemaColumn } from "./types" import { loadOptionalDriver } from "./resolve" +import { emitBrowserSignIn, openInBrowser } from "./sign-in" /** * Run `fn` with stdout/stderr writes swallowed for the (synchronous) duration of @@ -73,8 +74,31 @@ export function keepAliveSetting(config: ConnectionConfig): boolean { return true } -/** `sdk` replaces the installed snowflake-sdk in tests. */ -export async function connect(config: ConnectionConfig, sdk?: unknown): Promise { +/** How long `externalbrowser` SSO waits for the browser sign-in; the SDK's own default, made explicit so the notice states it. */ +export const SSO_WAIT_MS = 120_000 +/** + * Upper bound on opening a non-interactive connection. The SDK retries a failed + * login for at least 300 s (its `retryTimeout` floor), so a blocked network or a + * proxy that drops packets shows as minutes of silence; this turns it into an error. + */ +export const CONNECT_TIMEOUT_MS = 120_000 + +/** True for the SDK's "the browser sign-in never came back" failure. */ +export function isBrowserSignInTimeout(err: unknown): boolean { + return /browser action timed out/i.test(String((err as Error)?.message ?? err)) +} + +function describeAccount(account: unknown): string { + return typeof account === "string" && account ? ` for account '${account}'` : "" +} + +/** `sdk` replaces the installed snowflake-sdk in tests; `seams.connectTimeoutMs` shortens the connect limit there. */ +export async function connect( + config: ConnectionConfig, + sdk?: unknown, + seams?: { connectTimeoutMs?: number }, +): Promise { + const connectTimeoutMs = seams?.connectTimeoutMs ?? CONNECT_TIMEOUT_MS let snowflake: any snowflake = sdk ?? (await loadOptionalDriver("snowflake", "snowflake-sdk")) snowflake = snowflake.default || snowflake @@ -91,23 +115,70 @@ export async function connect(config: ConnectionConfig, sdk?: unknown): Promise< let reconnecting: Promise | undefined function openConnection(): Promise { - return new Promise((resolve, reject) => { - const conn = snowflake.createConnection(connectOptions) - if (connectViaBrowser) { + const account = connectOptions?.account + if (connectViaBrowser) { + // Set by the sign-in callback, so "completed"/"failed" is only reported + // for an attempt that actually asked the user to sign in. + let prompted = false + if (connectOptions && connectOptions.authenticator === "EXTERNALBROWSER") { + connectOptions.browserActionTimeout = SSO_WAIT_MS + connectOptions.openExternalBrowserCallback = (url: string) => { + prompted = true + emitBrowserSignIn({ warehouse: "snowflake", account: String(account ?? ""), phase: "waiting", url, timeoutMs: SSO_WAIT_MS }) + openInBrowser(url) + } + } + return new Promise((resolve, reject) => { + const conn = snowflake.createConnection(connectOptions) if (typeof conn.connectAsync !== "function") { reject(new Error("Snowflake browser/SSO auth requires snowflake-sdk with connectAsync support. Upgrade snowflake-sdk.")) return } conn.connectAsync((err: Error | null) => { - if (err) reject(err) - else resolve(conn) + if (prompted) emitBrowserSignIn({ warehouse: "snowflake", account: String(account ?? ""), phase: err ? "failed" : "completed" }) + if (!err) return resolve(conn) + if (isBrowserSignInTimeout(err)) { + return reject( + new Error( + `Snowflake browser sign-in${describeAccount(account)} was not completed within ${SSO_WAIT_MS / 60_000} minutes. ` + + `A sign-in page was opened in the default browser (it may be behind other windows). ` + + `Ask the user to complete the sign-in, or to confirm a browser tab opened, before trying again; ` + + `retrying without that opens another sign-in page and waits again.`, + ), + ) + } + reject(err) }).catch(reject) - } else { - conn.connect((err: Error | null) => { - if (err) reject(err) - else resolve(conn) - }) - } + }) + } + return new Promise((resolve, reject) => { + const conn = snowflake.createConnection(connectOptions) + let settled = false + const timer = setTimeout(() => { + settled = true + reject( + new Error( + `Snowflake did not accept the connection${describeAccount(account)} within ${Math.round(connectTimeoutMs / 1000)} seconds. ` + + `The network may be blocking or proxying the connection (check VPN and proxy settings), or Snowflake is unreachable.`, + ), + ) + // The SDK keeps retrying in the background; close it if it ever gets through. + }, connectTimeoutMs) + ;(timer as { unref?: () => void }).unref?.() + conn.connect((err: Error | null) => { + clearTimeout(timer) + if (settled) { + try { + conn.destroy?.(() => {}) + } catch { + // nothing to release + } + return + } + settled = true + if (err) reject(err) + else resolve(conn) + }) }) } diff --git a/packages/drivers/test/snowflake-sign-in.test.ts b/packages/drivers/test/snowflake-sign-in.test.ts new file mode 100644 index 0000000000..0a682ed1ae --- /dev/null +++ b/packages/drivers/test/snowflake-sign-in.test.ts @@ -0,0 +1,127 @@ +/** + * `externalbrowser` SSO: the SDK opens a sign-in page and waits up to two minutes + * with nothing on screen. The driver now reports the page and the outcome, and a + * sign-in that never came back is an actionable error, not the SDK's raw timeout. + * Non-interactive logins get an upper bound instead of the SDK's 300 s retry floor. + */ +import { afterEach, beforeEach, describe, expect, test } from "bun:test" +import { connect, SSO_WAIT_MS } from "../src/snowflake" +import { onBrowserSignIn, redactSignInUrl, setBrowserOpenerForTests, type BrowserSignInNotice } from "../src/sign-in" + +const SIGN_IN_URL = "https://idp.example.com/sso/saml?SAMLRequest=abc&RelayState=xyz" + +/** snowflake-sdk stand-in: `browser` decides what the SSO sign-in does; `connect` what a plain login does. */ +function fakeSdk(opts: { browser?: "complete" | "timeout" | "no-prompt"; connect?: "ok" | "never" }) { + const created: Array<{ options: any; destroyed: boolean }> = [] + const sdk = { + configure() {}, + createConnection(options: any) { + const entry = { options, destroyed: false } + created.push(entry) + return { + async connectAsync(cb: (err: Error | null) => void) { + if (opts.browser !== "no-prompt") options.openExternalBrowserCallback?.(SIGN_IN_URL) + setTimeout(() => { + if (opts.browser === "timeout") { + cb(new Error("Error while getting SAML token: Browser action timed out after 120000 ms.")) + } else cb(null) + }, 1) + }, + connect(cb: (err: Error | null) => void) { + if (opts.connect === "never") return + setTimeout(() => cb(null), 1) + }, + isUp: () => true, + execute() {}, + destroy(cb: (err: Error | null) => void) { + entry.destroyed = true + cb(null) + }, + } + }, + } + return { sdk, created } +} + +const sso = { type: "snowflake", account: "acme-xy123", user: "u@example.com", authenticator: "externalbrowser" } as any + +let notices: BrowserSignInNotice[] +let opened: string[] +let stopListening: () => void +let restoreOpener: () => void + +beforeEach(() => { + notices = [] + opened = [] + stopListening = onBrowserSignIn((n) => notices.push(n)) + restoreOpener = setBrowserOpenerForTests((url) => opened.push(url)) +}) + +afterEach(() => { + stopListening() + restoreOpener() +}) + +describe("Snowflake browser sign-in", () => { + test("the sign-in page is announced with its time limit, opened, and its completion reported", async () => { + const { sdk, created } = fakeSdk({ browser: "complete" }) + const c = await connect(sso, sdk) + await c.connect() + + expect(created[0].options.browserActionTimeout).toBe(SSO_WAIT_MS) + expect(opened).toEqual([SIGN_IN_URL]) + expect(notices).toEqual([ + { warehouse: "snowflake", account: "acme-xy123", phase: "waiting", url: SIGN_IN_URL, timeoutMs: SSO_WAIT_MS }, + { warehouse: "snowflake", account: "acme-xy123", phase: "completed" }, + ]) + }) + + test("a sign-in that never came back says what to do, instead of the SDK's raw timeout", async () => { + const { sdk } = fakeSdk({ browser: "timeout" }) + const c = await connect(sso, sdk) + const err = await c.connect().then( + () => null, + (e: Error) => e, + ) + expect(err?.message).toContain("Snowflake browser sign-in for account 'acme-xy123' was not completed within 2 minutes") + expect(err?.message).toContain("Ask the user to complete the sign-in") + expect(err?.message).not.toContain("SAML token") + expect(notices.map((n) => n.phase)).toEqual(["waiting", "failed"]) + }) + + test("no completion is reported for an attempt that never asked the user to sign in", async () => { + const { sdk } = fakeSdk({ browser: "no-prompt" }) + const c = await connect(sso, sdk) + await c.connect() + expect(notices).toEqual([]) + expect(opened).toEqual([]) + }) + + test("password logins never announce a browser sign-in", async () => { + const { sdk, created } = fakeSdk({ connect: "ok" }) + const c = await connect({ type: "snowflake", account: "a", user: "u", password: "p" } as any, sdk) + await c.connect() + expect(created[0].options.openExternalBrowserCallback).toBeUndefined() + expect(notices).toEqual([]) + }) + + test("logs keep the sign-in page's host and path but not its SAML query", () => { + expect(redactSignInUrl(SIGN_IN_URL)).toBe("https://idp.example.com/sso/saml") + }) +}) + +describe("Snowflake connect time limit", () => { + test("a login that never answers fails with a network hint instead of hanging", async () => { + const { sdk, created } = fakeSdk({ connect: "never" }) + const c = await connect({ type: "snowflake", account: "acme", user: "u", password: "p" } as any, sdk, { + connectTimeoutMs: 20, + }) + const err = await c.connect().then( + () => null, + (e: Error) => e, + ) + expect(err?.message).toContain("Snowflake did not accept the connection for account 'acme'") + expect(err?.message).toContain("VPN and proxy") + expect(created.length).toBe(1) + }) +}) diff --git a/packages/opencode/src/altimate/native/connections/registry.ts b/packages/opencode/src/altimate/native/connections/registry.ts index 3542ec8ea4..6ee43911ce 100644 --- a/packages/opencode/src/altimate/native/connections/registry.ts +++ b/packages/opencode/src/altimate/native/connections/registry.ts @@ -26,6 +26,8 @@ import { resolveConfig, saveConnection } from "./credential-store" import { startTunnel, extractSshConfig, closeTunnel } from "./ssh-tunnel" import type { WarehouseInfo } from "../types" import { Telemetry } from "../../../telemetry" +import * as SignInNotice from "./sign-in-notice" +import { fileLog } from "@/altimate/util/file-log" /** In-memory config store. */ let configs = new Map() @@ -426,6 +428,12 @@ export function categorizeConnectionError(e: unknown): string { ) return "store_locked" // altimate_change end + // altimate_change start — the driver's own wording for an SSO sign-in nobody + // completed and for a login that never answered; both also contain words the + // generic rules below match ("network", "sign-in"), and mean something else. + if (msg.includes("browser sign-in") && msg.includes("was not completed")) return "sso_not_completed" + if (msg.includes("did not accept the connection")) return "connect_timeout" + // altimate_change end if (msg.includes("password") || msg.includes("authentication") || msg.includes("unauthorized") || msg.includes("jwt")) return "auth_failed" if (msg.includes("timeout") || msg.includes("timed out")) return "timeout" @@ -480,6 +488,22 @@ export async function get(name: string): Promise { } const startTime = Date.now() + const authMethod = detectAuthMethod(config) + // altimate_change start — make connection attempts visible. Recorded BEFORE + // connecting: an attempt that hangs or takes the process down never reaches + // the outcome event below, so without this it leaves no trace at all. + SignInNotice.install() + fileLog("INFO", "warehouse-connect", "connecting", { name, type: config.type, auth: authMethod }) + try { + Telemetry.track({ + type: "warehouse_connect_started", + timestamp: startTime, + session_id: Telemetry.getContext().sessionId, + warehouse_type: config.type, + auth_method: authMethod, + }) + } catch {} + // altimate_change end const promise = (async () => { try { const connector = await createConnector(name, config) @@ -491,6 +515,7 @@ export async function get(name: string): Promise { throw connectErr } connectors.set(name, connector) + fileLog("INFO", "warehouse-connect", "connected", { name, type: config.type, auth: authMethod, duration_ms: Date.now() - startTime }) try { Telemetry.track({ type: "warehouse_connect", @@ -504,6 +529,14 @@ export async function get(name: string): Promise { } catch {} return connector } catch (e) { + fileLog("WARN", "warehouse-connect", "connect failed", { + name, + type: config?.type, + auth: authMethod, + duration_ms: Date.now() - startTime, + category: categorizeConnectionError(e), + error: Telemetry.maskString(String(e)).slice(0, 500), + }) try { Telemetry.track({ type: "warehouse_connect", diff --git a/packages/opencode/src/altimate/native/connections/sign-in-notice.ts b/packages/opencode/src/altimate/native/connections/sign-in-notice.ts new file mode 100644 index 0000000000..a02d1d9699 --- /dev/null +++ b/packages/opencode/src/altimate/native/connections/sign-in-notice.ts @@ -0,0 +1,99 @@ +/** + * Shows a warehouse's interactive sign-in (Snowflake `externalbrowser` SSO) while + * the driver waits on it. Without this the SDK opens a browser tab somewhere and + * waits two minutes behind a spinner; a tab that opened behind other windows, or + * never opened, looks exactly like a hang, and the agent retries into another wait. + */ +import { onBrowserSignIn, redactSignInUrl, type BrowserSignInNotice } from "@altimateai/drivers" +import { AppRuntime } from "@/effect/app-runtime" +import { EventV2Bridge } from "@/event-v2-bridge" +import { TuiEvent } from "@/server/tui-event" +import { fileLog } from "@/altimate/util/file-log" + +const COMPLETED_TOAST_MS = 4_000 + +export interface SignInToast { + title: string + message: string + variant: "info" | "success" | "warning" | "error" + duration: number +} + +/** C0/C1 control characters and line separators: the URL comes from the identity provider. */ +function clean(text: string): string { + return text.replace(/[\u0000-\u001F\u007F-\u009F\u2028\u2029]/g, "") +} + +function label(n: BrowserSignInNotice): string { + const name = n.warehouse.charAt(0).toUpperCase() + n.warehouse.slice(1) + return n.account ? `${name} (account ${clean(n.account)})` : name +} + +/** What to show for a notice; undefined when nothing should be shown (a failure is reported by the tool). */ +export function toastFor(n: BrowserSignInNotice): SignInToast | undefined { + if (n.phase === "waiting") { + const minutes = Math.max(1, Math.round((n.timeoutMs ?? 120_000) / 60_000)) + const link = n.url ? ` If no browser tab opened, open this link: ${clean(n.url)}` : "" + return { + title: "Waiting for sign-in", + message: `Sign in to ${label(n)} in your browser. Waiting up to ${minutes} minute${minutes === 1 ? "" : "s"}.${link}`, + variant: "warning", + duration: n.timeoutMs ?? 120_000, + } + } + if (n.phase === "completed") { + return { title: "Signed in", message: `Signed in to ${label(n)}.`, variant: "success", duration: COMPLETED_TOAST_MS } + } + return undefined +} + +export interface SignInNoticeDeps { + headless: () => boolean + toast: (t: SignInToast) => Promise + printLine: (line: string) => void +} + +const defaultDeps: SignInNoticeDeps = { + headless: () => process.env["ALTIMATE_CODE_HEADLESS"] === "1", + toast: async (t) => { + await AppRuntime.runPromise(EventV2Bridge.Service.use((events) => events.publish(TuiEvent.ToastShow, t))) + }, + printLine: (line) => { + try { + process.stderr.write(line + "\n") + } catch { + // stderr closed — the log still has it + } + }, +} + +export function handle(n: BrowserSignInNotice, deps: SignInNoticeDeps = defaultDeps): void { + fileLog("INFO", "warehouse-sign-in", "browser sign-in", { + warehouse: n.warehouse, + account: n.account, + phase: n.phase, + ...(n.url ? { url: redactSignInUrl(n.url) } : {}), + ...(n.timeoutMs ? { timeout_ms: n.timeoutMs } : {}), + }) + const t = toastFor(n) + if (!t) return + if (deps.headless()) { + // `run` has no screen for a toast; the waiting line is the one that matters. + if (n.phase === "waiting") deps.printLine(`${t.title}: ${t.message}`) + return + } + deps.toast(t).catch((err) => fileLog("WARN", "warehouse-sign-in", "could not show the sign-in notice", { err: String(err) })) +} + +let unsubscribe: (() => void) | undefined + +/** Idempotent; called before every warehouse connect. */ +export function install(deps: SignInNoticeDeps = defaultDeps): void { + if (unsubscribe) return + unsubscribe = onBrowserSignIn((n) => handle(n, deps)) +} + +export function resetForTests(): void { + unsubscribe?.() + unsubscribe = undefined +} diff --git a/packages/opencode/src/altimate/telemetry/index.ts b/packages/opencode/src/altimate/telemetry/index.ts index 52d695e548..daf7fc204d 100644 --- a/packages/opencode/src/altimate/telemetry/index.ts +++ b/packages/opencode/src/altimate/telemetry/index.ts @@ -573,6 +573,14 @@ export namespace Telemetry { budget: number scopes_used: string[] } + | { + // Sent before connecting, so an attempt that hangs or ends the process is still counted. + type: "warehouse_connect_started" + timestamp: number + session_id: string + warehouse_type: string + auth_method: string + } | { type: "warehouse_connect" timestamp: number diff --git a/packages/opencode/src/altimate/util/file-log.ts b/packages/opencode/src/altimate/util/file-log.ts new file mode 100644 index 0000000000..baa843b3f6 --- /dev/null +++ b/packages/opencode/src/altimate/util/file-log.ts @@ -0,0 +1,38 @@ +// altimate_change start — synchronous lines in opencode.log from code outside the Effect runtime +/** + * `opencode.log` is written by the Effect logger, which only code running inside + * the app runtime reaches; `Log.create` in `./log` goes to stderr, and only with + * print-logs on. Warehouse connections and debug tracing run outside both, so + * nothing they did ever reached the file a user sends us. This appends lines in + * the same `key=value` format, with the same per-thread run id. Synchronous on + * purpose: a line written just before a hang or a crash must not sit in a buffer. + */ +import fs from "fs" +import path from "path" +import { Global } from "@opencode-ai/core/global" +import { runID } from "@opencode-ai/core/observability/shared" + +export type FileLogLevel = "DEBUG" | "INFO" | "WARN" | "ERROR" + +function format(value: unknown): string { + const text = typeof value === "string" ? value : JSON.stringify(value) ?? String(value) + return /^[^\s="\\]+$/.test(text) ? text : JSON.stringify(text) +} + +/** One log line; `service` is written first so lines are easy to find. Never throws. */ +export function fileLog(level: FileLogLevel, service: string, message: string, fields: Record = {}): void { + try { + const entries: Array<[string, unknown]> = [ + ["timestamp", new Date().toISOString()], + ["level", level], + ["run", runID], + ["service", service], + ["message", message], + ...Object.entries(fields).filter(([, v]) => v !== undefined), + ] + fs.appendFileSync(path.join(Global.Path.log, "opencode.log"), entries.map(([k, v]) => `${k}=${format(v)}`).join(" ") + "\n") + } catch { + // logging must never break the caller + } +} +// altimate_change end diff --git a/packages/opencode/test/altimate/warehouse-sign-in-notice.test.ts b/packages/opencode/test/altimate/warehouse-sign-in-notice.test.ts new file mode 100644 index 0000000000..b64aeb876c --- /dev/null +++ b/packages/opencode/test/altimate/warehouse-sign-in-notice.test.ts @@ -0,0 +1,96 @@ +// @ts-nocheck +/** + * A warehouse waiting on a browser sign-in is shown to the user (a TUI toast for + * the whole wait, or one stderr line under `run`), and every connection attempt + * is recorded before it starts, so one that hangs or ends the process still + * leaves a trace. + */ +import { afterAll, beforeEach, describe, expect, spyOn, test } from "bun:test" +import { Telemetry } from "../../src/altimate/telemetry" +import * as Registry from "../../src/altimate/native/connections/registry" +import * as SignInNotice from "../../src/altimate/native/connections/sign-in-notice" + +const URL_WITH_QUERY = "https://idp.example.com/sso/saml?SAMLRequest=abc" +const waiting = { warehouse: "snowflake", account: "acme-xy123", phase: "waiting", url: URL_WITH_QUERY, timeoutMs: 120_000 } + +function deps(headless: boolean) { + const toasts: any[] = [] + const lines: string[] = [] + return { + toasts, + lines, + deps: { headless: () => headless, toast: async (t) => void toasts.push(t), printLine: (l) => lines.push(l) }, + } +} + +describe("browser sign-in notice", () => { + test("the TUI shows the sign-in for the whole wait, with the account and the link", () => { + const d = deps(false) + SignInNotice.handle(waiting, d.deps) + expect(d.toasts).toHaveLength(1) + const t = d.toasts[0] + expect(t.duration).toBe(120_000) + expect(t.variant).toBe("warning") + expect(t.message).toContain("Sign in to Snowflake (account acme-xy123) in your browser") + expect(t.message).toContain("Waiting up to 2 minutes") + expect(t.message).toContain(URL_WITH_QUERY) + expect(d.lines).toEqual([]) + }) + + test("headless `run` prints the waiting line to stderr instead of a toast", () => { + const d = deps(true) + SignInNotice.handle(waiting, d.deps) + SignInNotice.handle({ warehouse: "snowflake", account: "acme-xy123", phase: "completed" }, d.deps) + expect(d.toasts).toEqual([]) + expect(d.lines).toHaveLength(1) + expect(d.lines[0]).toStartWith("Waiting for sign-in: Sign in to Snowflake") + }) + + test("a completed sign-in is confirmed briefly; a failed one is left to the tool's error", () => { + const d = deps(false) + SignInNotice.handle({ warehouse: "snowflake", account: "a", phase: "completed" }, d.deps) + SignInNotice.handle({ warehouse: "snowflake", account: "a", phase: "failed" }, d.deps) + expect(d.toasts.map((t) => t.variant)).toEqual(["success"]) + }) + + test("control characters from the identity provider never reach the screen", () => { + const t = SignInNotice.toastFor({ ...waiting, url: "https://idp.example.com/x\u001b[2J\u2028y", account: "ac\u0007me" }) + expect(t.message).not.toMatch(/[\u0000-\u001F\u2028]/) + expect(t.message).toContain("account acme") + }) +}) + +describe("connection attempts are recorded before connecting", () => { + const tracked: any[] = [] + const trackSpy = spyOn(Telemetry, "track").mockImplementation((e: any) => void tracked.push(e)) + const ctxSpy = spyOn(Telemetry, "getContext").mockImplementation(() => ({ sessionId: "s1", projectId: "p1" })) + afterAll(() => { + trackSpy.mockRestore() + ctxSpy.mockRestore() + }) + beforeEach(() => { + Registry.reset() + tracked.length = 0 + }) + + test("a start event precedes the outcome, with the same auth method", async () => { + Registry.setConfigs({ sf: { type: "unsupported_db_type", authenticator: "externalbrowser" } }) + await Registry.get("sf").catch(() => {}) + const types = tracked.map((e) => e.type).filter((t) => t.startsWith("warehouse_connect")) + expect(types).toEqual(["warehouse_connect_started", "warehouse_connect"]) + expect(tracked[0].auth_method).toBe(tracked[1].auth_method) + }) + + test("the driver's SSO and connect-limit errors get their own categories", () => { + expect( + Registry.categorizeConnectionError( + new Error("Snowflake browser sign-in for account 'a' was not completed within 2 minutes. A sign-in page was opened"), + ), + ).toBe("sso_not_completed") + expect( + Registry.categorizeConnectionError( + new Error("Snowflake did not accept the connection within 120 seconds. The network may be blocking"), + ), + ).toBe("connect_timeout") + }) +}) diff --git a/packages/tui/src/app.tsx b/packages/tui/src/app.tsx index 865ec432d1..f825f5f60d 100644 --- a/packages/tui/src/app.tsx +++ b/packages/tui/src/app.tsx @@ -111,6 +111,9 @@ import { createTuiAttention } from "./attention" import * as TuiAudio from "./audio" import { win32DisableProcessedInput, win32FlushInputBuffer } from "./terminal-win32" import { destroyRenderer } from "./util/renderer" +// altimate_change start — terminal restore on an unclean exit +import { restoreTerminalOnUncleanExit } from "./util/terminal-restore" +// altimate_change end import { cliErrorMessage, errorFormat } from "./util/error" // altimate_change start — fix: pure helper extracted to terminal-detection for test coverage (#704) import { detectModeFromCOLORFGBG } from "./terminal-detection" @@ -249,6 +252,12 @@ export const run = Effect.fn("Tui.run")(function* (input: TuiInput) { }), ) win32DisableProcessedInput() + // altimate_change start — reset terminal modes if the process exits without the teardown below + yield* Effect.acquireRelease( + Effect.sync(() => restoreTerminalOnUncleanExit(renderer)), + (unregister) => Effect.sync(unregister), + ) + // altimate_change end const keymap = createDefaultOpenTuiKeymap(renderer) yield* Effect.acquireRelease( Effect.sync(() => registerOpencodeKeymap(keymap, renderer, input.config)), diff --git a/packages/tui/src/util/terminal-restore.ts b/packages/tui/src/util/terminal-restore.ts new file mode 100644 index 0000000000..5caa49f890 --- /dev/null +++ b/packages/tui/src/util/terminal-restore.ts @@ -0,0 +1,51 @@ +// altimate_change start — restore the terminal when the process exits without tearing the renderer down +/** + * OpenTUI turns on mouse tracking, focus/paste reporting and extended keyboard + * modes, and only switches them off when the renderer is destroyed. A process + * that exits without that (an exit from deep in a tool, an uncaught error path + * the renderer does not catch) leaves the terminal reporting every mouse move and + * key press as text — `[555;96;44M…` typed into the shell prompt, Esc included. + * + * Only on an unclean exit: after a clean teardown the modes are already off, and + * popping the keyboard-mode stack again could remove the shell's own setting. + * Nothing here can run after a hard kill or a native crash. + */ +import { writeSync } from "fs" + +/** Undoes exactly the modes OpenTUI 0.3.x enables (mouse 1000/1002/1003/1006, focus 1004, + * bracketed paste 2004, colour-scheme reports 2031, kitty keyboard push, modifyOtherKeys), + * shows the cursor and leaves the alternate screen. */ +export const TERMINAL_RESET = + "\x1b[?1000l\x1b[?1002l\x1b[?1003l\x1b[?1006l\x1b[?1004l\x1b[?2004l\x1b[?2031l" + + "\x1b[4;0m\x1b[?25h\x1b[?1049l" + +export interface TerminalRestoreDeps { + onExit: (fn: () => void) => () => void + write: (text: string) => void + isTTY: () => boolean +} + +const defaultDeps: TerminalRestoreDeps = { + onExit: (fn) => { + process.once("exit", fn) + return () => process.off("exit", fn) + }, + write: (text) => writeSync(1, text), + isTTY: () => Boolean(process.stdout.isTTY), +} + +/** Registers the exit guard for `renderer`; returns the unregister. */ +export function restoreTerminalOnUncleanExit( + renderer: { readonly isDestroyed: boolean }, + deps: TerminalRestoreDeps = defaultDeps, +): () => void { + return deps.onExit(() => { + if (renderer.isDestroyed || !deps.isTTY()) return + try { + deps.write(TERMINAL_RESET) + } catch { + // stdout already gone — nothing left to restore + } + }) +} +// altimate_change end diff --git a/packages/tui/test/terminal-restore.test.ts b/packages/tui/test/terminal-restore.test.ts new file mode 100644 index 0000000000..8ce91560b3 --- /dev/null +++ b/packages/tui/test/terminal-restore.test.ts @@ -0,0 +1,45 @@ +// altimate_change start — terminal restore on an unclean exit +import { describe, expect, test } from "bun:test" +import { restoreTerminalOnUncleanExit, TERMINAL_RESET } from "../src/util/terminal-restore" + +function harness(opts: { destroyed: boolean; tty?: boolean }) { + const renderer = { isDestroyed: opts.destroyed } + let exitHandler: (() => void) | undefined + const written: string[] = [] + const unregister = restoreTerminalOnUncleanExit(renderer, { + onExit: (fn) => { + exitHandler = fn + return () => { + exitHandler = undefined + } + }, + write: (t) => void written.push(t), + isTTY: () => opts.tty ?? true, + }) + return { renderer, written, exit: () => exitHandler?.(), unregister } +} + +describe("terminal restore on exit", () => { + test("an exit that skipped the renderer teardown switches every reporting mode off", () => { + const h = harness({ destroyed: false }) + h.exit() + expect(h.written).toEqual([TERMINAL_RESET]) + for (const mode of ["?1000l", "?1002l", "?1003l", "?1006l", "?1004l", "?2004l", "?2031l", "4;0m"]) { + expect(TERMINAL_RESET).toContain("\x1b[" + mode) + } + }) + + test("after a clean teardown nothing is written, so the shell's own keyboard mode is left alone", () => { + const h = harness({ destroyed: false }) + h.renderer.isDestroyed = true + h.exit() + expect(h.written).toEqual([]) + }) + + test("nothing is written when stdout is not a terminal", () => { + const h = harness({ destroyed: false, tty: false }) + h.exit() + expect(h.written).toEqual([]) + }) +}) +// altimate_change end From b1b812770fe14d44779d6e989240dacd47fc1a20 Mon Sep 17 00:00:00 2001 From: Haider Date: Tue, 6 Oct 2026 01:45:38 +0530 Subject: [PATCH 3/3] fix(warehouse): never resend a write after a closed session, and clean up saved secrets (review) Consensus review fixes. Snowflake reconnect: - A closed-session error (407002) can arrive while the SDK polls the result of a statement Snowflake already accepted, so only a single read-only statement (`isRetrySafe`) is sent again. A write gets an error saying it may already have run. - A late error from a connection that was already replaced no longer reconnects again and tears down its replacement. - `close()` during a reconnect destroys the connection that reconnect opens, instead of leaving a live session behind. - After `USE`, `ALTER SESSION`, `SET` or a temporary table, a replaced session is reported once instead of silently running later statements against the default schema. - The connect time limit now stops the SDK's own login retries. Credentials: - `warehouse_remove` deletes the connection's secrets from the OS credential store, unless the project config or an env var still defines a connection by that name. - Saving a connection again under the same name drops secrets the new config no longer has (an old private key outranked a new browser sign-in), but only once the new secrets were stored. - `Bun.secrets` adapter tested against its call shape. Also: - The browser sign-in toast is published with the workspace the connect started in, so a TUI showing a workspace no longer drops it. - The terminal is also restored on SIGTERM. - Docs: retry behaviour and credential removal. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_018fJ3X7pcGT4R9yzjsJnqsV --- docs/docs/configure/warehouses.md | 4 +- packages/drivers/src/snowflake.ts | 113 +++++++++++++++-- .../drivers/test/snowflake-reconnect.test.ts | 116 ++++++++++++++++-- .../drivers/test/snowflake-sign-in.test.ts | 2 + .../native/connections/credential-store.ts | 21 +++- .../altimate/native/connections/registry.ts | 9 +- .../native/connections/sign-in-notice.ts | 43 ++++++- .../altimate/credential-store-backend.test.ts | 77 ++++++++++++ .../altimate/warehouse-sign-in-notice.test.ts | 13 ++ packages/tui/src/util/terminal-restore.ts | 22 +++- packages/tui/test/terminal-restore.test.ts | 30 ++++- 11 files changed, 418 insertions(+), 32 deletions(-) diff --git a/docs/docs/configure/warehouses.md b/docs/docs/configure/warehouses.md index b754aac7e8..6f00f056e3 100644 --- a/docs/docs/configure/warehouses.md +++ b/docs/docs/configure/warehouses.md @@ -24,7 +24,7 @@ Each warehouse has a key (the connection name) and a config object: Use `{env:...}` substitution for passwords and tokens so you never commit secrets to version control. !!! note "Where saved passwords go" - When the agent adds a connection for you, passwords, keys and tokens are kept in your OS credential store (macOS Keychain, Windows Credential Manager, or libsecret on Linux), never in `connections.json`. If no credential store is available, the credential is not saved: the connection works for that session only, and Altimate Code says so. Use an `ALTIMATE_CODE_CONN_` environment variable instead in that case. + When the agent adds a connection for you, passwords, keys and tokens are kept in your OS credential store (macOS Keychain, Windows Credential Manager, or libsecret on Linux), never in `connections.json`. If no credential store is available, the credential is not saved: the connection works for that session only, and Altimate Code says so. Use an `ALTIMATE_CODE_CONN_` environment variable instead in that case. Removing a connection removes its credentials from the store, and saving a connection again under the same name drops credentials it no longer uses. ## Snowflake @@ -53,7 +53,7 @@ Each warehouse has a key (the connection name) and a config object: | `database` | No | Database name | | `schema` | No | Schema name | | `role` | No | User role | -| `client_session_keep_alive` | No | Keep the session alive while Altimate Code is running (default `true`). A connection Snowflake closes anyway is reopened automatically. | +| `client_session_keep_alive` | No | Keep the session alive while Altimate Code is running (default `true`). A connection Snowflake closes anyway is reopened automatically. A read-only query interrupted by the close is run again; a statement that changes data is not, because it may already have run, and the error says so. After a reopen, `USE` and `ALTER SESSION` settings must be run again. | ### Key-pair authentication diff --git a/packages/drivers/src/snowflake.ts b/packages/drivers/src/snowflake.ts index dc81404fb7..12b386cf0d 100644 --- a/packages/drivers/src/snowflake.ts +++ b/packages/drivers/src/snowflake.ts @@ -53,11 +53,12 @@ export function suppressSnowflakeLogging(snowflake: any): void { } /** - * Errors meaning the statement never reached Snowflake because the session was - * already gone, so reconnecting and sending it again cannot run it twice: - * 407002 is raised client-side for a terminated connection; 390111 (session no - * longer exists), 390112 (session expired) and 390114 (session token expired) - * are the server refusing the request before executing anything. + * Errors meaning the session is gone: 407002 is raised client-side for any + * request on a terminated connection; 390111 (session no longer exists), + * 390112 (session expired) and 390114 (session token expired) come from the + * server. They do NOT prove the statement never ran: 407002 is also what the + * SDK returns when it polls for the result of a statement Snowflake already + * accepted. Whether a statement may be sent again is `isRetrySafe`'s call. */ const CLOSED_SESSION_CODES = new Set(["407002", "390111", "390112", "390114"]) @@ -67,6 +68,44 @@ export function isClosedConnectionError(err: unknown): boolean { return /unable to perform operation using terminated connection/i.test(String((err as Error)?.message ?? err)) } +/** Leading `--` / `//` line comments and block comments, which can precede a statement's first keyword. */ +function stripLeadingComments(sql: string): string { + let s = sql + for (;;) { + const next = s.replace(/^\s+/, "").replace(/^(--|\/\/)[^\n]*(\n|$)/, "").replace(/^\/\*[\s\S]*?\*\//, "") + if (next === s) return s + s = next + } +} + +/** Keywords that change data, objects or the session, anywhere in a statement. */ +const WRITE_KEYWORDS = + /\b(INSERT|UPDATE|DELETE|MERGE|CREATE|DROP|ALTER|TRUNCATE|COPY|CALL|PUT|GET|REMOVE|GRANT|REVOKE|UNDROP|EXECUTE|BEGIN|COMMIT|ROLLBACK|SET|UNSET|USE)\b/i + +/** + * True when sending `sql` a second time cannot change anything: a single + * read-only statement. A closed-session error can arrive after Snowflake already + * accepted the statement (the SDK's result poll fails with 407002 once the + * session drops, and the first execution keeps running server-side), so a + * write is never resent. Errs toward false: a read the rule cannot recognise is + * reported to the caller rather than retried. + */ +export function isRetrySafe(sql: string): boolean { + const s = stripLeadingComments(sql).replace(/;\s*$/, "") + if (s.includes(";")) return false // more than one statement + if (/^(SHOW|DESCRIBE|DESC|EXPLAIN|LIST|LS)\b/i.test(s)) return true + if (/^(SELECT|WITH|VALUES)\b/i.test(s)) return !WRITE_KEYWORDS.test(s.replace(/'(?:[^']|'')*'/g, "''")) + return false +} + +/** Statements whose effect lives in the session (current database/schema/role, session parameters, variables, + * temporary objects, an open transaction) and is lost when the session is replaced. */ +export function changesSession(sql: string): boolean { + const s = stripLeadingComments(sql) + return /^(USE|SET|UNSET|BEGIN|START\s+TRANSACTION|ALTER\s+SESSION)\b/i.test(s) || + /^CREATE\s+(OR\s+REPLACE\s+)?(LOCAL\s+|GLOBAL\s+)?(TEMP|TEMPORARY|VOLATILE)\b/i.test(s) +} + /** `client_session_keep_alive` / `clientSessionKeepAlive`; on unless explicitly false. */ export function keepAliveSetting(config: ConnectionConfig): boolean { const value = config.client_session_keep_alive ?? config.clientSessionKeepAlive @@ -113,6 +152,10 @@ export async function connect( let connectOptions: Record | undefined let connectViaBrowser = false let reconnecting: Promise | undefined + /** Set by close(): a reconnect that finishes afterwards must not install a session nobody will close. */ + let closed = false + /** A statement changed session state, which a reconnect silently drops; such a session's statements are not resent. */ + let sessionChanged = false function openConnection(): Promise { const account = connectOptions?.account @@ -162,7 +205,13 @@ export async function connect( `The network may be blocking or proxying the connection (check VPN and proxy settings), or Snowflake is unreachable.`, ), ) - // The SDK keeps retrying in the background; close it if it ever gets through. + // Stop the SDK's own login retries (they run for at least 300 s); the late + // callback below also destroys the connection if it gets through anyway. + try { + conn.destroy?.(() => {}) + } catch { + // not started far enough to release + } }, connectTimeoutMs) ;(timer as { unref?: () => void }).unref?.() conn.connect((err: Error | null) => { @@ -188,6 +237,14 @@ export async function connect( const previous = connection reconnecting = openConnection() .then((conn) => { + if (closed) { + try { + conn.destroy?.(() => {}) + } catch { + // nothing to release + } + throw new Error("Snowflake connection was closed while reconnecting") + } connection = conn suppressSnowflakeLogging(snowflake) try { @@ -203,11 +260,22 @@ export async function connect( return reconnecting } + /** The error for a statement held back because the replaced session had state it depended on. Reported once: + * the reopened session starts clean, so later statements run as usual. */ + function sessionLostError(cause: string): Error { + sessionChanged = false + return new Error( + `Snowflake closed the session (${cause}), and its USE, ALTER SESSION, variables or temporary objects went with it, ` + + `so this statement was not run on the new session. The connection has been reopened; run those statements again first.`, + ) + } + /** Reopen before use when the SDK already knows the connection is gone (idle timeout, network drop, sleep). */ async function ensureLive(): Promise { if (reconnecting) return reconnecting if (connection && connectOptions && typeof connection.isUp === "function" && !connection.isUp()) { await reconnect() + if (sessionChanged) throw sessionLostError("it had expired") } } @@ -215,19 +283,37 @@ export async function connect( return value.replace(/"/g, '""') } - /** Run a statement, reopening the connection and retrying once if it was already closed when the statement was sent. */ + /** + * Run a statement. When Snowflake has closed the session, the connection is + * reopened and a read-only statement is retried once; a write is not, because + * it may already have run (see `isRetrySafe`). + */ async function executeQuery(sql: string, binds?: any[]): Promise<{ columns: string[]; rows: any[][] }> { await ensureLive() + const used = connection try { - return await runQuery(sql, binds) + const result = await runQuery(used, sql, binds) + if (changesSession(sql)) sessionChanged = true + return result } catch (err) { if (!connectOptions || !isClosedConnectionError(err)) throw err - await reconnect() - return runQuery(sql, binds) + // Reopen only if the failed connection is still the current one: a late error from a connection + // another statement already replaced must not tear down its replacement. + if (connection === used) await reconnect() + else if (reconnecting) await reconnecting + const cause = String((err as Error)?.message ?? err) + if (sessionChanged) throw sessionLostError(cause) + if (!isRetrySafe(sql)) { + throw new Error( + `Snowflake closed the session while this statement was running, so it was not run again: it may change data ` + + `and could already have run. The connection has been reopened; check the result before running it again. (${cause})`, + ) + } + return runQuery(connection, sql, binds) } } - function runQuery(sql: string, binds?: any[]): Promise<{ columns: string[]; rows: any[][] }> { + function runQuery(conn: any, sql: string, binds?: any[]): Promise<{ columns: string[]; rows: any[][] }> { return new Promise((resolve, reject) => { const options: Record = { sqlText: sql, @@ -245,12 +331,13 @@ export async function connect( }, } if (binds && binds.length > 0) options.binds = binds - connection.execute(options) + conn.execute(options) }) } return { async connect() { + closed = false const options: Record = { account: config.account, username: config.user ?? config.username, @@ -498,6 +585,8 @@ export async function connect( }, async close() { + closed = true + await reconnecting?.catch(() => {}) if (connection) { await new Promise((resolve) => { connection.destroy((err: Error | null) => { diff --git a/packages/drivers/test/snowflake-reconnect.test.ts b/packages/drivers/test/snowflake-reconnect.test.ts index ca4b2622a8..c10eae117f 100644 --- a/packages/drivers/test/snowflake-reconnect.test.ts +++ b/packages/drivers/test/snowflake-reconnect.test.ts @@ -4,29 +4,37 @@ * with "Unable to perform operation using terminated connection" until restart. */ import { describe, test, expect } from "bun:test" -import { connect, isClosedConnectionError, keepAliveSetting } from "../src/snowflake" +import { changesSession, connect, isClosedConnectionError, isRetrySafe, keepAliveSetting } from "../src/snowflake" -type Behaviour = { up: boolean; failNextWith?: { code: unknown; message: string } } +type Failure = { code: unknown; message: string; delayMs?: number } +type Behaviour = { up: boolean; failNextWith?: Failure; failQueue?: Failure[] } -/** A minimal stand-in for snowflake-sdk: records connections and lets a test close them. */ +/** A minimal stand-in for snowflake-sdk: records connections and lets a test close them. `gate`, when set, holds + * every new connect() until the test calls it. */ function fakeSdk() { const created: Array<{ options: any; state: Behaviour; destroyed: boolean; executed: string[] }> = [] + const held: Array<() => void> = [] const sdk = { + hold: false, + release() { + for (const r of held.splice(0)) r() + }, configure() {}, createConnection(options: any) { const entry = { options, state: { up: true } as Behaviour, destroyed: false, executed: [] as string[] } created.push(entry) return { connect(cb: (err: Error | null) => void) { - setTimeout(() => cb(null), 1) + if (sdk.hold) held.push(() => cb(null)) + else setTimeout(() => cb(null), 1) }, isUp: () => entry.state.up, execute(opts: { sqlText: string; complete: (err: any, stmt: any, rows: any[]) => void }) { - if (entry.state.failNextWith) { - const f = entry.state.failNextWith + const f = entry.state.failNextWith ?? entry.state.failQueue?.shift() + if (f) { entry.state.failNextWith = undefined const err = Object.assign(new Error(f.message), { code: f.code }) - setTimeout(() => opts.complete(err, null, []), 1) + setTimeout(() => opts.complete(err, null, []), f.delayMs ?? 1) return } entry.executed.push(opts.sqlText) @@ -82,10 +90,70 @@ describe("Snowflake connection lifecycle", () => { const result = await c.execute("SELECT 1") expect(created.length).toBe(2) + expect(created[0].executed).toEqual([]) expect(created[1].executed).toEqual(["SELECT 1 LIMIT 1001"]) expect(result.rows).toEqual([[1]]) }) + test("a write whose session closed mid-statement is not sent again: it may already have run", async () => { + // 407002 is also what the SDK returns when it polls for the result of a statement Snowflake already accepted. + const { sdk, created } = fakeSdk() + const c = await connect(config, sdk) + await c.connect() + created[0].state.failNextWith = { code: 407002, message: "Unable to perform operation using terminated connection." } + await expect(c.execute("MERGE INTO t USING s ON t.id = s.id WHEN MATCHED THEN DELETE")).rejects.toThrow( + "it may change data and could already have run", + ) + expect(created.length).toBe(2) // reopened for the next statement + expect(created[1].executed).toEqual([]) + const after = await c.execute("SELECT 1") + expect(after.rows).toEqual([[1]]) + }) + + test("a late error from a connection already replaced does not replace the new one", async () => { + const { sdk, created } = fakeSdk() + const c = await connect(config, sdk) + await c.connect() + // Two statements in flight on the first connection; one fails at once, the other only after the reconnect. + created[0].state.failQueue = [ + { code: 407002, message: "terminated connection", delayMs: 1 }, + { code: 407002, message: "terminated connection", delayMs: 40 }, + ] + const [a, b] = await Promise.all([c.execute("SELECT 1"), c.execute("SELECT 2")]) + expect(created.length).toBe(2) + expect(created[1].destroyed).toBe(false) + expect(created[1].executed).toEqual(["SELECT 1 LIMIT 1001", "SELECT 2 LIMIT 1001"]) + expect(a.rows).toEqual([[1]]) + expect(b.rows).toEqual([[1]]) + }) + + test("close() while a reconnect is pending leaves no live session behind", async () => { + const { sdk, created } = fakeSdk() + const c = await connect(config, sdk) + await c.connect() + created[0].state.up = false + sdk.hold = true + const running = c.execute("SELECT 1") + await new Promise((r) => setTimeout(r, 5)) + const closing = c.close() + sdk.release() + await closing + await expect(running).rejects.toThrow("closed while reconnecting") + expect(created.length).toBe(2) + expect(created[1].destroyed).toBe(true) + }) + + test("a session that ran USE is not silently replaced: the next statement says so, then runs normally", async () => { + const { sdk, created } = fakeSdk() + const c = await connect(config, sdk) + await c.connect() + await c.execute("USE SCHEMA analytics") + created[0].state.failNextWith = { code: 407002, message: "terminated connection" } + await expect(c.execute("SELECT * FROM orders")).rejects.toThrow("run those statements again first") + expect(created[1].executed).toEqual([]) + expect((await c.execute("SELECT 1")).rows).toEqual([[1]]) + }) + test("an expired session token (390114) is also retried", async () => { const { sdk, created } = fakeSdk() const c = await connect(config, sdk) @@ -115,6 +183,40 @@ describe("Snowflake connection lifecycle", () => { }) }) +describe("isRetrySafe", () => { + test("only a single read-only statement may be sent twice", () => { + for (const ok of [ + "SELECT 1", + " select * from t limit 5", + "-- why\nSELECT 1", + "/* note */ WITH a AS (SELECT 1) SELECT * FROM a", + "SHOW SCHEMAS", + "DESCRIBE TABLE t", + "SELECT 'insert into x' AS label", + "SELECT 1;", + ]) + expect(isRetrySafe(ok)).toBe(true) + for (const no of [ + "INSERT INTO t VALUES (1)", + "MERGE INTO t USING s ON 1=1 WHEN MATCHED THEN DELETE", + "CREATE TABLE t AS SELECT 1", + "WITH a AS (SELECT 1) INSERT INTO t SELECT * FROM a", + "SELECT 1; DELETE FROM t", + "CALL refresh()", + "COPY INTO t FROM @s", + "USE SCHEMA x", + "", + ]) + expect(isRetrySafe(no)).toBe(false) + }) + + test("session-changing statements are recognised", () => { + for (const yes of ["USE SCHEMA x", "use warehouse w", "ALTER SESSION SET TIMEZONE = 'UTC'", "SET v = 1", "CREATE TEMP TABLE t (a int)", "create or replace temporary table t as select 1", "BEGIN"]) + expect(changesSession(yes)).toBe(true) + for (const no of ["SELECT 1", "CREATE TABLE t (a int)", "ALTER TABLE t ADD c int"]) expect(changesSession(no)).toBe(false) + }) +}) + describe("isClosedConnectionError", () => { test("matches only errors raised before the statement reached Snowflake", () => { expect(isClosedConnectionError({ code: 407002 })).toBe(true) diff --git a/packages/drivers/test/snowflake-sign-in.test.ts b/packages/drivers/test/snowflake-sign-in.test.ts index 0a682ed1ae..f4c6617722 100644 --- a/packages/drivers/test/snowflake-sign-in.test.ts +++ b/packages/drivers/test/snowflake-sign-in.test.ts @@ -123,5 +123,7 @@ describe("Snowflake connect time limit", () => { expect(err?.message).toContain("Snowflake did not accept the connection for account 'acme'") expect(err?.message).toContain("VPN and proxy") expect(created.length).toBe(1) + // The SDK's own login retries are stopped rather than left running in the background. + expect(created[0].destroyed).toBe(true) }) }) diff --git a/packages/opencode/src/altimate/native/connections/credential-store.ts b/packages/opencode/src/altimate/native/connections/credential-store.ts index feeaec5847..d3879f517c 100644 --- a/packages/opencode/src/altimate/native/connections/credential-store.ts +++ b/packages/opencode/src/altimate/native/connections/credential-store.ts @@ -75,8 +75,8 @@ async function loadKeytar(): Promise { } } -function loadBunSecrets(): SecretBackend | null { - const secrets = (globalThis as { Bun?: { secrets?: any } }).Bun?.secrets +/** `secrets` is `Bun.secrets` unless a test passes a stand-in with the same call shape. */ +export function loadBunSecrets(secrets: any = (globalThis as { Bun?: { secrets?: any } }).Bun?.secrets): SecretBackend | null { if (typeof secrets?.set !== "function" || typeof secrets?.get !== "function") return null return { name: "Bun.secrets", @@ -155,6 +155,20 @@ export async function deleteCredential( } } +/** + * Delete every secret stored for connection `name`, except the fields `keep` holds. Entries are keyed by + * connection name only, so without this a removed connection's secrets stay in the OS store, and a connection + * re-added under the same name gets them back at the next restart (`resolveConfig` fills absent fields from the + * store): an old private key would then outrank the new sign-in method. + */ +export async function forgetCredentials(name: string, keep?: ConnectionConfig): Promise { + for (const field of SENSITIVE_FIELDS) { + const kept = keep?.[field] + if (typeof kept === "string" && kept) continue + await deleteCredential(name, field) + } +} + /** * Resolve a connection config by pulling sensitive fields from the OS credential store. * If no store is available, returns the config as-is (credentials stay in JSON). @@ -202,6 +216,9 @@ export async function saveConnection( delete sanitized[field] } } + // Secrets a previous config under this name stored, which this one no longer has. Only once every new secret + // was stored: a failed write must not also discard the credentials that still work. + if (warnings.length === 0) await forgetCredentials(name, config) return { sanitized, warnings } } diff --git a/packages/opencode/src/altimate/native/connections/registry.ts b/packages/opencode/src/altimate/native/connections/registry.ts index 6ee43911ce..72b631fb7e 100644 --- a/packages/opencode/src/altimate/native/connections/registry.ts +++ b/packages/opencode/src/altimate/native/connections/registry.ts @@ -22,7 +22,7 @@ import { Instance } from "@/project/instance" import { Log } from "@/altimate/util/log" import type { ConnectionConfig, Connector } from "@altimateai/drivers" import { isLocalFilePath, normalizeConfig } from "@altimateai/drivers" -import { resolveConfig, saveConnection } from "./credential-store" +import { forgetCredentials, resolveConfig, saveConnection } from "./credential-store" import { startTunnel, extractSshConfig, closeTunnel } from "./ssh-tunnel" import type { WarehouseInfo } from "../types" import { Telemetry } from "../../../telemetry" @@ -493,6 +493,7 @@ export async function get(name: string): Promise { // connecting: an attempt that hangs or takes the process down never reaches // the outcome event below, so without this it leaves no trace at all. SignInNotice.install() + SignInNotice.rememberOrigin() fileLog("INFO", "warehouse-connect", "connecting", { name, type: config.type, auth: authMethod }) try { Telemetry.track({ @@ -733,6 +734,12 @@ export async function remove(name: string): Promise<{ success: boolean; error?: // Remove from in-memory configs.delete(name) + // altimate_change start — remove its secrets from the OS credential store too. Entries are keyed by name + // only, so they are kept while this project's config or an env var still defines a connection by that name. + const stillDefined = name in loadFromFile(localConfigPath()) || name in loadFromEnv() + if (!stillDefined) await forgetCredentials(name) + // altimate_change end + return { success: true } } catch (e) { return { success: false, error: String(e) } diff --git a/packages/opencode/src/altimate/native/connections/sign-in-notice.ts b/packages/opencode/src/altimate/native/connections/sign-in-notice.ts index a02d1d9699..ccf416fc5c 100644 --- a/packages/opencode/src/altimate/native/connections/sign-in-notice.ts +++ b/packages/opencode/src/altimate/native/connections/sign-in-notice.ts @@ -5,7 +5,13 @@ * never opened, looks exactly like a hang, and the agent retries into another wait. */ import { onBrowserSignIn, redactSignInUrl, type BrowserSignInNotice } from "@altimateai/drivers" +import { Context, Fiber } from "effect" import { AppRuntime } from "@/effect/app-runtime" +import { InstanceRef, WorkspaceRef } from "@/effect/instance-ref" +import { attachWith } from "@/effect/run-service" +import { WorkspaceContext } from "@/control-plane/workspace-context" +import { Instance } from "@/project/instance" +import type { InstanceContext } from "@/project/instance-context" import { EventV2Bridge } from "@/event-v2-bridge" import { TuiEvent } from "@/server/tui-event" import { fileLog } from "@/altimate/util/file-log" @@ -47,16 +53,44 @@ export function toastFor(n: BrowserSignInNotice): SignInToast | undefined { return undefined } +/** The instance and workspace a connect was started from. */ +export interface SignInOrigin { + instance?: InstanceContext + workspace?: string +} + +/** Where the latest connect started. The SDK calls back on its own, outside any instance, and a toast published + * without a workspace is dropped by a TUI that is showing one; so the origin is captured when the connect starts. */ +let origin: SignInOrigin = {} + +/** Call when a connect starts, from the caller's own context. */ +export function rememberOrigin(): void { + const fiber = Fiber.getCurrent() + let instance: InstanceContext | undefined + if (fiber) instance = Context.getReferenceUnsafe(fiber.context, InstanceRef) + else { + try { + instance = Instance.current + } catch { + instance = undefined + } + } + const workspace = WorkspaceContext.workspaceID ?? (fiber ? Context.getReferenceUnsafe(fiber.context, WorkspaceRef) : undefined) + origin = { instance, workspace } +} + export interface SignInNoticeDeps { headless: () => boolean - toast: (t: SignInToast) => Promise + toast: (t: SignInToast, from: SignInOrigin) => Promise printLine: (line: string) => void } const defaultDeps: SignInNoticeDeps = { headless: () => process.env["ALTIMATE_CODE_HEADLESS"] === "1", - toast: async (t) => { - await AppRuntime.runPromise(EventV2Bridge.Service.use((events) => events.publish(TuiEvent.ToastShow, t))) + toast: async (t, from) => { + await AppRuntime.runPromise( + attachWith(EventV2Bridge.Service.use((events) => events.publish(TuiEvent.ToastShow, t)), from), + ) }, printLine: (line) => { try { @@ -82,7 +116,7 @@ export function handle(n: BrowserSignInNotice, deps: SignInNoticeDeps = defaultD if (n.phase === "waiting") deps.printLine(`${t.title}: ${t.message}`) return } - deps.toast(t).catch((err) => fileLog("WARN", "warehouse-sign-in", "could not show the sign-in notice", { err: String(err) })) + deps.toast(t, origin).catch((err) => fileLog("WARN", "warehouse-sign-in", "could not show the sign-in notice", { err: String(err) })) } let unsubscribe: (() => void) | undefined @@ -96,4 +130,5 @@ export function install(deps: SignInNoticeDeps = defaultDeps): void { export function resetForTests(): void { unsubscribe?.() unsubscribe = undefined + origin = {} } diff --git a/packages/opencode/test/altimate/credential-store-backend.test.ts b/packages/opencode/test/altimate/credential-store-backend.test.ts index 8cc251740f..f3fa0058f0 100644 --- a/packages/opencode/test/altimate/credential-store-backend.test.ts +++ b/packages/opencode/test/altimate/credential-store-backend.test.ts @@ -6,6 +6,7 @@ */ import { afterEach, describe, expect, test } from "bun:test" import * as CredentialStore from "../../src/altimate/native/connections/credential-store" +import * as Registry from "../../src/altimate/native/connections/registry" function memoryBackend(opts: { failSet?: boolean } = {}) { const store = new Map() @@ -76,6 +77,82 @@ describe("credential store with an OS backend", () => { expect(await CredentialStore.deleteCredential("pg", "password")).toBe(false) }) + test("re-saving a connection under the same name drops secrets the new config no longer has", async () => { + // Key-pair first, then the same name switched to browser sign-in: the old key must not come back, + // because the driver tries key-pair before any other method. + const { store, backend } = memoryBackend() + CredentialStore.setSecretBackendForTests(backend) + await CredentialStore.saveConnection("sf", { type: "snowflake", account: "a", user: "u", private_key: "-----BEGIN PRIVATE KEY-----x" } as any) + expect(store.has("sf/private_key")).toBe(true) + + const { sanitized } = await CredentialStore.saveConnection("sf", { type: "snowflake", account: "a", user: "u", authenticator: "externalbrowser" } as any) + expect(store.has("sf/private_key")).toBe(false) + expect((await CredentialStore.resolveConfig("sf", sanitized)).private_key).toBeUndefined() + }) + + test("a re-save whose new secret could not be stored keeps the old ones", async () => { + const { store, backend } = memoryBackend() + CredentialStore.setSecretBackendForTests(backend) + await CredentialStore.saveConnection("pg", { type: "postgres", password: "old", ssl_key: "k" } as any) + CredentialStore.setSecretBackendForTests({ ...backend, set: async () => { throw new Error("keychain locked") } }) + const { warnings } = await CredentialStore.saveConnection("pg", { type: "postgres", password: "new" } as any) + expect(warnings.length).toBe(1) + expect(store.get("pg/password")).toBe("old") + expect(store.get("pg/ssl_key")).toBe("k") + }) + + test("removing a connection removes its secrets from the OS store", async () => { + const { store, backend } = memoryBackend() + CredentialStore.setSecretBackendForTests(backend) + const name = `rm_${Date.now()}` + expect((await Registry.add(name, { type: "postgres", host: "h", user: "u", password: "s3cret" } as any)).success).toBe(true) + expect(store.get(`${name}/password`)).toBe("s3cret") + expect((await Registry.remove(name)).success).toBe(true) + expect([...store.keys()].filter((k) => k.startsWith(`${name}/`))).toEqual([]) + }) + + test("a connection still defined by an env var keeps its secrets when the saved copy is removed", async () => { + const { store, backend } = memoryBackend() + CredentialStore.setSecretBackendForTests(backend) + const name = `envkeep_${Date.now()}` + await Registry.add(name, { type: "postgres", host: "h", user: "u", password: "s3cret" } as any) + process.env[`ALTIMATE_CODE_CONN_${name.toUpperCase()}`] = JSON.stringify({ type: "postgres", host: "h", user: "u" }) + try { + await Registry.remove(name) + expect(store.get(`${name}/password`)).toBe("s3cret") + } finally { + delete process.env[`ALTIMATE_CODE_CONN_${name.toUpperCase()}`] + } + }) + + test("the Bun.secrets adapter calls the API in its object form and maps its results", async () => { + const calls: unknown[] = [] + const values = new Map() + const fake = { + set: async (o: { service: string; name: string; value: string }) => { + calls.push(["set", o]) + values.set(o.name, o.value) + }, + get: async (o: { service: string; name: string }) => { + calls.push(["get", o]) + return values.get(o.name) ?? null + }, + delete: async (o: { service: string; name: string }) => { + calls.push(["delete", o]) + return values.delete(o.name) + }, + } + const b = CredentialStore.loadBunSecrets(fake)! + expect(b.name).toBe("Bun.secrets") + await b.set("pg/password", "x") + expect(await b.get("pg/password")).toBe("x") + expect(await b.get("pg/missing")).toBeNull() + expect(await b.delete("pg/password")).toBe(true) + expect(await b.delete("pg/password")).toBe(false) + expect(calls[0]).toEqual(["set", { service: "altimate-code", name: "pg/password", value: "x" }]) + expect(CredentialStore.loadBunSecrets({})).toBeNull() + }) + test("with no backend the test preload keeps the real keychain untouched", async () => { // The preload sets the disable switch; re-probing must respect it. CredentialStore.setSecretBackendForTests(undefined) diff --git a/packages/opencode/test/altimate/warehouse-sign-in-notice.test.ts b/packages/opencode/test/altimate/warehouse-sign-in-notice.test.ts index b64aeb876c..192e27317a 100644 --- a/packages/opencode/test/altimate/warehouse-sign-in-notice.test.ts +++ b/packages/opencode/test/altimate/warehouse-sign-in-notice.test.ts @@ -9,6 +9,7 @@ import { afterAll, beforeEach, describe, expect, spyOn, test } from "bun:test" import { Telemetry } from "../../src/altimate/telemetry" import * as Registry from "../../src/altimate/native/connections/registry" import * as SignInNotice from "../../src/altimate/native/connections/sign-in-notice" +import { WorkspaceContext } from "../../src/control-plane/workspace-context" const URL_WITH_QUERY = "https://idp.example.com/sso/saml?SAMLRequest=abc" const waiting = { warehouse: "snowflake", account: "acme-xy123", phase: "waiting", url: URL_WITH_QUERY, timeoutMs: 120_000 } @@ -24,6 +25,18 @@ function deps(headless: boolean) { } describe("browser sign-in notice", () => { + test("the toast carries the workspace the connect started in, though the SDK calls back outside it", async () => { + // A toast published without a workspace is dropped by a TUI showing one, so the notice remembers where + // the connect began and publishes there. + SignInNotice.resetForTests() + const seen: any[] = [] + await WorkspaceContext.provide({ workspaceID: "wrk_signin", fn: () => SignInNotice.rememberOrigin() }) + SignInNotice.handle(waiting, { headless: () => false, toast: async (_t, from) => void seen.push(from), printLine: () => {} }) + expect(seen).toHaveLength(1) + expect(seen[0].workspace).toBe("wrk_signin") + SignInNotice.resetForTests() + }) + test("the TUI shows the sign-in for the whole wait, with the account and the link", () => { const d = deps(false) SignInNotice.handle(waiting, d.deps) diff --git a/packages/tui/src/util/terminal-restore.ts b/packages/tui/src/util/terminal-restore.ts index 5caa49f890..9f32b3434b 100644 --- a/packages/tui/src/util/terminal-restore.ts +++ b/packages/tui/src/util/terminal-restore.ts @@ -8,7 +8,8 @@ * * Only on an unclean exit: after a clean teardown the modes are already off, and * popping the keyboard-mode stack again could remove the shell's own setting. - * Nothing here can run after a hard kill or a native crash. + * Covers a normal exit and SIGTERM (`kill `), whose default action ends the + * process without an `exit` event. Nothing here can run after SIGKILL or a native crash. */ import { writeSync } from "fs" @@ -21,6 +22,8 @@ export const TERMINAL_RESET = export interface TerminalRestoreDeps { onExit: (fn: () => void) => () => void + onTerminate: (fn: () => void) => () => void + exit: (code: number) => void write: (text: string) => void isTTY: () => boolean } @@ -30,6 +33,11 @@ const defaultDeps: TerminalRestoreDeps = { process.once("exit", fn) return () => process.off("exit", fn) }, + onTerminate: (fn) => { + process.on("SIGTERM", fn) + return () => process.off("SIGTERM", fn) + }, + exit: (code) => process.exit(code), write: (text) => writeSync(1, text), isTTY: () => Boolean(process.stdout.isTTY), } @@ -39,13 +47,21 @@ export function restoreTerminalOnUncleanExit( renderer: { readonly isDestroyed: boolean }, deps: TerminalRestoreDeps = defaultDeps, ): () => void { - return deps.onExit(() => { + const restore = () => { if (renderer.isDestroyed || !deps.isTTY()) return try { deps.write(TERMINAL_RESET) } catch { // stdout already gone — nothing left to restore } - }) + } + const offExit = deps.onExit(restore) + // SIGTERM's default action ends the process without an `exit` event. Exiting from a listener instead fires it, + // so the restore above runs; 143 (128 + 15) is the status the default action would have given. + const offTerminate = deps.onTerminate(() => deps.exit(143)) + return () => { + offExit() + offTerminate() + } } // altimate_change end diff --git a/packages/tui/test/terminal-restore.test.ts b/packages/tui/test/terminal-restore.test.ts index 8ce91560b3..d5c1d6ae7a 100644 --- a/packages/tui/test/terminal-restore.test.ts +++ b/packages/tui/test/terminal-restore.test.ts @@ -5,6 +5,8 @@ import { restoreTerminalOnUncleanExit, TERMINAL_RESET } from "../src/util/termin function harness(opts: { destroyed: boolean; tty?: boolean }) { const renderer = { isDestroyed: opts.destroyed } let exitHandler: (() => void) | undefined + let termHandler: (() => void) | undefined + const exits: number[] = [] const written: string[] = [] const unregister = restoreTerminalOnUncleanExit(renderer, { onExit: (fn) => { @@ -13,10 +15,20 @@ function harness(opts: { destroyed: boolean; tty?: boolean }) { exitHandler = undefined } }, + onTerminate: (fn) => { + termHandler = fn + return () => { + termHandler = undefined + } + }, + exit: (code) => { + exits.push(code) + exitHandler?.() // process.exit fires the exit event + }, write: (t) => void written.push(t), isTTY: () => opts.tty ?? true, }) - return { renderer, written, exit: () => exitHandler?.(), unregister } + return { renderer, written, exits, exit: () => exitHandler?.(), term: () => termHandler?.(), unregister } } describe("terminal restore on exit", () => { @@ -36,6 +48,22 @@ describe("terminal restore on exit", () => { expect(h.written).toEqual([]) }) + test("SIGTERM restores the terminal and still ends the process with 143", () => { + // SIGTERM's default action ends the process without an `exit` event, so `kill ` left the modes on. + const h = harness({ destroyed: false }) + h.term() + expect(h.written).toEqual([TERMINAL_RESET]) + expect(h.exits).toEqual([143]) + }) + + test("unregistering removes both guards", () => { + const h = harness({ destroyed: false }) + h.unregister() + h.term() + h.exit() + expect(h.written).toEqual([]) + }) + test("nothing is written when stdout is not a terminal", () => { const h = harness({ destroyed: false, tty: false }) h.exit()