apps/backend/scripts/clickhouse-migrations.ts (1)

9-12: Using plaintext password authentication.

While plaintext_password is acceptable for development environments, consider documenting this choice or using sha256_password for better security practices.

Example with SHA256:

import { createHash } from 'crypto';

const passwordHash = createHash('sha256')
  .update(clickhouseExternalPassword)
  .digest('hex');

await client.exec({
  query: "CREATE USER IF NOT EXISTS limited_user IDENTIFIED WITH sha256_password BY {passwordHash:String}",
  query_params: { passwordHash },
});

apps/e2e/tests/backend/endpoints/api/v1/analytics-query.test.ts (2)

1-29: Good test coverage for basic query execution.

The tests appropriately use inline snapshots as per coding guidelines. Consider adding a negative test case verifying that client accessType is rejected, since the route requires serverOrHigherAuthTypeSchema.

---

274-387: Security tests are comprehensive.

Good coverage of dangerous operations (CREATE TABLE, system tables, KILL QUERY, INSERT). Note that error messages expose the internal username limited_user - consider whether this level of detail should be sanitized in the route handler before returning to clients.

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Learnt from: CR
Repo: stack-auth/stack-auth PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T02:09:03.104Z
Learning: Applies to packages/stack-shared/src/config/** : When making backwards-incompatible changes to the config schema, update the migration functions in `packages/stack-shared/src/config/schema.ts`

Learnt from: CR
Repo: stack-auth/stack-auth PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T02:09:03.104Z
Learning: Applies to apps/e2e/**/*.{ts,tsx} : Always add new E2E tests when you change the API or SDK interface

Learnt from: CR
Repo: stack-auth/stack-auth PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T02:09:03.104Z
Learning: Applies to **/*.{test,spec}.{ts,tsx} : Use `.toMatchInlineSnapshot` for test assertions instead of other selectors when possible; check snapshot-serializer.ts for formatting and non-deterministic value handling

Learnt from: CR
Repo: stack-auth/stack-auth PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T02:09:03.104Z
Learning: Applies to apps/backend/src/app/api/latest/**/*.{ts,tsx} : In the Stack Auth monorepo backend API, organize routes by resource type (auth, user management, team management, OAuth providers) in `/apps/backend/src/app/api/latest/*`

apps/backend/scripts/clickhouse-migrations.ts (1)

4-23: Migration GRANT targets a likely non-existent table and hardcodes the external user

This script still has the two issues called out in earlier reviews:

• GRANT SELECT ON analytics.allowed_table1 TO limited_user; will fail if analytics.allowed_table1 doesn’t exist yet (which seems likely given the TODO about creating migration files).
• The external user name is hardcoded as limited_user, even though STACK_CLICKHOUSE_EXTERNAL_USER exists and is used elsewhere; that can drift.

You could make this more robust along these lines:

 export async function runClickhouseMigrations() {
   console.log("Running Clickhouse migrations...");
   const client = createClickhouseClient("admin");
-  const clickhouseExternalPassword = getEnvVariable("STACK_CLICKHOUSE_EXTERNAL_PASSWORD");
-  // todo: create migration files
-  await client.exec({
-    query: "CREATE USER IF NOT EXISTS limited_user IDENTIFIED WITH plaintext_password BY {clickhouseExternalPassword:String}",
-    query_params: { clickhouseExternalPassword },
-  });
-  const queries = [
-    "GRANT SELECT ON analytics.allowed_table1 TO limited_user;",
-    "REVOKE ALL ON system.* FROM limited_user;",
-    "REVOKE CREATE, ALTER, DROP, INSERT ON *.* FROM limited_user;"
-  ];
-  for (const query of queries) {
-    console.log(query);
-    await client.exec({ query });
-  }
-  console.log("Clickhouse migrations complete");
-  await client.close();
+  const externalUser = getEnvVariable("STACK_CLICKHOUSE_EXTERNAL_USER", "limited_user");
+  const clickhouseExternalPassword = getEnvVariable("STACK_CLICKHOUSE_EXTERNAL_PASSWORD");
+
+  try {
+    // TODO: create proper ClickHouse schema migrations
+    await client.exec({
+      query: "CREATE USER IF NOT EXISTS {externalUser:Identifier} IDENTIFIED WITH plaintext_password BY {clickhouseExternalPassword:String}",
+      query_params: { externalUser, clickhouseExternalPassword },
+    });
+
+    const queries = [
+      // Enable GRANTs once the target tables exist.
+      // "GRANT SELECT ON analytics.some_table TO {externalUser:Identifier};",
+      `REVOKE ALL ON system.* FROM ${externalUser};`,
+      `REVOKE CREATE, ALTER, DROP, INSERT ON *.* FROM ${externalUser};`,
+    ];
+
+    for (const query of queries) {
+      console.log(query);
+      await client.exec({ query });
+    }
+
+    console.log("Clickhouse migrations complete");
+  } finally {
+    await client.close();
+  }
 }

Adjust the GRANT line once your analytics tables are in place.

apps/backend/src/lib/clickhouse.tsx (1)

22-55: Fix getQueryTimingStats typing and guard against missing query_log rows

This function still has the issues called out in earlier reviews:

• profile.json is typed as if it returns a flat { cpu_time_ms, wall_clock_time_ms }, but you then access stats.data[0]; this doesn’t match the ClickHouse JSON shape and will likely fail type-checking.
• At runtime, system.query_log can legitimately return an empty data array (log entry not flushed yet), making stats.data[0] undefined and crashing callers that assume stats are always present.

A safer version would be:

-export const getQueryTimingStats = async (client: ClickHouseClient, queryId: string) => {
+export const getQueryTimingStats = async (
+  client: ClickHouseClient,
+  queryId: string,
+): Promise<{ cpu_time_ms: number; wall_clock_time_ms: number }> => {
@@
-  const stats = await profile.json<{
-    cpu_time_ms: number,
-    wall_clock_time_ms: number,
-  }>();
-  return stats.data[0];
+  const stats = await profile.json<{
+    data: Array<{
+      cpu_time_ms: number;
+      wall_clock_time_ms: number;
+    }>;
+  }>();
+
+  if (!stats.data || stats.data.length === 0) {
+    throw new Error(
+      `Query timing stats not found for query_id=${queryId}; query_log entry may not be available yet.`,
+    );
+  }
+
+  return stats.data[0];
 };

Callers can then convert this error into a KnownErrors.AnalyticsQueryError or similar, as appropriate for the API surface.

.github/workflows/docker-server-test.yaml (1)

26-30: Consider adding a lightweight readiness check for ClickHouse

The container may not always be ready within 5 seconds, especially on a busy CI runner. A transient slow start would make this job flaky.

Consider replacing the fixed sleep with a small readiness loop, for example:

      - name: Setup clickhouse
        run: |
          docker run -d --name clickhouse \
            -e CLICKHOUSE_DB=analytics \
            -e CLICKHOUSE_USER=stackframe \
            -e CLICKHOUSE_PASSWORD=password \
            -p 8133:8123 clickhouse/clickhouse-server:25.10

          for i in {1..30}; do
            if docker exec clickhouse clickhouse-client --query "SELECT 1" >/dev/null 2>&1; then
              echo "ClickHouse is ready"
              break
            fi
            echo "Waiting for ClickHouse..."
            sleep 1
          done

          docker logs clickhouse

</blockquote></details>
<details>
<summary>apps/backend/.env.development (1)</summary><blockquote>

`74-80`: **ClickHouse env block looks consistent with backend usage**

The new `STACK_CLICKHOUSE_*` variables line up with `createClickhouseClient` and the CI ClickHouse setup (DB `analytics`, admin user `stackframe`, external user `limited_user`), so this should wire up cleanly for local dev.

The `dotenv-linter` warnings here (substitution and key ordering) are stylistic only; the same `${NEXT_PUBLIC_STACK_PORT_PREFIX:-81}` pattern is already used above. You can reorder the keys if you want to silence the linter, but it’s not functionally required.

</blockquote></details>

</blockquote></details>

<details>
<summary>📜 Review details</summary>

**Configuration used**: CodeRabbit UI

**Review profile**: CHILL

**Plan**: Pro

<details>
<summary>📥 Commits</summary>

Reviewing files that changed from the base of the PR and between 0c088962934cdf829c9e6b282d2fa5a4e07723a3 and 41287db62997e2a54a1084037a90136517fcbbe5.

</details>

<details>
<summary>📒 Files selected for processing (5)</summary>

* `.github/workflows/docker-server-test.yaml` (1 hunks)
* `apps/backend/.env.development` (1 hunks)
* `apps/backend/scripts/clickhouse-migrations.ts` (1 hunks)
* `apps/backend/src/lib/clickhouse.tsx` (1 hunks)
* `docker/server/.env.example` (1 hunks)

</details>

<details>
<summary>🧰 Additional context used</summary>

<details>
<summary>📓 Path-based instructions (2)</summary>

<details>
<summary>**/*.{ts,tsx}</summary>


**📄 CodeRabbit inference engine (AGENTS.md)**

> `**/*.{ts,tsx}`: Ensure code passes `pnpm typecheck` for TypeScript type validation
> Never use `toast` for blocking alerts and errors; use alerts instead as they are more visible to users
> Keep hover/click transitions snappy and fast; apply transitions after the action (e.g., smooth fade-out on hover end) rather than delaying actions with pre-transitions to avoid sluggish UI
> Never use try-catch-all, never void a promise, never use .catch(console.error) or similar; use `runAsynchronously` or `runAsynchronouslyWithAlert` for error handling instead of try-catch blocks
> Use loading indicators instead of try-catch blocks for asynchronous UI operations; button components like <Button> support async callbacks with built-in loading state
> Use ES6 maps instead of records wherever possible

Files:
- `apps/backend/src/lib/clickhouse.tsx`
- `apps/backend/scripts/clickhouse-migrations.ts`

</details>
<details>
<summary>apps/**/*.{ts,tsx}</summary>


**📄 CodeRabbit inference engine (AGENTS.md)**

> Prefer client components and dynamic functions like `usePathname` over Next.js dynamic functions to keep pages static; avoid using `await params` or similar dynamic patterns

Files:
- `apps/backend/src/lib/clickhouse.tsx`
- `apps/backend/scripts/clickhouse-migrations.ts`

</details>

</details><details>
<summary>🧠 Learnings (1)</summary>

<details>
<summary>📚 Learning: 2025-11-25T02:09:03.104Z</summary>

Learnt from: CR
Repo: stack-auth/stack-auth PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T02:09:03.104Z
Learning: Applies to packages/stack-shared/src/config/** : When making backwards-incompatible changes to the config schema, update the migration functions in packages/stack-shared/src/config/schema.ts

**Applied to files:**
- `apps/backend/scripts/clickhouse-migrations.ts`

</details>

</details><details>
<summary>🧬 Code graph analysis (2)</summary>

<details>
<summary>apps/backend/src/lib/clickhouse.tsx (1)</summary><blockquote>

<details>
<summary>packages/stack-shared/src/utils/env.tsx (1)</summary>

* `getEnvVariable` (16-58)

</details>

</blockquote></details>
<details>
<summary>apps/backend/scripts/clickhouse-migrations.ts (2)</summary><blockquote>

<details>
<summary>apps/backend/src/lib/clickhouse.tsx (1)</summary>

* `createClickhouseClient` (11-19)

</details>
<details>
<summary>packages/stack-shared/src/utils/env.tsx (1)</summary>

* `getEnvVariable` (16-58)

</details>

</blockquote></details>

</details><details>
<summary>🪛 dotenv-linter (4.0.0)</summary>

<details>
<summary>docker/server/.env.example</summary>

[warning] 20-20: [UnorderedKey] The STACK_CLICKHOUSE_ADMIN_PASSWORD key should go before the STACK_CLICKHOUSE_URL key

(UnorderedKey)

---

[warning] 21-21: [UnorderedKey] The STACK_CLICKHOUSE_EXTERNAL_PASSWORD key should go before the STACK_CLICKHOUSE_URL key

(UnorderedKey)

</details>
<details>
<summary>apps/backend/.env.development</summary>

[warning] 75-75: [SubstitutionKey] The STACK_CLICKHOUSE_URL key is not assigned properly

(SubstitutionKey)

---

[warning] 76-76: [UnorderedKey] The STACK_CLICKHOUSE_ADMIN_USER key should go before the STACK_CLICKHOUSE_URL key

(UnorderedKey)

---

[warning] 77-77: [UnorderedKey] The STACK_CLICKHOUSE_ADMIN_PASSWORD key should go before the STACK_CLICKHOUSE_ADMIN_USER key

(UnorderedKey)

---

[warning] 78-78: [UnorderedKey] The STACK_CLICKHOUSE_EXTERNAL_USER key should go before the STACK_CLICKHOUSE_URL key

(UnorderedKey)

---

[warning] 79-79: [UnorderedKey] The STACK_CLICKHOUSE_EXTERNAL_PASSWORD key should go before the STACK_CLICKHOUSE_EXTERNAL_USER key

(UnorderedKey)

---

[warning] 80-80: [UnorderedKey] The STACK_CLICKHOUSE_DATABASE key should go before the STACK_CLICKHOUSE_EXTERNAL_PASSWORD key

(UnorderedKey)

</details>

</details>

</details>

<details>
<summary>⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)</summary>

* GitHub Check: Vercel Agent Review
* GitHub Check: docker
* GitHub Check: restart-dev-and-test-with-custom-base-port
* GitHub Check: check_prisma_migrations (22.x)
* GitHub Check: setup-tests
* GitHub Check: build (22.x)
* GitHub Check: lint_and_build (latest)
* GitHub Check: all-good
* GitHub Check: build (22.x)
* GitHub Check: restart-dev-and-test
* GitHub Check: build (22.x)

</details>

</details>

<!-- This is an auto-generated comment by CodeRabbit for review status -->

docker/server/.env.example (1)

19-21: Reorder ClickHouse keys for consistency with dotenv-linter and env-var conventions.

The three ClickHouse environment variables should be in alphabetical order to match the Postgres connection strings pattern and resolve the dotenv-linter UnorderedKey warnings.

Apply this diff:

-STACK_CLICKHOUSE_URL=http://host.docker.internal:8133
 STACK_CLICKHOUSE_ADMIN_PASSWORD=password
 STACK_CLICKHOUSE_EXTERNAL_PASSWORD=password
+STACK_CLICKHOUSE_URL=http://host.docker.internal:8133

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Learnt from: CR
Repo: stack-auth/stack-auth PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T02:09:03.104Z
Learning: When making dashboard changes, provide deep links to the dashboard page in the format `http://localhost:<$NEXT_PUBLIC_STACK_PORT_PREFIX>01/projects/-selector-/...` (or use a.localhost, b.localhost, c.localhost for port prefixes 91, 92, 93)

apps/backend/scripts/clickhouse-migrations.ts (2)

14-14: GRANT statement references non-existent table.

This grants SELECT on analytics.allowed_table1, which doesn't exist yet and will cause the migration to fail.

Comment or remove this line until the table is created in a prior migration step.

---

22-23: Move client cleanup to a finally block.

If any query fails before line 23, the client won't be closed, leaving the connection open.

Apply this diff to guarantee cleanup:

-  console.log("Clickhouse migrations complete");
-  await client.close();
+  try {
+    // existing migration logic here
+    console.log("Clickhouse migrations complete");
+  } finally {
+    await client.close();
+  }

Note: Per coding guidelines, avoid try-catch-all for error handling. Let errors propagate naturally while ensuring cleanup in the finally block.

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Learnt from: CR
Repo: stack-auth/stack-auth PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-28T21:21:39.123Z
Learning: Applies to packages/stack-shared/src/config/schema.ts : Whenever making backwards-incompatible changes to the config schema, update the migration functions in `packages/stack-shared/src/config/schema.ts`

.github/workflows/docker-server-build-run.yaml (1)

26-30: ClickHouse setup step looks good and maintains consistency.

The step is well-configured:

• Uses the same ClickHouse version (25.10) as the docker-compose file for consistency
• Port mapping (8133:8123) aligns with the development environment configuration
• Environment variables match those in .env.development
• Logging output helps with debugging startup issues

Consider whether 5 seconds is sufficient for ClickHouse to fully initialize. Depending on the CI environment performance, you might want to add a health check loop similar to the "Check server health" step, or increase the sleep duration to ensure ClickHouse is ready before subsequent steps.

apps/backend/.env.development (1)

75-82: Consider alphabetically reordering the ClickHouse configuration keys for improved maintainability.

While the variable substitution pattern is already supported via the backend's custom environment variable handling, the ClickHouse keys should follow alphabetical order: STACK_CLICKHOUSE_ADMIN_PASSWORD, STACK_CLICKHOUSE_ADMIN_USER, STACK_CLICKHOUSE_DATABASE, STACK_CLICKHOUSE_EXTERNAL_PASSWORD, STACK_CLICKHOUSE_EXTERNAL_USER, STACK_CLICKHOUSE_URL.

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Learnt from: CR
Repo: stack-auth/stack-auth PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-04T18:03:49.984Z
Learning: Applies to {.env*,**/*.{ts,tsx,js}} : Prefix environment variables with STACK_ (or NEXT_PUBLIC_STACK_ if public) so changes are picked up by Turborepo and improves readability

apps/backend/scripts/clickhouse-migrations.ts (2)

14-14: Migration will fail: analytics.allowed_table1 doesn't exist.

This GRANT references a table that hasn't been created. The migration will fail at runtime. Per past discussion, this is noted as temporary, but it should be commented out or removed until the table exists.

---

9-21: Add try-finally to ensure client cleanup on failure.

If any client.exec() call throws, client.close() on line 23 won't be reached, causing a resource leak. Wrap the migration logic in a try-finally block.

 export async function runClickhouseMigrations() {
   console.log("[Clickhouse] Running Clickhouse migrations...");
-  const client = clickhouseAdminClient;
+  const client = createClickhouseClient("admin");
   const clickhouseExternalPassword = getEnvVariable("STACK_CLICKHOUSE_EXTERNAL_PASSWORD");
-  // todo: create migration files
-  await client.exec({
-    query: "CREATE USER IF NOT EXISTS limited_user IDENTIFIED WITH plaintext_password BY {clickhouseExternalPassword:String}",
-    query_params: { clickhouseExternalPassword },
-  });
-  const queries = [
-    "GRANT SELECT ON analytics.allowed_table1 TO limited_user;",
-    "REVOKE ALL ON system.* FROM limited_user;",
-    "REVOKE CREATE, ALTER, DROP, INSERT ON *.* FROM limited_user;"
-  ];
-  for (const query of queries) {
-    console.log("[Clickhouse] " + query);
-    await client.exec({ query });
+  try {
+    await client.exec({
+      query: "CREATE USER IF NOT EXISTS limited_user IDENTIFIED WITH plaintext_password BY {clickhouseExternalPassword:String}",
+      query_params: { clickhouseExternalPassword },
+    });
+    const queries = [
+      "GRANT SELECT ON analytics.allowed_table1 TO limited_user;",
+      "REVOKE ALL ON system.* FROM limited_user;",
+      "REVOKE CREATE, ALTER, DROP, INSERT ON *.* FROM limited_user;"
+    ];
+    for (const query of queries) {
+      console.log("[Clickhouse] " + query);
+      await client.exec({ query });
+    }
+    console.log("[Clickhouse] Clickhouse migrations complete");
+  } finally {
+    await client.close();
   }
-  console.log("[Clickhouse] Clickhouse migrations complete");
-  await client.close();
 }

apps/backend/src/app/api/latest/internal/analytics/query/route.ts (1)

52-53: Consider wrapping getQueryTimingStats errors.

If getQueryTimingStats fails (e.g., query log entry not found), it throws a StackAssertionError which will propagate as an internal error rather than a user-friendly AnalyticsQueryError. Consider wrapping this call for consistent error responses.

     const rows = await resultSet.data.json<Record<string, unknown>[]>();
-    const stats = await getQueryTimingStats(client, queryId);
+    let stats;
+    try {
+      stats = await getQueryTimingStats(client, queryId);
+    } catch (error) {
+      const message = error instanceof Error ? error.message : "Failed to retrieve query timing statistics";
+      throw new KnownErrors.AnalyticsQueryError(message);
+    }

apps/e2e/tests/backend/endpoints/api/v1/analytics-query.test.ts (1)

217-239: Test timeout behavior may be unstable across ClickHouse versions

The SELECT sleep(3) with 100ms timeout can be affected by ClickHouse versions where sleep queries behave inconsistently with max_execution_time enforcement. Recent ClickHouse versions show timeout behavior inconsistencies where max_execution_time is sometimes treated differently.

Consider adding an inline comment explaining that the test expects ANALYTICS_QUERY_ERROR due to server-side timeout enforcement, and note any specific ClickHouse version constraints if the timeout behavior is known to be reliable only in certain versions.

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Learnt from: N2D4
Repo: stack-auth/stack-auth PR: 1032
File: apps/backend/src/app/api/latest/analytics/query/route.tsx:20-20
Timestamp: 2025-12-17T01:23:07.460Z
Learning: In apps/backend/src/app/api/latest/analytics/query/route.tsx, the include_all_branches field controls which ClickHouse user type is used: when true, use "admin" authType for access to all branches; when false (default), use "external" authType for limited/filtered branch access.

Learnt from: CR
Repo: stack-auth/stack-auth PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-04T18:03:49.984Z
Learning: Applies to **/*.{ts,tsx} : NEVER try-catch-all, NEVER void a promise, and NEVER .catch(console.error); use loading indicators and async callbacks instead, or use runAsynchronously/runAsynchronouslyWithAlert for error handling

Learnt from: CR
Repo: stack-auth/stack-auth PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-04T18:03:49.984Z
Learning: Applies to apps/e2e/**/*.{ts,tsx} : Always add new E2E tests when changing API or SDK interface; err on the side of creating too many tests due to the critical nature of the industry

Learnt from: CR
Repo: stack-auth/stack-auth PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-04T18:03:49.984Z
Learning: Applies to **/*.{ts,tsx} : Always add new E2E tests when changing the API or SDK interface

Learnt from: CR
Repo: stack-auth/stack-auth PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-04T18:03:49.984Z
Learning: Applies to packages/stack-shared/src/config/schema.ts : Whenever making backwards-incompatible changes to the config schema, update the migration functions in packages/stack-shared/src/config/schema.ts