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
trunk-io Bot
pushed a commit
that referenced
this pull request
Sep 4, 2026
…06) (#830) * feat(store): RLS enforcement + per-transaction tenant scoping (RIG-3106) T2 of the frozen RIG-2861 managed-multitenancy record: the enforcement half of tenant isolation. T1 gave `accounts` a `tenant_id` and the bootstrap tenant; T2 propagates `tenant_id` onto every tenant-owned table, turns on Postgres RLS with FORCE, and threads a per-transaction `compass.tenant_id` GUC through the whole store so a request-path query reads and writes only its own tenant's rows. Migration `0002_rls.sql`: - Two cluster roles: `compass_app` (non-owner, non-BYPASSRLS — the request-path role the policies constrain) and `compass_system` (BYPASSRLS — the N5/OQ-4 cross-tenant background role). Idempotent, serialized by the migration lock. - 26 tenant-owned tables (bucket B account-FK-rooted + `accounts` + C2 `linear_agent_sessions`) gain `tenant_id`, backfilled via the FK chain to `accounts.tenant_id`, `ENABLE`+`FORCE ROW LEVEL SECURITY`, and a fail-closed scalar-subquery policy: `current_setting('compass.tenant_id', true) <> '' AND tenant_id = (SELECT current_setting(...))` as USING + WITH CHECK. - Forge-board tables (`issues`, `forge_repo_subscriptions`, `forge_artifact_cursors`) fold `tenant_id` INTO the coordinate unique key/PK so two tenants may watch the same coordinate without colliding. - N7 (RIG-2921): `account_handles` uniqueness becomes org-scoped (`(tenant_id, handle) WHERE owner_user_id IS NULL`, and the owner tier). - Bucket A (`tenants`, `tokens`, `agent_config_bundle`) stays RLS-exempt: token resolution establishes the tenant before any GUC exists, and the config bundle is a fleet singleton. Threading (`tenant_tx.go`): the sqlc `*db.Queries` binds to a `scopedDBTX` that arms `SET LOCAL ROLE` + `set_config('compass.tenant_id', <tenant>, true)` into one pgx batch ahead of every pool-path statement (single round-trip). Multi- statement methods use `beginTenantTx`, which arms once at BEGIN. `SET LOCAL` is transaction-scoped, so a transaction-mode pooler carries no leftover scope. The four N5 background loops (delivery-cursor sweep, deliver-ack + forge-notify-ack advance) run under `WithSystemRole` (the BYPASSRLS role, no GUC). Tests (`rls_pgtest_test.go`, pgtest): cross-tenant read=0 rows, cross-tenant write lands under the writer's tenant, unset-GUC fails closed, pooled-conn reuse carries no scope, non-owner role is RLS-scoped, forge-board two-tenants-same- coordinate, N7 @handle under two tenants, and a catalog check asserting ENABLE+FORCE on all 26 tables and the bucket-A exemption. Full store pgtest suite green. Refs RIG-3106 RIG-2861 Co-authored-by: Matt Wilkinson <matt@rigel.build> * fix(store): address T2 RLS review findings (RIG-3106) Review of PR #830 (0 high, 2 medium, 4 low). Additive commit atop the pushed T2 commit so the PR shows an interdiff. M1 (Matt-ruled): fold tenant_id into forge_authored_artifacts' PRIMARY KEY (tenant_id, forge_provider, forge_host, repo, kind, number), matching the sibling forge-board tables. Two tenants may now hold authorship at the same forge coordinate without an ON-CONFLICT collision against an RLS-invisible row — needed for the future case of pulling in externally-authored PRs for review. Updates the ON CONFLICT arbiter + regenerates sqlc; adds TestForgeAuthoredTwoTenantsSameCoordinate. M2 (Matt-ruled): rewrite TestPooledConnReuseCarriesNoScope to drive the PRODUCTION single-statement path (scopedDBTX's pgx SendBatch implicit-transaction) across a pinned physical-connection reuse boundary, instead of explicit Begin/Commit — the path whose transaction-mode-pooler safety the design requires. A real transaction-mode-pooler CI service is tracked as follow-up RIG-3138 (SET LOCAL is pooler-safe by construction; the specific pooler is a separable deployment choice). Low findings: - Document RecordOwedMention's INSERT..SELECT zero-row precondition (resolved-member caller; ON CONFLICT DO NOTHING is the intended idempotent re-record, so a rows-affected assertion would wrongly fail a replay). - Soften the 0002_rls.sql header: FK-rooted backfills are correct for real data; the account-less (LIMIT 1) backfills are correct only in the single-tenant case. - Make TestRLSCatalogEnabledAndForced self-auditing: enumerate every tenant_id-bearing table from the catalog and assert ENABLE+FORCE, so a future tenant-owned table that forgets RLS fails automatically rather than needing a hand-maintained list edit. Co-authored-by: Matt Wilkinson <matt@rigel.build> * fix(server): stamp tenant_id in backfilled-placement pgtest seed (RIG-3106) CI pgtest caught a real gap the store-only local run missed: T2 added tenant_id NOT NULL to agent_placements, and TestStartAgentSessionWithBackfilledPlacementRecordsSession seeds the migration-0004 backfill row shape via a raw owner-connection INSERT (execSQL) that bypasses the store's tenant-scoping, so the column DEFAULT never stamps tenant_id and the NOT NULL constraint fires (SQLSTATE 23502). The seed now resolves tenant_id from the agent's account FK (SELECT ... a.tenant_id FROM accounts a WHERE a.id = $1), exactly as the 0002_rls backfill does for account-rooted tables — keeping the test faithful to what a real backfilled pre-upgrade row looks like under RLS. Verified: full go/... pgtest suite green locally (all 7 pgtest packages: store, server, auth, board, comms, delivery, runnerhub). Co-authored-by: Matt Wilkinson <matt@rigel.build> * refactor(store): fold 0002_rls into 0001_init (RIG-3106) 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 --------- Co-authored-by: Matt Wilkinson <matt@rigel.build>
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