diff --git a/schema/setup.sql b/schema/setup.sql index badd255..70cd62e 100644 --- a/schema/setup.sql +++ b/schema/setup.sql @@ -404,7 +404,12 @@ BEGIN updated_at = NOW() FROM usage_stats us WHERE l.id = us.scar_id - AND COALESCE(l.is_active, true) = true; + AND COALESCE(l.is_active, true) = true + -- Only touch rows whose multiplier actually changes. Rewriting unchanged + -- rows bumped updated_at on every session_start, which invalidates every + -- client's on-disk embedding cache (GIT-98) for no reason. + AND l.decay_multiplier IS DISTINCT FROM + GREATEST(0.1, 1.0 - (us.times_dismissed::FLOAT / us.times_surfaced::FLOAT) * 0.8); GET DIAGNOSTICS v_updated = ROW_COUNT; @@ -529,9 +534,12 @@ CREATE TABLE IF NOT EXISTS gitmem_license_activations ( ALTER TABLE gitmem_licenses ENABLE ROW LEVEL SECURITY; ALTER TABLE gitmem_license_activations ENABLE ROW LEVEL SECURITY; +-- Drop-then-create so setup.sql can be re-applied (CREATE POLICY has no IF NOT EXISTS) +DROP POLICY IF EXISTS "Service role full access" ON gitmem_licenses; CREATE POLICY "Service role full access" ON gitmem_licenses FOR ALL USING (auth.role() = 'service_role'); +DROP POLICY IF EXISTS "Service role full access" ON gitmem_license_activations; CREATE POLICY "Service role full access" ON gitmem_license_activations FOR ALL USING (auth.role() = 'service_role'); diff --git a/src/services/analytics.ts b/src/services/analytics.ts index a1a637e..e42e827 100644 --- a/src/services/analytics.ts +++ b/src/services/analytics.ts @@ -201,7 +201,7 @@ export async function queryScarUsageByDateRange( surfaced_at: `gte.${startDate}`, }; - return directQueryAll("scar_usage", { + return directQueryAll(getTableName("scar_usage"), { select: "scar_id,scar_title,scar_severity,agent,reference_type,execution_successful,surfaced_at", filters, order: "surfaced_at.desc", diff --git a/src/services/behavioral-decay.ts b/src/services/behavioral-decay.ts index f29071f..652e715 100644 --- a/src/services/behavioral-decay.ts +++ b/src/services/behavioral-decay.ts @@ -12,6 +12,7 @@ */ import { isConfigured, safeInFilter } from "./supabase-client.js"; +import { getTableName } from "./tier.js"; const SUPABASE_URL = process.env.SUPABASE_URL || ""; const SUPABASE_KEY = process.env.SUPABASE_SERVICE_ROLE_KEY || process.env.SUPABASE_KEY || ""; @@ -98,7 +99,7 @@ export async function fetchDismissalCounts( try { // Query scar_usage for the given scar IDs (last 90 days) - const url = new URL(`${SUPABASE_REST_URL}/scar_usage`); + const url = new URL(`${SUPABASE_REST_URL}/${getTableName("scar_usage")}`); url.searchParams.set("select", "scar_id,reference_type"); url.searchParams.set("scar_id", safeInFilter(scarIds)); url.searchParams.set("surfaced_at", `gte.${new Date(Date.now() - 90 * 86400000).toISOString()}`); diff --git a/src/tools/record-scar-usage-batch.ts b/src/tools/record-scar-usage-batch.ts index b50c5ab..01c3783 100644 --- a/src/tools/record-scar-usage-batch.ts +++ b/src/tools/record-scar-usage-batch.ts @@ -161,7 +161,7 @@ export async function recordScarUsageBatch( // Insert all usage records in parallel const insertPromises = usageRecords.map(async (record) => { const result = await supabase.directUpsert<{ id: string }>( - "scar_usage", + getTableName("scar_usage"), record ); return result?.id || null; diff --git a/src/tools/record-scar-usage.ts b/src/tools/record-scar-usage.ts index b6e5146..40ca688 100644 --- a/src/tools/record-scar-usage.ts +++ b/src/tools/record-scar-usage.ts @@ -10,7 +10,7 @@ import { v4 as uuidv4 } from "uuid"; import { wrapDisplay } from "../services/display-protocol.js"; import * as supabase from "../services/supabase-client.js"; -import { hasSupabase } from "../services/tier.js"; +import { hasSupabase, getTableName } from "../services/tier.js"; import { getStorage } from "../services/storage.js"; import { detectAgent } from "../services/agent-detection.js"; import { getCurrentSession } from "../services/session-state.js"; @@ -61,7 +61,7 @@ export async function recordScarUsage( if (hasSupabase()) { const upsertStart = Date.now(); - await supabase.directUpsert("scar_usage", usageData); + await supabase.directUpsert(getTableName("scar_usage"), usageData); breakdown.upsert = { latency_ms: Date.now() - upsertStart, source: "supabase", diff --git a/tests/unit/schema-setup-reapply.test.ts b/tests/unit/schema-setup-reapply.test.ts new file mode 100644 index 0000000..af11f8e --- /dev/null +++ b/tests/unit/schema-setup-reapply.test.ts @@ -0,0 +1,57 @@ +/** + * schema/setup.sql must be safe to apply twice (GIT-84). + * + * Customers re-run setup.sql to pick up schema changes. CREATE POLICY and + * CREATE TRIGGER have no IF NOT EXISTS, so each needs a DROP ... IF EXISTS + * first; one missing pair (gitmem_licenses) made the second run fail at + * statement 72 on a real Supabase. + * + * Also pins the GIT-84 guard on refresh_scar_behavioral_scores(): rows whose + * decay_multiplier would not change must not be rewritten, because the rewrite + * bumps updated_at and invalidates every client's GIT-98 disk cache. + */ + +import { describe, it, expect } from "vitest"; +import { readFileSync } from "fs"; +import { join } from "path"; + +const SQL = readFileSync(join(__dirname, "../../schema/setup.sql"), "utf-8") + // strip -- comments so commented-out statements don't count + .replace(/--[^\n]*/g, ""); + +const norm = (s: string) => s.replace(/\s+/g, " ").trim().toLowerCase(); + +describe("schema/setup.sql re-apply safety (GIT-84)", () => { + it("drops every policy before creating it", () => { + const created = [...SQL.matchAll(/CREATE POLICY\s+("[^"]+")\s+ON\s+([\w.]+)/gi)].map((m) => `${m[1]} ON ${m[2]}`); + expect(created.length).toBeGreaterThan(0); + const missing = created.filter((p) => { + const [name, table] = p.split(" ON "); + const drop = new RegExp(`DROP POLICY IF EXISTS\\s+${name.replace(/[.*+?^${}()|[\]\\]/g, "\\$&")}\\s+ON\\s+${table}\\s*;`, "i"); + const dropAt = SQL.search(drop); + const createAt = SQL.search(new RegExp(`CREATE POLICY\\s+${name.replace(/[.*+?^${}()|[\]\\]/g, "\\$&")}\\s+ON\\s+${table}\\b`, "i")); + return dropAt < 0 || dropAt > createAt; + }); + expect(missing).toEqual([]); + }); + + it("drops every trigger before creating it", () => { + const created = [...SQL.matchAll(/CREATE TRIGGER\s+(\w+)[\s\S]*?\bON\s+([\w.]+)/gi)].map((m) => [m[1], m[2]]); + expect(created.length).toBeGreaterThan(0); + const missing = created.filter(([name, table]) => !new RegExp(`DROP TRIGGER IF EXISTS\\s+${name}\\s+ON\\s+${table}\\s*;`, "i").test(SQL)); + expect(missing).toEqual([]); + }); + + it("creates tables and indexes with IF NOT EXISTS", () => { + const bare = [...SQL.matchAll(/CREATE\s+(?:UNIQUE\s+)?(TABLE|INDEX)\s+(?!IF NOT EXISTS)(\w+)/gi)].map((m) => `${m[1]} ${m[2]}`); + expect(bare).toEqual([]); + }); + + it("refresh_scar_behavioral_scores() only rewrites rows whose multiplier changes", () => { + const fn = SQL.slice(SQL.search(/CREATE OR REPLACE FUNCTION refresh_scar_behavioral_scores/i)); + const update = norm(fn.slice(0, fn.indexOf("GET DIAGNOSTICS"))); + const expr = "greatest(0.1, 1.0 - (us.times_dismissed::float / us.times_surfaced::float) * 0.8)"; + expect(update).toContain(`set decay_multiplier = ${expr}`); + expect(update).toContain(`and l.decay_multiplier is distinct from ${expr}`); + }); +}); diff --git a/tests/unit/table-name-literals.test.ts b/tests/unit/table-name-literals.test.ts new file mode 100644 index 0000000..569f046 --- /dev/null +++ b/tests/unit/table-name-literals.test.ts @@ -0,0 +1,115 @@ +/** + * Guard: Supabase table names must go through getTableName() (GIT-84). + * + * The user schema is prefixed (gitmem_ by default, GITMEM_TABLE_PREFIX to + * override). A bare literal like "scar_usage" passed straight to PostgREST + * silently 404s on every customer project — that is how scar usage was lost. + * + * Checked: + * - directUpsert / directQuery / directQueryAll / directPatch whose table + * argument is a string literal + * - `table: "..."` option objects (listRecords and friends) + * - REST URLs built as `${SUPABASE_REST_URL}/name` or `/rest/v1/name` + * The storage layer (getStorage().upsert/query/get) takes a COLLECTION name + * and applies the prefix itself, so there the rule is reversed: passing a + * prefixed name or getTableName() would double-prefix. + * + * Known literals awaiting a ruling are allowlisted below, each with a reason. + * Removing an entry is the point — the list should only shrink. + */ + +import { describe, it, expect } from "vitest"; +import { readFileSync, readdirSync, statSync } from "fs"; +import { join, relative } from "path"; + +const SRC = join(__dirname, "../../src"); + +function collectTsFiles(dir: string): string[] { + const out: string[] = []; + for (const entry of readdirSync(dir)) { + const full = join(dir, entry); + if (statSync(full).isDirectory()) out.push(...collectTsFiles(full)); + else if (entry.endsWith(".ts") && !entry.endsWith(".test.ts") && !entry.endsWith(".d.ts")) out.push(full); + } + return out; +} + +/** + * Literal table names that setup.sql creates under exactly that name (or that + * live outside the user schema). Changing the code alone would break them, so + * they stay literal until ruled on. Keyed by table name. + */ +const ALLOWED_LITERALS: Record = { + gitmem_query_metrics: "setup.sql creates it with a hard-coded gitmem_ prefix — ruling needed (GIT-84 sweep)", + knowledge_triples: "setup.sql creates it unprefixed — ruling needed (GIT-84 sweep)", + scar_enforcement_variants: "setup.sql creates it unprefixed; column drift tracked in GIT-106", + variant_assignments: "not created by setup.sql at all — ruling needed (GIT-84 sweep)", + variant_performance_metrics: "not created by setup.sql at all — ruling needed (GIT-84 sweep)", + community_feedback: "gitmem's own feedback endpoint (feedback-remote.ts), not the user schema", +}; + +/** Files that build REST URLs for diagnostics with a literal prefix — ruling needed (GIT-84 sweep). */ +const ALLOWED_URL_FILES = new Set(["src/commands/check.ts"]); + +const DIRECT_CALL = /\b(directUpsert|directQuery|directQueryAll|directPatch)\s*(?:<(?:[^<>]|<[^<>]*>)*>)?\s*\(\s*(["'`])([A-Za-z_][\w]*)\2/g; +const TABLE_OPTION = /\btable:\s*(["'`])([A-Za-z_][\w]*)\1/g; +const REST_URL = /(?:SUPABASE_REST_URL\}|\/rest\/v1)\/([A-Za-z_][\w]*)/g; +const STORAGE_CALL = /\b(?:storage|getStorage\(\))\s*\.\s*(upsert|query|get)\s*(?:<(?:[^<>]|<[^<>]*>)*>)?\s*\(\s*([^,)]+)/g; + +interface Finding { file: string; line: number; text: string } + +function scan(): { bare: Finding[]; doublePrefixed: Finding[]; allowlistedSeen: Set } { + const bare: Finding[] = []; + const doublePrefixed: Finding[] = []; + const allowlistedSeen = new Set(); + + for (const full of collectTsFiles(SRC)) { + const file = relative(join(SRC, ".."), full); + const src = readFileSync(full, "utf-8"); + const lineOf = (i: number) => src.slice(0, i).split("\n").length; + const flag = (name: string, i: number, text: string) => { + if (name in ALLOWED_LITERALS) { allowlistedSeen.add(name); return; } + bare.push({ file, line: lineOf(i), text }); + }; + + for (const m of src.matchAll(DIRECT_CALL)) flag(m[3], m.index!, `${m[1]}("${m[3]}")`); + for (const m of src.matchAll(TABLE_OPTION)) flag(m[2], m.index!, `table: "${m[2]}"`); + for (const m of src.matchAll(REST_URL)) { + if (m[1] === "rpc") continue; // RPC function names are not tables + if (ALLOWED_URL_FILES.has(file)) continue; + flag(m[1], m.index!, `REST /${m[1]}`); + } + for (const m of src.matchAll(STORAGE_CALL)) { + const arg = m[2].trim(); + if (/getTableName\s*\(/.test(arg) || /^["'`]gitmem_/.test(arg)) { + doublePrefixed.push({ file, line: lineOf(m.index!), text: `storage.${m[1]}(${arg})` }); + } + } + } + return { bare, doublePrefixed, allowlistedSeen }; +} + +describe("Supabase table names go through getTableName() (GIT-84)", () => { + const { bare, doublePrefixed, allowlistedSeen } = scan(); + const fmt = (fs: Finding[]) => fs.map((f) => `${f.file}:${f.line} ${f.text}`).join("\n"); + + it("passes no bare table-name literal to directUpsert/directQuery/directQueryAll/directPatch, table: options or REST URLs", () => { + expect(fmt(bare), "wrap these in getTableName()").toBe(""); + }); + + it("never hands the storage layer an already-prefixed name (it prefixes collections itself)", () => { + expect(fmt(doublePrefixed), "pass the bare collection name to storage").toBe(""); + }); + + it("keeps the allowlist honest: every entry is still in use", () => { + const stale = Object.keys(ALLOWED_LITERALS).filter((name) => !allowlistedSeen.has(name)); + expect(stale, "remove allowlist entries that no longer occur").toEqual([]); + }); + + it("would catch the GIT-84 bug (self-test on a synthetic source)", () => { + const sample = `await supabase.directUpsert<{ id: string }>(\n "scar_usage",\n record\n);`; + const hits = [...sample.matchAll(DIRECT_CALL)].map((m) => m[3]); + expect(hits).toEqual(["scar_usage"]); + expect([...`new URL(\`\${SUPABASE_REST_URL}/scar_usage\`)`.matchAll(REST_URL)].map((m) => m[1])).toEqual(["scar_usage"]); + }); +});