fix(sql-migration-gate): make the gate fail-closed (RIG-3208) - #845
Merged
trunk-io[bot] merged 2 commits intoSep 3, 2026
Merged
Conversation
The gate ran both linters via an inline moon command: command: 'bash -c "squawk …; rc=$?; sqruff …; rc2=$?; exit $(( rc | rc2 ))"' moon wraps every task command in its own bash -c "<command>". The command is itself bash -c "…$?…$(( rc | rc2 ))…" in double quotes, so the nested quotes collide: the outer (moon) shell expands $?, $rc, $rc2 and $(( rc | rc2 )) — all unset, so 0 — before the inner shell runs. The inner shell received a literal '…; rc=0; …; rc2=0; exit 0' and ran fail-OPEN: it printed every finding then unconditionally exited 0. The 'Fail-closed' comment was false; the gate never failed on anything. It went unnoticed because 0001_init.sql genuinely passes both linters. 0002_rls.sql (RIG-3106, PR #830) is the first migration to actually trip findings, and the gate swallowed them, reporting the moon (nix) job green. Rewrite the gate as a bun/TypeScript CLI matching the sibling inline-sql-gate: the exit-code combination is now real code (rule://scripts-ts-over-bash + the no-bash-gate CI task forbid this logic in bash) and unit-tested in index.test.ts, red-green on the exact regression (squawk clean + sqruff finds -> exit 1). moon.yml moves from language:nix/ci-group.nix to language:typescript/ci-group.bun; both linters are on PATH on every moon leg (ci.yml phase-two puts the devenv nixpkgs tools on PATH per running leg). Also drop the CP02 exclusion from .sqruff — it existed solely to suppress a false positive on 'USING GIN' — and write 0001_init.sql:311 as canonical lowercase 'USING gin', so all capitalisation rules CP01-CP05 stay live. Verified: gate passes on clean 0001, fails (exit 1) with 0002's findings present; 12 unit tests pass; typecheck + biome clean. Co-authored-by: Matt Wilkinson <matt@rigel.build>
|
😎 Merged successfully - details. |
|
Compass engineering docs preview: https://compass-managed-rig-3208-sql.compass-eng-docs.pages.dev Deployed from |
…3208) Address review findings on PR #845 (additive; the core fail-closed fix is unchanged): - Bun.spawn throws synchronously when a linter binary is missing (ENOENT), so a spawn failure escaped runLinter as an unhandled rejection (exit 1 + stack trace) instead of the documented code 2. Extract the spawn closure into an exported makeSpawnLinter() and wrap it in try/catch that maps a throw to a clean code-2 result, so the code-2-dominates safety path is real and the second linter still runs if the first is missing. - Root resolution fell back to an empty string when git reported no toplevel (which is exactly the case in a jj workspace, whose .git lives in the colocated clone). Fall back to process.cwd() instead of "" (moon runs the gate with runFromWorkspaceRoot:true, so cwd is the repo root in CI). - Guard the per-linter err() emission so a clean run no longer prints blank lines ahead of the OK verdict. - Tests: drive the real makeSpawnLinter for the empty-glob and missing-binary code-2 paths (previously only the pure combine was covered), and assert runOnce surfaces BOTH linters' outputs and emits no blank noise on a clean run. Co-authored-by: Matt Wilkinson <matt@rigel.build>
rigel-mintaka
marked this pull request as ready for review
September 3, 2026 03:32
mattwilkinsonn
approved these changes
Sep 3, 2026
Contributor
|
/trunk merge |
trunk-io
Bot
deleted the
compass-managed/rig-3208-sql-migration-gate-fail-closed
branch
September 3, 2026 04:35
rigel-mintaka
added a commit
that referenced
this pull request
Sep 3, 2026
The now-strict sql-migration-gate (RIG-3208, #845) surfaced 7 sqruff capitalisation findings + 34 squawk migration-safety warnings on `0002_rls.sql`. Those warnings all flag the ALTER/backfill/DROP-INDEX mechanics of an *incremental* migration against a live table — hazards that do not exist pre-live. Matt ruled: we have no incremental migrations yet, so fold 0002 back into the squashed 0001 (matching 0001's own documented 'fold each later migration in as it accretes' convention). Every tenant-owned table now declares `tenant_id TEXT NOT NULL DEFAULT current_setting('compass.tenant_id', TRUE)` inline; the forge-coordinate tables fold tenant_id INTO their PK/unique key; and a trailing RLS section turns on ENABLE + FORCE ROW LEVEL SECURITY with the frozen per-tenant `tenant_isolation` policy. The ALTER/backfill/DROP-INDEX statements (and their squawk warnings) are gone; the inline column defs are capitalisation-clean, so the strict gate passes with 0 issues. Verified: sqlc regen produces a byte-identical `internal/store/db` tree (the fold is schema-equivalent to 0001+0002); `moon run sql-migration-gate:check` → 0 issues; full `./...` pgtest with -race → all packages ok (RLS isolation intact). Spec-impact: none. Refs RIG-3106
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The gate ran both linters via an inline moon command:
command: 'bash -c "squawk …; rc=$?; sqruff …; rc2=$?; exit $(( rc | rc2 ))"'
moon wraps every task command in its own bash -c "". The command
is itself bash -c "…$?…$(( rc | rc2 ))…" in double quotes, so the nested
quotes collide: the outer (moon) shell expands $?, $rc, $rc2 and
$(( rc | rc2 )) — all unset, so 0 — before the inner shell runs. The inner
shell received a literal '…; rc=0; …; rc2=0; exit 0' and ran fail-OPEN: it
printed every finding then unconditionally exited 0. The 'Fail-closed'
comment was false; the gate never failed on anything.
It went unnoticed because 0001_init.sql genuinely passes both linters.
0002_rls.sql (RIG-3106, PR #830) is the first migration to actually trip
findings, and the gate swallowed them, reporting the moon (nix) job green.
Rewrite the gate as a bun/TypeScript CLI matching the sibling inline-sql-gate:
the exit-code combination is now real code (rule://scripts-ts-over-bash + the
no-bash-gate CI task forbid this logic in bash) and unit-tested in
index.test.ts, red-green on the exact regression (squawk clean + sqruff finds
-> exit 1). moon.yml moves from language:nix/ci-group.nix to
language:typescript/ci-group.bun; both linters are on PATH on every moon leg
(ci.yml phase-two puts the devenv nixpkgs tools on PATH per running leg).
Also drop the CP02 exclusion from .sqruff — it existed solely to suppress a
false positive on 'USING GIN' — and write 0001_init.sql:311 as canonical
lowercase 'USING gin', so all capitalisation rules CP01-CP05 stay live.
Verified: gate passes on clean 0001, fails (exit 1) with 0002's findings
present; 12 unit tests pass; typecheck + biome clean.
Co-authored-by: Matt Wilkinson matt@rigel.build