refactor: resolve code review findings including dialect sniffing, standalone migrations, SQLite transactions, ORM parameterization, request timeouts, PAT redaction, and Postgres CI testing

Author: mhenke

.github/workflows/ci.yml

@@ -6,10 +6,28 @@ on:
   pull_request:
     branches: [main]
 
+permissions:
+  contents: read
+
 jobs:
   test:
     runs-on: ubuntu-latest
 
+    services:
+      postgres:
+        image: postgres:16
+        env:
+          POSTGRES_USER: postgres
+          POSTGRES_PASSWORD: postgres
+          POSTGRES_DB: postgres
+        ports:
+          - 5432:5432
+        options: >-
+          --health-cmd pg_isready
+          --health-interval 10s
+          --health-timeout 5s
+          --health-retries 5
+
     steps:
       - uses: actions/checkout@v4
 
@@ -22,4 +40,7 @@ jobs:
 
       - run: npx tsc --noEmit
 
-      - run: npm test
+      - name: Run tests
+        run: npm test
+        env:
+          TEST_POSTGRES_URL: postgres://postgres:postgres@localhost:5432/postgres

.github/workflows/daily-budget-check.yml

@@ -3,6 +3,9 @@ on:
   schedule:
     - cron: '0 9 * * *'
   workflow_dispatch: {}
+permissions:
+  contents: read
+
 jobs:
   budget-sync:
     runs-on: ubuntu-latest

.github/workflows/daily-forecast.yml

@@ -3,6 +3,9 @@ on:
   schedule:
     - cron: '0 8 * * *'
   workflow_dispatch: {}
+permissions:
+  contents: read
+
 jobs:
   forecast:
     runs-on: ubuntu-latest

.github/workflows/nightly-etl.yml

@@ -3,6 +3,9 @@ on:
   schedule:
     - cron: '0 1 * * *'
   workflow_dispatch: {}
+permissions:
+  contents: read
+
 jobs:
   etl:
     runs-on: ubuntu-latest

.github/workflows/weekly-classify.yml

@@ -3,6 +3,9 @@ on:
   schedule:
     - cron: '0 6 * * 1'
   workflow_dispatch: {}
+permissions:
+  contents: read
+
 jobs:
   classify:
     runs-on: ubuntu-latest

src/budget/budget_sync.ts

@@ -65,7 +65,7 @@ function computeAlertLevel(pctOfBudget7d: number | null, pctOfBudget30d: number
 }
 
 async function getLatestPoolSnapshot(db: DbClient): Promise<PoolSnapshot | null> {
-  const isPg = db.constructor.name.toLowerCase().includes('postgres');
+  const isPg = typeof db.run !== 'function' && !db.constructor?.name?.toLowerCase().includes('sqlite');
 
   if (isPg) {
     const results = await db
@@ -93,7 +93,7 @@ async function getLatestPoolSnapshot(db: DbClient): Promise<PoolSnapshot | null>
 }
 
 async function getYesterdaySnapshot(db: DbClient, date: string): Promise<BudgetSnapshot | null> {
-  const isPg = db.constructor.name.toLowerCase().includes('postgres');
+  const isPg = typeof db.run !== 'function' && !db.constructor?.name?.toLowerCase().includes('sqlite');
 
   if (isPg) {
     const results = await db
@@ -138,7 +138,7 @@ async function upsertBudgetSnapshot(
     note: string | null;
   },
 ): Promise<void> {
-  const isPg = db.constructor.name.toLowerCase().includes('postgres');
+  const isPg = typeof db.run !== 'function' && !db.constructor?.name?.toLowerCase().includes('sqlite');
 
   if (isPg) {
     await db

src/budget/notifications.ts

@@ -8,8 +8,10 @@ import { notificationLogPg, notificationLogSq } from '../db/schema.js';
 export function sanitizeErrorMessage(error: unknown): string {
   const message = error instanceof Error ? error.message : String(error);
 
-  // Strip GitHub PAT patterns (ghp_, gho_, ghu_, ghs_, ghr_)
-  const sanitized = message.replace(/gh[pousr]_[a-zA-Z0-9]{36,}/g, '[REDACTED]');
+  // Strip GitHub PAT patterns (classic ghp_ etc and fine-grained github_pat_)
+  const sanitized = message
+    .replace(/gh[pousr]_[a-zA-Z0-9]{36,}/g, '[REDACTED]')
+    .replace(/github_pat_[a-zA-Z0-9_]{82}/g, '[REDACTED]');
 
   // Truncate very long error bodies (likely API response bodies)
   if (sanitized.length > 500) {
@@ -71,6 +73,7 @@ export async function sendSlackNotification(
       method: 'POST',
       headers: { 'Content-Type': 'application/json' },
       body: JSON.stringify(payload),
+      signal: AbortSignal.timeout(15000),
     });
 
     if (!response.ok) {
@@ -152,6 +155,7 @@ export async function sendGitHubIssue(
         'Accept': 'application/vnd.github+json',
       },
       body: JSON.stringify(payload),
+      signal: AbortSignal.timeout(15000),
     });
 
     if (!response.ok) {
@@ -209,7 +213,7 @@ async function logNotification(
     errorMessage?: string;
   },
 ): Promise<void> {
-  const isPg = db.constructor.name.toLowerCase().includes('postgres');
+  const isPg = typeof db.run !== 'function' && !db.constructor?.name?.toLowerCase().includes('sqlite');
 
   if (isPg) {
     await db.insert(notificationLogPg).values({

src/classify/engine.ts

@@ -40,13 +40,35 @@ export type ClassifyResult = {
   };
 };
 
+/**
+ * Map credit consumption percentile to a consumption tier.
+ * Percentile thresholds:
+ * - >= 85%: extreme
+ * - >= 60% and < 85%: high
+ * - >= 25% and < 60%: medium
+ * - < 25%: low
+ * 
+ * @param percentile The calculated percentile (value between 0 and 1)
+ */
 function assignConsumptionTier(percentile: number): ConsumptionTier {
   if (percentile >= 0.85) return 'extreme';
   if (percentile >= 0.60) return 'high';
   if (percentile >= 0.25) return 'medium';
   return 'low';
 }
 
+/**
+ * Classify users based on their credit usage relative to the organization.
+ * Calculates credit consumption percentiles and maps them to consumption tiers.
+ * Maps team assignments to business value tiers based on the config.
+ * 
+ * If total users < 4, falls back to assigning all users to the 'medium' consumption tier.
+ * 
+ * @param userCredits List of user GitHub logins and total credits used over 30 days
+ * @param currentUsers Current users database records
+ * @param config Team resolving config mapping teams to business value tiers
+ * @param reason Reason for running the classification (e.g., weekly_recalc, manual)
+ */
 export function classifyUsers(
   userCredits: UserCredits[],
   currentUsers: CurrentUser[],

src/classify/runner.ts

@@ -1,8 +1,8 @@
 import { loadValueConfig, resolveValueTier as resolveValueTierFn } from './value_config.js';
 import { classifyUsers } from './engine.js';
 import type { DbClient } from '../db/client.js';
-import { sql } from 'drizzle-orm';
-import { usersPg, usersSq, classificationHistoryPg, classificationHistorySq } from '../db/schema.js';
+import { sql, gte } from 'drizzle-orm';
+import { usersPg, usersSq, classificationHistoryPg, classificationHistorySq, dailyUsagePg, dailyUsageSq } from '../db/schema.js';
 
 export type ClassifyOptions = {
   valueConfigPath: string;
@@ -35,69 +35,59 @@ export async function runClassify(
   const valueConfig = loadValueConfig(options.valueConfigPath);
   const resolveValueTier = (team: string | null) => resolveValueTierFn(team, valueConfig);
 
-  // Read phase: get 30 days of daily_usage aggregates
-  const usageQuery = isSqlite
-    ? `SELECT github_login, SUM(credits) as credits
-       FROM daily_usage
-       WHERE usage_date >= date('now', '-30 days')
-       GROUP BY github_login`
-    : `SELECT github_login, SUM(credits::numeric) as credits
-       FROM daily_usage
-       WHERE usage_date >= CURRENT_DATE - INTERVAL '30 days'
-       GROUP BY github_login`;
-
-  const usageRows = await (isSqlite
-    ? Promise.resolve(db.all(sql.raw(usageQuery)) as Array<{ github_login: string; credits: string }>)
-    : db.execute(sql.raw(usageQuery)).then((r: any) => r.rows as Array<{ github_login: string; credits: string }>));
+  const dailyUsageTable = isSqlite ? dailyUsageSq : dailyUsagePg;
+  const usersTable = isSqlite ? usersSq : usersPg;
+
+  // Calculate threshold date (30 days ago) in JS as YYYY-MM-DD
+  const thresholdDate = new Date();
+  thresholdDate.setDate(thresholdDate.getDate() - 30);
+  const dateString = thresholdDate.toISOString().slice(0, 10);
+
+  // Read phase: get 30 days of daily_usage aggregates using Drizzle
+  const usageRows = await db
+    .select({
+      github_login: dailyUsageTable.githubLogin,
+      credits: sql<number>`SUM(${dailyUsageTable.credits})`.mapWith(Number),
+    })
+    .from(dailyUsageTable)
+    .where(gte(dailyUsageTable.usageDate, dateString))
+    .groupBy(dailyUsageTable.githubLogin);
 
   if (usageRows.length === 0) {
     throw new Error('No daily_usage data found. Run `burnrate etl` first.');
   }
 
-  // Check for 30 distinct days
-  const distinctDaysQuery = isSqlite
-    ? `SELECT COUNT(DISTINCT usage_date) as days FROM daily_usage WHERE usage_date >= date('now', '-30 days')`
-    : `SELECT COUNT(DISTINCT usage_date) as days FROM daily_usage WHERE usage_date >= CURRENT_DATE - INTERVAL '30 days'`;
-
-  const daysResult = await (isSqlite
-    ? Promise.resolve(db.all(sql.raw(distinctDaysQuery)) as Array<{ days: number }>)
-    : db.execute(sql.raw(distinctDaysQuery)).then((r: any) => r.rows as Array<{ days: number }>));
-
-  if (daysResult[0].days < 30) {
-    throw new Error(`Insufficient data: only ${daysResult[0].days} distinct days found, need 30.`);
+  // Check for 30 distinct days using Drizzle
+  const daysResult = await db
+    .select({
+      days: sql<number>`COUNT(DISTINCT ${dailyUsageTable.usageDate})`.mapWith(Number),
+    })
+    .from(dailyUsageTable)
+    .where(gte(dailyUsageTable.usageDate, dateString));
+
+  const distinctDays = daysResult[0]?.days ?? 0;
+  if (distinctDays < 30) {
+    throw new Error(`Insufficient data: only ${distinctDays} distinct days found, need 30.`);
   }
 
-  // Read current users
-  const usersQuery = `SELECT github_login, team, consumption_tier, value_tier, bucket_updated_at FROM users`;
-  const usersRows = await (isSqlite
-    ? Promise.resolve(db.all(sql.raw(usersQuery)) as Array<{
-        github_login: string;
-        team: string | null;
-        consumption_tier: string | null;
-        value_tier: string | null;
-        bucket_updated_at: string | null;
-      }>)
-    : db.execute(sql.raw(usersQuery)).then((r: any) => r.rows as Array<{
-        github_login: string;
-        team: string | null;
-        consumption_tier: string | null;
-        value_tier: string | null;
-        bucket_updated_at: string | null;
-      }>));
+  // Read current users using Drizzle
+  const usersRows = await db
+    .select({
+      github_login: usersTable.githubLogin,
+      team: usersTable.team,
+      consumption_tier: usersTable.consumptionTier,
+      value_tier: usersTable.valueTier,
+      bucket_updated_at: usersTable.bucketUpdatedAt,
+    })
+    .from(usersTable);
 
   // Classify
-  const userCredits = usageRows.map((r: { github_login: string; credits: string }) => ({
+  const userCredits = usageRows.map((r: any) => ({
     githubLogin: r.github_login,
     totalCredits: Number(r.credits),
   }));
 
-  const currentUsers = usersRows.map((r: {
-    github_login: string;
-    team: string | null;
-    consumption_tier: string | null;
-    value_tier: string | null;
-    bucket_updated_at: string | null;
-  }) => ({
+  const currentUsers = usersRows.map((r: any) => ({
     githubLogin: r.github_login,
     team: r.team,
     consumptionTier: r.consumption_tier,
@@ -108,33 +98,36 @@ export async function runClassify(
   const effectiveDate = new Date().toISOString().slice(0, 10);
   const result = classifyUsers(userCredits, currentUsers, { resolveValueTier }, options.reason);
 
-  // Write phase: use transactions for PostgreSQL, batch for SQLite
+  // Write phase: use transaction for SQLite and PostgreSQL
   if (result.changes.length > 0) {
     const now = new Date().toISOString();
 
     if (isSqlite) {
-      // SQLite: batch updates (better-sqlite3 transactions require different setup)
-      for (const change of result.changes) {
-        await db.update(usersSq)
-          .set({
-            consumptionTier: change.consumptionTierNew,
-            valueTier: change.valueTierNew,
-            bucketUpdatedAt: now,
-            updatedAt: sql`CURRENT_TIMESTAMP`,
-          })
-          .where(sql`${usersSq.githubLogin} = ${change.githubLogin}`);
-
-        await db.insert(classificationHistorySq)
-          .values({
-            effectiveDate,
-            githubLogin: change.githubLogin,
-            consumptionTierOld: change.consumptionTierOld,
-            consumptionTierNew: change.consumptionTierNew,
-            valueTier: change.valueTierNew,
-            reason: change.reason,
-          })
-          .onConflictDoNothing();
-      }
+      db.transaction((tx: any) => {
+        for (const change of result.changes) {
+          tx.update(usersSq)
+            .set({
+              consumptionTier: change.consumptionTierNew,
+              valueTier: change.valueTierNew,
+              bucketUpdatedAt: now,
+              updatedAt: sql`CURRENT_TIMESTAMP`,
+            })
+            .where(sql`${usersSq.githubLogin} = ${change.githubLogin}`)
+            .run();
+
+          tx.insert(classificationHistorySq)
+            .values({
+              effectiveDate,
+              githubLogin: change.githubLogin,
+              consumptionTierOld: change.consumptionTierOld,
+              consumptionTierNew: change.consumptionTierNew,
+              valueTier: change.valueTierNew,
+              reason: change.reason,
+            })
+            .onConflictDoNothing()
+            .run();
+        }
+      });
     } else {
       await db.transaction(async (tx: any) => {
         for (const change of result.changes) {

src/db/migrate.ts

@@ -1,4 +1,5 @@
 import { sql } from 'drizzle-orm';
+import { fileURLToPath } from 'node:url';
 
 export const pgSchemaStatements = [
   `CREATE TABLE IF NOT EXISTS raw_reports (
@@ -214,3 +215,22 @@ export async function runMigrations(db: any): Promise<void> {
     }
   }
 }
+
+if (process.argv[1] === fileURLToPath(import.meta.url)) {
+  const url = process.env.DATABASE_URL;
+  if (!url) {
+    console.error('Error: DATABASE_URL environment variable is required.');
+    process.exit(1);
+  }
+  const { initDb, closeDb } = await import('./client.js');
+  const db = initDb(url);
+  try {
+    await runMigrations(db);
+    console.log('Migrations completed successfully.');
+  } catch (err) {
+    console.error('Migrations failed:', err);
+    process.exit(1);
+  } finally {
+    await closeDb();
+  }
+}