diff --git a/.sqruff b/.sqruff index 816218d05..8448c7ecf 100644 --- a/.sqruff +++ b/.sqruff @@ -13,9 +13,9 @@ [sqruff] dialect = postgres -# Only the layout + capitalisation rules are excluded (see per-rule notes); every -# other default rule stays enabled. -exclude_rules = LT01,LT02,LT05,CP02 +# Only the layout rules are excluded (see per-rule notes); every capitalisation +# and correctness rule stays enabled. +exclude_rules = LT01,LT02,LT05 # LT01 (layout.spacing): the schema aligns column types into readable columns # (id TEXT PRIMARY KEY / slug TEXT NOT NULL). Multi-space alignment is @@ -26,6 +26,9 @@ exclude_rules = LT01,LT02,LT05,CP02 # LT05 (layout.long_lines): the file's value is its dense inline design-rationale # comments (with design.md / RIG-NNNN cross-refs); an 80-col cap would force # mechanical, meaning-fragmenting rewraps of prose, not SQL. -# CP02 (capitalisation.identifiers): the file uses uppercase SQL keywords and -# access-method names (USING GIN); CP02 misreads GIN as an identifier that must -# be lowercased, fighting the consistent uppercase-keyword style. +# (Capitalisation rules CP01–CP05 all stay ON: the schema uses a consistent +# uppercase-keyword style and the linter enforces it. The one access-method +# keyword is written lowercase — `USING gin` (canonical Postgres) — so CP02 +# does not misread `GIN` as an identifier needing lowercasing. Earlier CP02 +# was excluded solely to suppress that one false positive; lowercasing the +# access method is the narrower fix and keeps the identifier-case check live.) diff --git a/bun.lock b/bun.lock index 20ba35388..822413a3d 100644 --- a/bun.lock +++ b/bun.lock @@ -246,6 +246,16 @@ "typescript": "catalog:", }, }, + "tools/sql-migration-gate": { + "name": "@compass/sql-migration-gate", + "bin": { + "sql-migration-gate": "./index.ts", + }, + "devDependencies": { + "@types/bun": "catalog:", + "typescript": "catalog:", + }, + }, "tools/stamp-gate": { "name": "@compass/stamp-gate", "devDependencies": { @@ -491,6 +501,8 @@ "@compass/sea-ref-gate": ["@compass/sea-ref-gate@workspace:tools/sea-ref-gate"], + "@compass/sql-migration-gate": ["@compass/sql-migration-gate@workspace:tools/sql-migration-gate"], + "@compass/stamp-gate": ["@compass/stamp-gate@workspace:tools/stamp-gate"], "@compass/toolchain-parity": ["@compass/toolchain-parity@workspace:tools/toolchain"], diff --git a/go/internal/store/migrations/0001_init.sql b/go/internal/store/migrations/0001_init.sql index 99bbf168f..eb202c8ca 100644 --- a/go/internal/store/migrations/0001_init.sql +++ b/go/internal/store/migrations/0001_init.sql @@ -308,7 +308,7 @@ CREATE INDEX messages_mentions_unrouted_idx ON messages (seq) WHERE mentions_rou CREATE INDEX messages_topic_seq_idx ON messages (topic_id, seq DESC); -- Full-text search index (design.md:1137-1139): GIN over the generated tsvector. -CREATE INDEX messages_search_idx ON messages USING GIN (search_tsv); +CREATE INDEX messages_search_idx ON messages USING gin (search_tsv); -- Idempotency: at most one stored message per (author, client_request_id) when -- the key is supplied, so a retried PostMessage returns the stored row instead diff --git a/tools/sql-migration-gate/biome.json b/tools/sql-migration-gate/biome.json new file mode 100644 index 000000000..ece11dd9b --- /dev/null +++ b/tools/sql-migration-gate/biome.json @@ -0,0 +1,4 @@ +{ + "extends": "//", + "linter": { "rules": { "suspicious": { "noConsole": "off" } } } +} diff --git a/tools/sql-migration-gate/index.test.ts b/tools/sql-migration-gate/index.test.ts new file mode 100644 index 000000000..f11503341 --- /dev/null +++ b/tools/sql-migration-gate/index.test.ts @@ -0,0 +1,185 @@ +// Unit tests for the sql-migration-gate's pure core + I/O orchestration. +// +// This gate is a CI oracle: it decides whether the first-party migrations pass +// the squawk (safety) + sqruff (style) batteries. Its whole reason to be a +// script is that the previous inline-`bash -c` form combined the two exit codes +// with a shell expression moon double-expanded to a constant `exit 0`, so the +// gate ran fail-OPEN. This suite defends the machine-readable contract the bug +// violated: the exit-code combination is fail-closed, and runOnce runs BOTH +// linters before combining. +// +// Conventions (mirroring tools/inline-sql-gate/index.test.ts): +// - Literal expectations, not values derived from the module. + +import { describe, expect, test } from "bun:test"; +import { mkdirSync, mkdtempSync, rmSync, writeFileSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { + combineExitCodes, + type Deps, + formatVerdict, + type LinterResult, + MIGRATION_GLOB, + makeSpawnLinter, + runOnce, +} from "./index.ts"; + +const ok = (name: string): LinterResult => ({ name, code: 0, output: "" }); +const fail = (name: string): LinterResult => ({ + name, + code: 1, + output: `${name} findings`, +}); +const broke = (name: string): LinterResult => ({ + name, + code: 2, + output: `${name} could not run`, +}); + +// --------------------------------------------------------------------------- +// combineExitCodes — the fail-closed contract the false-green bug violated. +// --------------------------------------------------------------------------- + +describe("combineExitCodes", () => { + test("both pass -> 0", () => { + expect(combineExitCodes([ok("squawk"), ok("sqruff")])).toBe(0); + }); + + test("squawk finds, sqruff clean -> 1 (fail-closed on either)", () => { + expect(combineExitCodes([fail("squawk"), ok("sqruff")])).toBe(1); + }); + + test("squawk clean, sqruff finds -> 1 (the case the old gate hid)", () => { + expect(combineExitCodes([ok("squawk"), fail("sqruff")])).toBe(1); + }); + + test("both find -> 1", () => { + expect(combineExitCodes([fail("squawk"), fail("sqruff")])).toBe(1); + }); + + test("a spawn failure (2) dominates so an un-run gate is never green", () => { + expect(combineExitCodes([broke("squawk"), ok("sqruff")])).toBe(2); + expect(combineExitCodes([ok("squawk"), broke("sqruff")])).toBe(2); + expect(combineExitCodes([broke("squawk"), fail("sqruff")])).toBe(2); + }); + + test("no results -> 0 (vacuous; runOnce guards the empty-glob case)", () => { + expect(combineExitCodes([])).toBe(0); + }); +}); + +// --------------------------------------------------------------------------- +// formatVerdict — the human-readable line. +// --------------------------------------------------------------------------- + +describe("formatVerdict", () => { + test("all-pass names every linter", () => { + expect(formatVerdict([ok("squawk"), ok("sqruff")])).toContain("OK"); + expect(formatVerdict([ok("squawk"), ok("sqruff")])).toContain( + "squawk + sqruff", + ); + }); + + test("failure names only the failing linters", () => { + const v = formatVerdict([ok("squawk"), fail("sqruff")]); + expect(v).toContain("FAIL"); + expect(v).toContain("sqruff"); + expect(v).not.toContain("squawk + sqruff"); + }); +}); + +// --------------------------------------------------------------------------- +// runOnce — orchestration: BOTH linters run, output streamed, code combined. +// --------------------------------------------------------------------------- + +function harness(codes: Record) { + const ran: string[] = []; + const errs: string[] = []; + const logs: string[] = []; + const deps: Deps = { + runLinter: async (name) => { + ran.push(name); + const code = codes[name] ?? 0; + return { name, code, output: code === 0 ? "" : `${name} findings` }; + }, + log: (m) => logs.push(m), + err: (m) => errs.push(m), + }; + return { deps, ran, errs, logs }; +} + +describe("runOnce", () => { + test("runs BOTH linters even when the first fails, surfacing both outputs", async () => { + const { deps, ran, errs } = harness({ squawk: 1, sqruff: 1 }); + await runOnce(deps); + expect(ran).toEqual(["squawk", "sqruff"]); + // Both batteries' findings must surface in one push — the old bug hid + // one half; dropping either err() call would re-hide it. + const joined = errs.join("\n"); + expect(joined).toContain("squawk findings"); + expect(joined).toContain("sqruff findings"); + }); + + test("returns 1 when only sqruff finds — the exact regression", async () => { + const { deps, ran, logs } = harness({ squawk: 0, sqruff: 1 }); + expect(await runOnce(deps)).toBe(1); + expect(ran).toEqual(["squawk", "sqruff"]); + // A failing gate must NOT emit the OK line. + expect(logs.join("\n")).not.toContain("OK"); + }); + + test("returns 0 and logs OK when both pass", async () => { + const { deps, logs } = harness({ squawk: 0, sqruff: 0 }); + expect(await runOnce(deps)).toBe(0); + expect(logs.join("\n")).toContain("OK"); + }); + + test("clean run emits no blank output lines (only the OK verdict)", async () => { + const { deps, errs } = harness({ squawk: 0, sqruff: 0 }); + await runOnce(deps); + // Clean linters produce empty output; the guard must suppress those so + // stderr carries no blank noise ahead of the OK line. + expect(errs).toEqual([]); + }); + + test("propagates a spawn failure as 2", async () => { + const { deps } = harness({ squawk: 2, sqruff: 0 }); + expect(await runOnce(deps)).toBe(2); + }); +}); + +// --------------------------------------------------------------------------- +// makeSpawnLinter — the REAL spawn path: empty-glob and missing-binary both +// resolve to the documented code 2 (never an escaping throw, never green). +// --------------------------------------------------------------------------- + +describe("makeSpawnLinter", () => { + test("empty glob (no migrations under root) -> code 2", async () => { + const root = mkdtempSync(join(tmpdir(), "sql-gate-empty-")); + try { + const linter = makeSpawnLinter(root); + const res = await linter("squawk", [MIGRATION_GLOB]); + expect(res.code).toBe(2); + expect(res.output).toContain("no migrations matched"); + } finally { + rmSync(root, { recursive: true, force: true }); + } + }); + + test("missing binary throws in spawn -> mapped to code 2, not an escaping rejection", async () => { + const root = mkdtempSync(join(tmpdir(), "sql-gate-nobin-")); + const migDir = join(root, "go/internal/store/migrations"); + mkdirSync(migDir, { recursive: true }); + writeFileSync(join(migDir, "0001_init.sql"), "SELECT 1;\n"); + try { + // A binary that cannot exist on PATH; Bun.spawn throws synchronously. + const linter = makeSpawnLinter(root); + const res = await linter("squawk-does-not-exist-xyz", [MIGRATION_GLOB]); + expect(res.code).toBe(2); + expect(res.output).toContain("could not spawn"); + } finally { + rmSync(root, { recursive: true, force: true }); + } + }); +}); diff --git a/tools/sql-migration-gate/index.ts b/tools/sql-migration-gate/index.ts new file mode 100644 index 000000000..11184fb44 --- /dev/null +++ b/tools/sql-migration-gate/index.ts @@ -0,0 +1,176 @@ +// sql-migration-gate — SQL migration lint over the first-party migrations +// under go/internal/store/migrations/. Two nix-pinned linters run +// UNCONDITIONALLY and their exit codes are OR'd, so one push surfaces both +// batteries' findings together and the gate fails if EITHER fails: +// +// squawk migration-SAFETY analysis (unsafe DDL). Config in /.squawk.toml. +// sqruff SQL style/lint (structural + correctness + capitalisation). Config +// in /.sqruff. +// +// WHY THIS IS A SCRIPT, NOT AN INLINE `bash -c`: +// The gate was a moon `command: 'bash -c "squawk …; rc=$?; sqruff …; rc2=$?; +// exit $(( rc | rc2 ))"'`. moon wraps every task command in its OWN +// `bash -c ""`, and the nested double quotes collide: the OUTER shell +// expands `$?`, `$rc`, `$rc2`, and `$(( rc | rc2 ))` (all unset → 0) BEFORE the +// inner shell runs, so the inner shell received a literal `…; rc=0; …; rc2=0; +// exit 0` and ran fail-OPEN — it printed every finding and always exited 0. The +// bug went unseen because 0001_init.sql genuinely passes both linters; the +// first migration to actually trip a finding would have shipped green. A script +// makes the exit-code combination real code (rule://scripts-ts-over-bash + the +// no-bash-gate CI task forbid this logic in bash) and unit-testable. +// +// Inputs (env): +// GATE_ROOT - workspace root to run the linters from. Default: the git +// toplevel, falling back to process.cwd() when git reports none +// (a jj workspace's .git lives in the colocated clone). moon runs +// this with runFromWorkspaceRoot:true, so cwd is the repo root in +// CI. The linters discover their repo-root configs (/.squawk.toml, +// /.sqruff) and the migration glob resolves repo-relative from +// here. +// Exit codes: +// 0 - both linters passed (no findings). +// 1 - one or both linters reported findings. +// 2 - a linter could not be spawned / internal error. + +import { $ } from "bun"; + +/** The migration glob both linters lint, relative to the gate root. */ +export const MIGRATION_GLOB = "go/internal/store/migrations/*.sql"; + +/** One linter's result: its name, exit code, and combined stdout+stderr. */ +export interface LinterResult { + name: string; + code: number; + output: string; +} + +/** + * Combine the linters' exit codes into the gate's exit code. Fail-closed: the + * gate fails (1) if ANY linter reported findings (non-zero), passes (0) only + * when every linter passed. A spawn/internal failure (code 2) dominates so a + * gate that could not actually run never reads as green. + * + * Pure and exported: this is the contract the false-green bug violated, so it + * is the unit-tested core. + */ +export function combineExitCodes(results: LinterResult[]): number { + let exit = 0; + for (const { code } of results) { + if (code === 2) return 2; + if (code !== 0) exit = 1; + } + return exit; +} + +/** Render the human-readable gate verdict from the linters' results. */ +export function formatVerdict(results: LinterResult[]): string { + const failed = results.filter((r) => r.code !== 0).map((r) => r.name); + if (failed.length === 0) { + return `sql-migration-gate: OK — ${results.map((r) => r.name).join(" + ")} passed.`; + } + return `sql-migration-gate: FAIL — findings from ${failed.join(" + ")}. See the annotations above.`; +} + +export interface Deps { + /** Run one linter over the glob; returns its exit code + combined output. */ + runLinter: (name: string, argv: string[]) => Promise; + log: (msg: string) => void; + err: (msg: string) => void; +} + +/** + * Run both linters over the migration glob and combine their exit codes. + * Both ALWAYS run (findings from both batteries surface in one push) before the + * codes are combined. + */ +export async function runOnce(deps: Deps): Promise { + const { runLinter, log, err } = deps; + + const squawk = await runLinter("squawk", [MIGRATION_GLOB]); + if (squawk.output) err(squawk.output); + const sqruff = await runLinter("sqruff", ["lint", MIGRATION_GLOB]); + if (sqruff.output) err(sqruff.output); + + const results = [squawk, sqruff]; + const exit = combineExitCodes(results); + const verdict = formatVerdict(results); + if (exit === 0) log(verdict); + else err(verdict); + return exit; +} + +/** + * Build the production `runLinter`: resolve the migration glob to real file + * paths under `root` and spawn the linter over them. Exported so the real + * spawn path (including the missing-binary throw → code 2) is unit-testable. + */ +export function makeSpawnLinter( + root: string, +): (name: string, argv: string[]) => Promise { + return async (name, argv) => { + // Glob expansion is the shell's job in the original gate; do it here + // so each linter receives real file paths, not a literal glob. A glob + // that matches nothing is a hard error — a gate with no subject must + // not read as green. + const glob = new Bun.Glob(MIGRATION_GLOB); + const files = [...glob.scanSync({ cwd: root, onlyFiles: true })] + .map((f) => f.replaceAll("\\", "/")) + .sort(); + if (files.length === 0) { + return { + name, + code: 2, + output: `sql-migration-gate: no migrations matched ${MIGRATION_GLOB} under ${root}`, + }; + } + // argv is [] for squawk, ["lint", ] for sqruff — replace + // the glob token with the resolved file list. + const resolved = argv.flatMap((a) => (a === MIGRATION_GLOB ? files : [a])); + try { + const proc = Bun.spawn([name, ...resolved], { + cwd: root, + stdout: "pipe", + stderr: "pipe", + }); + const [stdout, stderr] = await Promise.all([ + new Response(proc.stdout).text(), + new Response(proc.stderr).text(), + ]); + const code = await proc.exited; + return { name, code, output: (stdout + stderr).trimEnd() }; + } catch (e) { + // Bun.spawn throws synchronously when the binary is missing + // (ENOENT — e.g. a PATH regression or running outside the dev + // shell). Map it to the documented code 2 with a clean message + // instead of letting it escape as an unhandled rejection with a + // raw stack trace. Still fail-closed: 2 dominates the combine. + return { + name, + code: 2, + output: `sql-migration-gate: could not spawn ${name}: ${e instanceof Error ? e.message : String(e)}`, + }; + } + }; +} + +if (import.meta.main) { + // Resolve the root the linters run from and the migration glob resolves + // against, in priority order: explicit GATE_ROOT, then the git toplevel, + // then process.cwd(). The git toplevel is empty in a jj workspace (its .git + // lives in the colocated clone, not the workspace) — a legitimate local dev + // context, not an error — and moon runs this with runFromWorkspaceRoot:true + // so cwd is the repo root in CI. Falling back to cwd (never "") keeps every + // valid environment working without the empty-string-as-path fragility. + const gitTop = ( + await $`git rev-parse --show-toplevel`.nothrow().quiet().text() + ).trim(); + const root = process.env.GATE_ROOT ?? (gitTop || process.cwd()); + + process.exit( + await runOnce({ + runLinter: makeSpawnLinter(root), + log: (msg) => console.log(msg), + err: (msg) => console.error(msg), + }), + ); +} diff --git a/tools/sql-migration-gate/moon.yml b/tools/sql-migration-gate/moon.yml index 035625378..a079b28eb 100644 --- a/tools/sql-migration-gate/moon.yml +++ b/tools/sql-migration-gate/moon.yml @@ -7,22 +7,34 @@ # squawk migration-SAFETY analysis — flags unsafe DDL (a NOT-NULL column with # no default, a full-table rewrite, a non-CONCURRENT index on a live # table). Config + accepted-rule rationale in /.squawk.toml. -# sqruff SQL style/lint — structural + correctness rules kept, pure-layout -# reformatting rules excluded for this hand-authored schema. Config in -# /.sqruff. +# sqruff SQL style/lint — structural + correctness + capitalisation rules +# kept, pure-layout reformatting rules excluded for this hand-authored +# schema. Config in /.sqruff. # -# NOT a bun or go project: it execs two nix-provided binaries over a file glob, -# so it inherits neither the tag-bun install nor the whole-repo biome lint/format -# (excluded below). A nix-tool gate like its siblings (flake-gate, guest-image, -# agent-image): `language: nix` + `tags: ['ci-group.nix']` (even though the work -# is a shell exec, not a nix build — the sibling convention), so it rides the -# same non-bun/non-go CI leg and `moon run :ci` sweeps it. +# A bun/TypeScript CLI (`bun` tag), like its gate siblings inline-sql-gate and +# microvm-boot-test: install is inherited via .moon/tasks/tag-bun.yml (the shared +# root install) and lint/format are whole-repo root tasks, so this leaf carries +# no own bun.lock. The `check` task execs the two nix-provided linter binaries +# (squawk/sqruff) — both are on PATH on every moon CI leg (ci.yml phase-two puts +# the devenv nixpkgs tools on PATH for each running leg, gated on matrix.run, not +# on the group), so a bun-group leg has them just as the nix leg did. +# +# WHY A SCRIPT, NOT AN INLINE `bash -c`: the gate WAS +# `command: 'bash -c "squawk …; rc=$?; sqruff …; rc2=$?; exit $(( rc | rc2 ))"'`. +# moon wraps every task command in its own `bash -c ""`, and the nested +# double quotes collided: the OUTER shell expanded `$?`, `$rc`, `$rc2`, and +# `$(( rc | rc2 ))` (all unset → 0) BEFORE the inner shell ran, so the inner +# shell got a literal `…; rc=0; …; rc2=0; exit 0` and the gate ran fail-OPEN — +# it printed every finding and always exited 0. It went unnoticed because +# 0001_init.sql genuinely passes both linters. The exit-code combination now +# lives in index.ts (rule://scripts-ts-over-bash + the no-bash-gate CI task) and +# is unit-tested (index.test.ts). # # Compass CI is a single moon-driven job (.github/workflows/ci.yml runs # `moon run :ci`), so the `ci` aggregate below is swept automatically. layer: 'tool' -language: 'nix' -tags: ['ci-group.nix'] +language: 'typescript' +tags: ['bun', 'ci-group.bun'] # Affected-detection walks the PROJECT GRAPH ONLY (moon query projects # --affected → downstream); it NEVER consults a project's cross-tree task @@ -44,20 +56,25 @@ tags: ['ci-group.nix'] dependsOn: - 'compass-go' -workspace: - inheritedTasks: - exclude: ['install', 'lint', 'format'] - tasks: + typecheck: + command: 'bunx tsc --noEmit' + deps: ['install'] + inputs: ['*.ts', 'tsconfig.json', '/tsconfig.base.json', 'package.json', '/bun.lock'] + test: + # The pure core (index.ts): the fail-closed exit-code combination + verdict + # formatting + the no-short-circuit orchestration — the contract the old + # inline-`bash -c` form silently violated. + command: 'bun test' + deps: ['install'] + inputs: ['*.ts', 'tsconfig.json', '/tsconfig.base.json', 'package.json', '/bun.lock'] check: - # Both linters over every first-party migration. Run from the workspace root - # so squawk/sqruff discover their repo-root configs (/.squawk.toml, /.sqruff) - # and the glob resolves repo-relative. squawk takes the files as args; sqruff - # takes `lint `. Run BOTH unconditionally and OR their exit codes so a - # single push surfaces squawk-safety AND sqruff-style findings together (a - # plain `&&` would short-circuit and hide the sqruff half behind a squawk - # failure, costing a second red cycle). Fail-closed: non-zero from either. - command: 'bash -c "squawk go/internal/store/migrations/*.sql; rc=$?; sqruff lint go/internal/store/migrations/*.sql; rc2=$?; exit $(( rc | rc2 ))"' + # Both linters over every first-party migration, exit codes OR'd fail-closed + # in index.ts. Run from the workspace root so squawk/sqruff discover their + # repo-root configs (/.squawk.toml, /.sqruff) and the migration glob resolves + # repo-relative (GATE_ROOT defaults to the git toplevel = workspace root). + command: 'bun run tools/sql-migration-gate/index.ts' + deps: ['install'] options: runFromWorkspaceRoot: true # Never cache: a fail-closed lint gate must not ride a cached green from @@ -68,8 +85,9 @@ tasks: - '/go/internal/store/migrations/**/*.sql' - '/.squawk.toml' - '/.sqruff' + - 'index.ts' ci: - deps: ['check'] + deps: ['typecheck', 'test', 'check'] options: cache: false runInCI: true diff --git a/tools/sql-migration-gate/package.json b/tools/sql-migration-gate/package.json new file mode 100644 index 000000000..64a7b57e7 --- /dev/null +++ b/tools/sql-migration-gate/package.json @@ -0,0 +1,14 @@ +{ + "name": "@compass/sql-migration-gate", + "private": true, + "type": "module", + "description": "CI gate (RIG-3031): squawk (migration-safety) + sqruff (SQL style) over the first-party migrations under go/internal/store/migrations/, with the two linters' exit codes combined fail-closed so a finding from either reds the gate.", + "module": "index.ts", + "bin": { + "sql-migration-gate": "./index.ts" + }, + "devDependencies": { + "@types/bun": "catalog:", + "typescript": "catalog:" + } +} diff --git a/tools/sql-migration-gate/tsconfig.json b/tools/sql-migration-gate/tsconfig.json new file mode 100644 index 000000000..d40cc9e50 --- /dev/null +++ b/tools/sql-migration-gate/tsconfig.json @@ -0,0 +1,11 @@ +{ + "extends": "../../tsconfig.base.json", + "compilerOptions": { + "lib": ["ES2022"], + "moduleDetection": "force", + "allowJs": true, + "allowImportingTsExtensions": true, + "noUncheckedIndexedAccess": true, + "types": ["bun"] + } +}