feat: Snowflake connector integration - #401

apps/sqlrooms-cli-ui/src/runtimeConfig.ts (1)

9-20: Extract a shared bridge config type to avoid drift.

The connection shape here is duplicated in apps/sqlrooms-cli-ui/src/store.ts. Exporting a shared RuntimeDbBridgeConfig/RuntimeDbBridgeConnection type from this module would keep both sides in sync.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@apps/sqlrooms-cli-ui/src/runtimeConfig.ts` around lines 9 - 20, Extract and
export shared types for the bridge config from this module: define and export
RuntimeDbBridgeConnection (with fields id, engineId, title, runtimeSupport?:
'browser'|'server'|'both', requiresBridge?: boolean, bridgeId?: string, isCore?:
boolean) and RuntimeDbBridgeConfig (with id and connections:
RuntimeDbBridgeConnection[]), then replace the inline connection/type definition
in store.ts to import and use these exported types
(RuntimeDbBridgeConfig/RuntimeDbBridgeConnection) so both modules share the
exact same type definitions.

python/sqlrooms-cli/tests/test_db_bridge.py (1)

31-70: Add a regression test for duplicate connector IDs.

Given registry registration is ID-keyed, add a test that registering the same connection_id twice raises (after the register guard fix). This prevents future silent overrides.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@python/sqlrooms-cli/tests/test_db_bridge.py` around lines 31 - 70, Add a
regression test that ensures DbBridgeRegistry.register rejects duplicate
connection IDs: create a DbBridgeRegistry(bridge_id="bridge-id"), register a
connector with a specific connection id (e.g., using the existing
_FakeConnector()), then attempt to register another connector with the same
connection id and assert that registering the second one raises an exception
(use pytest.raises and refer to DbBridgeRegistry.register). Ensure the test name
reflects duplicate registration (e.g.,
test_registry_raises_for_duplicate_connection_id) and uses the same connection
id as in test_registry_routes_to_registered_connector so future silent overrides
are prevented.

python/sqlrooms-cli/README.md (1)

83-86: Prefer env vars over CLI args for Snowflake passwords in the primary example.

Line 85 documents --snowflake-password, which is easy to leak via shell history/process inspection. Recommend using env vars in the main snippet and keeping CLI password flag as secondary/advanced usage.

📝 Safer docs tweak

 uvx sqlrooms \
   ./sqlrooms.db \
   --ws-port 4000 \
   --port 4173 \
   --snowflake-account "<account>" \
   --snowflake-user "<user>" \
-  --snowflake-password "<password>" \
   --snowflake-warehouse "<warehouse>" \
   --snowflake-database "<database>" \
   --snowflake-schema "<schema>"
+
+# Prefer env var for secrets:
+# SNOWFLAKE_PASSWORD="<password>" uvx sqlrooms ...

Also applies to: 101-102

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@python/sqlrooms-cli/README.md` around lines 83 - 86, Update the primary
README example to avoid passing secrets on the CLI by replacing the use of the
--snowflake-password flag with an environment variable (e.g.,
SNOWFLAKE_PASSWORD) and show how to export it before running the command; keep
the existing --snowflake-password option only as a secondary/advanced usage
note. Reference the example lines that currently show --snowflake-password and
the related Snowflake flags so reviewers can update the snippet and add a short
note about why env var usage is preferred.

Configuration used: defaults

Review profile: CHILL

Plan: Pro

apps/sqlrooms-cli-ui/src/store.ts (1)

262-281: Consider wrapping bridge registration in a try-catch.

The bridge registration runs at module load time. If createHttpDbBridge or the registration methods throw, it could prevent the entire application from loading.

🛡️ Suggested defensive wrapper

 const bridgeConfig = getRuntimeBridgeConfig();
 if (bridgeConfig?.connections.length) {
+  try {
     const bridge = createHttpDbBridge({
       id: bridgeConfig.id,
       baseUrl: runtimeConfig.apiBaseUrl || '',
     });
     const state = roomStore.getState();
     state.db.connectors.registerBridge(bridge);
     for (const connection of bridgeConfig.connections) {
       const normalizedConnection: DbConnection = {
         id: connection.id,
         engineId: connection.engineId,
         title: connection.title || connection.id,
         runtimeSupport: connection.runtimeSupport || 'server',
         requiresBridge: connection.requiresBridge ?? true,
         bridgeId: connection.bridgeId || bridgeConfig.id,
         isCore: connection.isCore ?? false,
       };
       state.db.connectors.registerConnection(normalizedConnection);
     }
+  } catch (err) {
+    console.error('Failed to register DB bridge:', err);
+  }
 }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@apps/sqlrooms-cli-ui/src/store.ts` around lines 262 - 281, The bridge
registration at module load uses getRuntimeBridgeConfig(), createHttpDbBridge(),
roomStore.getState(), state.db.connectors.registerBridge(), and
state.db.connectors.registerConnection() and can throw during app
initialization; wrap the entire block that creates the bridge and registers
connections in a try-catch so any exception from createHttpDbBridge or the
register* calls is caught, log or report the error (including the error object)
and skip registering the bridge/connections as a safe fallback to avoid breaking
app load.

python/sqlrooms-cli/sqlrooms/web/db_bridge.py (2)

145-150: Wrap connection test in exception handling to return False on failure.

If _connect() or the SELECT 1 query fails (e.g., bad credentials, network issues), the exception propagates to the caller. The test_connection semantics suggest returning False on failure rather than raising.

🛡️ Suggested pattern

 def test_connection(self) -> bool:
+    try:
         with self._connect() as conn:
             with conn.cursor() as cur:
                 cur.execute("SELECT 1")
                 cur.fetchone()
         return True
+    except Exception:
+        return False

Also applies to: 275-280

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@python/sqlrooms-cli/sqlrooms/web/db_bridge.py` around lines 145 - 150, The
test_connection function currently lets exceptions from _connect() or the SELECT
1 query bubble up; wrap the body of test_connection in a try/except that catches
broad connection/query exceptions (Exception) and returns False on any failure
while returning True on success; keep the existing context managers (with
self._connect() as conn / with conn.cursor() as cur) so resources close
normally, and apply the same try/except pattern to the similar connection-test
block around lines 275–280 (the other connection-testing function/section) to
ensure failures return False instead of raising.

---

208-210: Query cancellation is a no-op for both connectors.

Both PostgresBridgeConnector.cancel_query and SnowflakeBridgeConnector.cancel_query return False without attempting cancellation. Consider adding a TODO comment or raising NotImplementedError to make this explicit, or implementing actual cancellation using pg_cancel_backend() for Postgres and Snowflake's query cancellation API.

Also applies to: 355-357

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@python/sqlrooms-cli/sqlrooms/web/db_bridge.py` around lines 208 - 210, The
cancel_query implementations are no-ops (cancel_query in the base or connectors,
and PostgresBridgeConnector.cancel_query /
SnowflakeBridgeConnector.cancel_query) which silently return False; either make
the lack of support explicit by raising NotImplementedError or add clear TODO
comments, or implement real cancellation: for Postgres call pg_cancel_backend()
using the backend PID tracked by the query/session, and for Snowflake call its
query cancellation API; update cancel_query (and the duplicate at the other
occurrence) to either raise NotImplementedError or perform the appropriate
cancellation and return True/False based on success.

Configuration used: defaults

Review profile: CHILL

Plan: Pro

python/sqlrooms-cli/sqlrooms/web/db_bridge.py (1)

238-240: Query cancellation not implemented for Postgres or Snowflake.

Both connectors' cancel_query methods return False without attempting cancellation. Consider adding a TODO comment or docstring noting this limitation, especially since the streaming endpoints support abort signals that trigger cancellation requests.

📝 Suggested documentation

     def cancel_query(self, query_id: str) -> bool:
+        # TODO: Implement query cancellation using pg_cancel_backend() or connection.cancel()
         _ = query_id
         return False

     def cancel_query(self, query_id: str) -> bool:
+        # TODO: Implement query cancellation using session-level ABORT or connection.cursor.abort_query()
         _ = query_id
         return False

Also applies to: 399-401

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@python/sqlrooms-cli/sqlrooms/web/db_bridge.py` around lines 238 - 240, The
cancel_query methods (cancel_query) for the Postgres and Snowflake connectors
currently return False without attempting cancellation; update those methods to
either implement cancellation using the connector's cancel API or at minimum add
a clear TODO/docstring stating that cancellation is not supported and why, and
document that streaming endpoints send abort signals that will trigger
cancellation requests; reference the cancel_query method in db_bridge.py (and
the duplicate occurrences around the other cancel_query implementation) and
ensure the docstring/TODO mentions Postgres/Snowflake specifically and links to
where an abort/cancel request should be handled.

packages/db/src/DbSlice.ts (1)

292-314: Consider making chunkRows configurable.

The chunkRows is hardcoded to 5000 at Line 300. Consider exposing this as an optional parameter on QueryExecutionRequest to allow tuning for different query sizes and memory constraints.

♻️ Proposed extension

In types.ts QueryExecutionRequest:

+  /**
+   * Optional row batch size for streaming materialization.
+   */
+  chunkRows?: number;

Then in DbSlice.ts:

-            chunkRows: 5000,
+            chunkRows: request.chunkRows ?? 5000,

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/db/src/DbSlice.ts` around lines 292 - 314, The code hardcodes
chunkRows to 5000 in DbSlice.ts when calling bridge.fetchArrowStream inside the
materialization branch; make chunk size configurable by adding an optional
chunkRows?: number field to the QueryExecutionRequest type and use
request.chunkRows (falling back to 5000) when calling bridge.fetchArrowStream in
the materializeArrowStreamResult flow (symbols to change: QueryExecutionRequest,
DbSlice.ts call site where bridge.fetchArrowStream is invoked, and any caller
that constructs QueryExecutionRequest to optionally pass chunkRows); update
callers/tests to pass a tuned value where needed.

packages/db/__tests__/arrow-streaming.test.ts (1)

49-71: Unused variable payload2 in test.

payload2 is defined at Line 49 but never used in the test logic - it's only checked in an assertion that it still equals its original value, which is always true. This appears to be leftover code.

🧹 Proposed cleanup

   it('parses framed binary messages across chunk boundaries', async () => {
     const payload1 = new Uint8Array([1, 2, 3]);
-    const payload2 = new Uint8Array([4, 5]);
     const frame1 = encodeFrame({type: 'batch', payload: payload1});
     const frame2 = encodeFrame({type: 'end'});
...
     expect(parsed[1]?.type).toBe('end');
     expect(Array.from(parsed[1]?.payload ?? [])).toEqual([]);
-    expect(Array.from(payload2)).toEqual([4, 5]);
   });

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/db/__tests__/arrow-streaming.test.ts` around lines 49 - 71, The test
defines an unused variable payload2 and only asserts its literal value at the
end; remove the dead code by deleting the payload2 declaration and the final
expect(Array.from(payload2)).toEqual([4, 5]) assertion, or instead use payload2
in the test frames if the intent was to validate a second payload. Locate
payload2 and the trailing expect in the test (related symbols: payload2,
encodeFrame, readerFromChunks, parseFramedBinaryStream) and either remove both
lines or modify the frames/expectations so payload2 is actually sent and
asserted.

Configuration used: defaults

Review profile: CHILL

Plan: Pro

python/sqlrooms-cli/sqlrooms/web/db_bridge.py (1)

81-82: ⚠️ Potential issue | 🟠 Major

Duplicate connection_id registration still silently overwrites.

This issue was flagged in a previous review. register() silently replaces an existing connector with the same ID, which can hide configuration mistakes and unexpectedly reroute queries.

🔧 Proposed fix

 def register(self, connector: DbBridgeConnector) -> None:
+    existing = self._connectors.get(connector.connection_id)
+    if existing is not None and existing is not connector:
+        raise ValueError(
+            f"Duplicate DB bridge connection_id: {connector.connection_id}"
+        )
     self._connectors[connector.connection_id] = connector

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@python/sqlrooms-cli/sqlrooms/web/db_bridge.py` around lines 81 - 82, The
register method currently overwrites an existing connector silently; change
register(self, connector: DbBridgeConnector) to check self._connectors for
connector.connection_id and refuse/raise an explicit error (e.g., ValueError or
KeyError) or return a clear failure if the ID already exists, so duplicate
connection_id registrations are detected instead of silently replacing the
existing entry; ensure the check references self._connectors and
connector.connection_id and update any callers/tests to expect the new behavior.

apps/sqlrooms-cli-ui/src/components/AssistantDrawer.tsx (1)

143-143: Minor: Trailing whitespace after closing div.

Line 143 has a trailing {' '} JSX expression that appears unnecessary.

🧹 Remove trailing whitespace

-              </div>{' '}
+              </div>

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@apps/sqlrooms-cli-ui/src/components/AssistantDrawer.tsx` at line 143, In
AssistantDrawer.tsx, remove the unnecessary trailing JSX whitespace expression
{' '} after the closing div on line with the JSX fragment (inside the
AssistantDrawer component render/return) — locate the closing </div> near the
referenced snippet and delete the {' '} token so the markup has no trailing
whitespace expression.

python/sqlrooms-cli/tests/test_cli.py (1)

111-116: Consider using pytest.raises for cleaner exception assertions.

The try/except/else pattern works but pytest.raises is more idiomatic in pytest.

♻️ Suggested refactor

-    try:
-        _load_connector_config([config_path])
-    except RuntimeError as exc:
-        assert "Duplicate connector id" in str(exc)
-    else:
-        raise AssertionError("Expected duplicate connector id failure")
+    with pytest.raises(RuntimeError, match="Duplicate connector id"):
+        _load_connector_config([config_path])

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@python/sqlrooms-cli/tests/test_cli.py` around lines 111 - 116, Replace the
manual try/except/else assertion with pytest's context manager: in the test
calling _load_connector_config([config_path]) use pytest.raises(RuntimeError) as
excinfo to assert the exception is raised, then assert "Duplicate connector id"
in str(excinfo.value); update the test import to include pytest if not present
and remove the try/except/else block around the _load_connector_config call.

python/sqlrooms-cli/README.md (1)

133-149: Postgres and Snowflake sections show identical commands.

Both connector sections show the exact same CLI command with no differentiation. Consider adding comments or notes explaining that connectors are configured via sqlrooms.toml, or show example config snippets inline.

📝 Suggested improvement

 ### Postgres

+Ensure your `sqlrooms.toml` includes a Postgres connector:
+
+```toml
+[[connectors]]
+id = "postgres-local"
+engine = "postgres"
+dsn = "postgresql://user:pass@localhost:5432/db"
+```
+
 ```bash
 uvx sqlrooms \
   ./sqlrooms.db \
   --ws-port 4000 \
   --port 4173

</details>

<details>
<summary>🤖 Prompt for AI Agents</summary>

Verify each finding against the current code and only fix it if needed.

In @python/sqlrooms-cli/README.md around lines 133 - 149, The Postgres and
Snowflake sections currently show identical CLI usage; update the README to
clarify that connector selection is done via the sqlrooms.toml config rather
than differing CLI flags: in the Postgres and Snowflake subsections add a short
note stating "connectors are configured in sqlrooms.toml" and include an example
[[connectors]] TOML snippet showing engine = "postgres" with a DSN and engine =
"snowflake" with appropriate fields, and either keep the shared uvx sqlrooms
command with that explanatory note or show one identical command plus the
distinct example config per connector to make the difference explicit.

</details>

</blockquote></details>
<details>
<summary>python/sqlrooms-cli/sqlrooms/web/db_bridge.py (2)</summary><blockquote>

`237-253`: **`cancel_query` is a no-op for PostgresBridgeConnector.**

The method ignores `query_id` and always returns `False`. While this may be acceptable for an initial implementation, consider documenting this limitation or implementing actual cancellation using `pg_cancel_backend` if query IDs are tracked.

<details>
<summary>🤖 Prompt for AI Agents</summary>

Verify each finding against the current code and only fix it if needed.

In @python/sqlrooms-cli/sqlrooms/web/db_bridge.py around lines 237 - 253, The
cancel_query method is a no-op; either document this limitation or implement
real cancellation: when running stream_arrow_batches (or other query-starting
methods) capture the Postgres backend PID (pg_backend_pid()) and associate it
with the query_id (e.g., in an in-memory dict keyed by query_id), then implement
cancel_query(query_id) to look up the PID and call pg_cancel_backend(pid) (using
a new connection via _connect()) and return True/False based on the result;
ensure cleanup of the mapping when the query completes or is canceled.

</details>

---

`54-69`: **Memory concern: Full result set materialized before Arrow serialization.**

`_rows_to_arrow_bytes` loads all rows into memory via `_rows_to_json_rows` before creating the Arrow table. For large result sets, this could cause significant memory pressure. Consider streaming Arrow batches directly from cursor chunks in `stream_arrow_batches`.

<details>
<summary>🤖 Prompt for AI Agents</summary>

Verify each finding against the current code and only fix it if needed.

In @python/sqlrooms-cli/sqlrooms/web/db_bridge.py around lines 54 - 69,
_rows_to_arrow_bytes currently materializes the entire result set by calling
_rows_to_json_rows and pa.Table.from_pylist, which risks OOM for large queries;
change the implementation so it does not build the full JSON list but instead
accepts/iterates cursor chunks (or expose a helper used by stream_arrow_batches)
and writes Arrow output incrementally using RecordBatch(s) (e.g., build
pa.RecordBatch from each chunk and use
pa.ipc.new_stream/pa.RecordBatchStreamWriter to write batches to a
BufferOutputStream), ensuring the schema is derived up front (or from the first
chunk) and handling the empty-columns case similarly to the current columns ->
pa.null() behavior; update or create an overload used by stream_arrow_batches so
rows can be streamed rather than fully materialized by _rows_to_json_rows and
pa.Table.from_pylist.

</details>

</blockquote></details>
<details>
<summary>apps/sqlrooms-cli-ui/src/store.ts (1)</summary><blockquote>

`239-240`: **Avoid `as any` type assertion for `defaultProvider`.**

The `as any` cast bypasses type checking. Consider using proper typing or a more specific assertion if the provider string needs to conform to a union type.


<details>
<summary>♻️ Suggested improvement</summary>

If `defaultProvider` must be a specific union type, validate it at runtime:

```diff
-        defaultProvider: defaultProviderFromConfig as any,
+        defaultProvider: defaultProviderFromConfig,

If the type is genuinely incompatible, consider updating AiSliceConfig to accept string or adding a type guard.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@apps/sqlrooms-cli-ui/src/store.ts` around lines 239 - 240, The assignment
uses a blanket `as any` cast for defaultProvider, which bypasses type safety;
remove the `as any` and either (a) narrow the value by casting to the specific
union (e.g., validate with an isValidProvider(value) type guard and assign only
if it passes, falling back to a safe default), or (b) update the
AiSliceConfig/defaultProvider type to accept string if appropriate; locate the
code where defaultProvider: defaultProviderFromConfig is set and implement a
runtime check (isValidProvider or a small switch/lookup) or adjust the type
definitions so the assignment is type-correct without `as any`.

Configuration used: defaults

Review profile: CHILL

Plan: Pro