Skip to content

feat(store): RLS enforcement + per-transaction tenant scoping (RIG-3106) - #830

Open
rigel-mintaka wants to merge 4 commits into
mainfrom
compass-managed/rig-3106-rls-tenant-scoping
Open

feat(store): RLS enforcement + per-transaction tenant scoping (RIG-3106)#830
rigel-mintaka wants to merge 4 commits into
mainfrom
compass-managed/rig-3106-rls-tenant-scoping

Conversation

@rigel-mintaka

@rigel-mintaka rigel-mintaka commented Sep 1, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

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

Review outcome (formal review pass, second commit)

One adversarial review pass on the diff: 0 high, 2 medium, 4 low. No cross-tenant leak and no fail-open found — all tenant-owned tables carry ENABLE+FORCE+the fail-closed policy, every request-path query routes through the armed non-owner role, and the policy fails closed on unset/empty GUC. The second commit (fix(store): address T2 RLS review findings) resolves every finding:

  • M1 (Matt-ruled) — forge_authored_artifacts PK: folded tenant_id into the 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. New TestForgeAuthoredTwoTenantsSameCoordinate.
  • M2 (Matt-ruled) — pooler verification: rewrote 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. 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 — RecordOwedMention: documented the INSERT..SELECT zero-row precondition (resolved-member caller; the ON CONFLICT DO NOTHING is the intended idempotent re-record, so a rows-affected assertion would wrongly fail a replay).
  • Low — migration header: softened the backfill-correctness claim (FK-rooted backfills correct for real data; account-less LIMIT 1 backfills correct only in the single-tenant case).
  • Low — catalog test: made TestRLSCatalogEnabledAndForced self-auditing — it enumerates every tenant_id-bearing table from the catalog and asserts ENABLE+FORCE, so a future tenant-owned table that forgets RLS fails automatically rather than needing a hand-maintained list edit.
  • Low — messages.seq global sequence side channel: pre-existing, inherent to the shared monotonic seq the delivery cursor depends on; no T2 change. Noted for the tenancy record.

Full store pgtest suite green on the rebased tree.

@linear-code

linear-code Bot commented Sep 1, 2026

Copy link
Copy Markdown

RIG-3106

RIG-2861

@github-actions

github-actions Bot commented Sep 1, 2026 •
edited
Loading

Copy link
Copy Markdown

Compass engineering docs preview: https://compass-managed-rig-3106-rls.compass-eng-docs.pages.dev

Deployed from compass-managed/rig-3106-rls-tenant-scoping at 99de44e.

rigel-mintaka added a commit that referenced this pull request Sep 1, 2026
@rigel-mintaka @mattwilkinsonn
fix(store): address T2 RLS review findings (RIG-3106)
4ffa71c 
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>
@rigel-mintaka
rigel-mintaka force-pushed the compass-managed/rig-3106-rls-tenant-scoping branch from a8ae080 to 4ffa71c Compare September 1, 2026 04:20
@rigel-mintaka
rigel-mintaka marked this pull request as ready for review September 1, 2026 04:21
@rigel-mintaka rigel-mintaka mentioned this pull request Sep 3, 2026
Merged
trunk-io Bot pushed a commit that referenced this pull request Sep 3, 2026
@rigel-mintaka @mattwilkinsonn
fix(sql-migration-gate): make the gate fail-closed (RIG-3208) (#845)
e54f5dc 
* fix(sql-migration-gate): make the gate fail-closed (RIG-3208)

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>

* fix(sql-migration-gate): harden spawn failure + root resolution (RIG-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>

---------

Co-authored-by: Matt Wilkinson <matt@rigel.build>
rigel-mintaka and others added 4 commits September 3, 2026 00:54
@rigel-mintaka @mattwilkinsonn
feat(store): RLS enforcement + per-transaction tenant scoping (RIG-3106)
38143b9 
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>
@rigel-mintaka @mattwilkinsonn
fix(store): address T2 RLS review findings (RIG-3106)
68e915f 
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>
@rigel-mintaka @mattwilkinsonn
fix(server): stamp tenant_id in backfilled-placement pgtest seed (RIG…
a5ae9e4 
…-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>
@rigel-mintaka
refactor(store): fold 0002_rls into 0001_init (RIG-3106)
99de44e 
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
@rigel-mintaka
rigel-mintaka force-pushed the compass-managed/rig-3106-rls-tenant-scoping branch from 99e19be to 99de44e Compare September 3, 2026 05:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mattwilkinsonn mattwilkinsonn Awaiting requested review from mattwilkinsonn mattwilkinsonn is a code owner

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@rigel-mintaka