[BigSimmo]

User description

Requested by Josh · project thread

Summary

Before: nothing signed in as two different users and checked the database's privacy rules by actually querying. The four SQL behaviour tests in tests/sql/ were never run by CI, and one of them (enrichment-repair.sql) had gone stale and failed.

After: tests/sql/two-user-privacy.sql signs in as two synthetic doctors and proves, by querying, that:

• The browser roles (anon, authenticated) hold no privilege on any public table, view or sequence, and every read, insert and delete each role attempts on every table is refused. Work mode data (roster, on call, CPD, teaching, admin) is reachable only through server routes.
• Only five reviewed functions are callable by a signed-in user, none by anon, and the four administrator ones refuse an ordinary doctor.
• Every owner policy written for the browser roles (24 tables plus storage objects) isolates the two doctors when a grant is simulated: each sees only their own rows, cannot change or delete the other's, and cannot move a row into the other's name. A new browser-facing policy without coverage fails the test.

All five tests/sql files now run in the Migration replay CI job against the full migration chain, and locally with npm run test:sql (bare-image replay of schema.sql, Docker only, no provider). tests/sql/** now selects the database CI scope.

No migration and no live database change. No policy or grant changed; no gap was found that needed one.

Areas touched: Data platform and access (test, runner), Delivery (CI step, scope pattern, npm script), Source intake (fixture repair only).

Verification

• npm run test:sql: All 5 SQL behaviour tests passed. including TWO_USER_PRIVACY_SQL_PASS.
• Same CI loop run locally against a Supabase CLI 2.108.0 emulator after supabase migration up --local (309 migrations): all five files printed their pass line.
• Planted-leak checks (each run then rolled back): granting a Work table to authenticated, granting a function to anon or authenticated, opening the documents, chunks or storage read policy, and adding an uncovered policy were each caught with a PRIVACY LEAK or coverage error.
• npm run test: Test Files 1882 passed (1882), Tests 29901 passed | 5 skipped.
• typecheck, check:migration-role, check:gate-manifest, check:function-grants, check:owner-scope, check:diff-integrity, check:verification-plan passed; eslint and prettier clean on changed files.
• The new CI step only runs on non-draft PRs, so its first hosted run happens when this is marked ready.

Risk and rollout

• Risk: CI only. If the emulator differs from the local replay the new step fails red rather than passing silently.
• Rollback: revert the commit.
• Provider or production effects: None.

Notes

• The real line between doctors is the server routes, which use the service-role key with app-side owner filters. That layer is covered by check:owner-scope and the nightly staging cross-tenant harness, not by this test.
• src/lib/supabase/admin.ts still lacks a server-only guard. Not added here because the ingestion worker and scripts import it in plain Node, where server-only throws; it needs its own small change.

🤖 Generated with Claude Code

https://claude.ai/code/session_01KhogQUCHgdtFBHFtpjnd1n

---

Generated by Claude Code

---

Summary by cubic

Adds a two-user database privacy test that signs in as two synthetic doctors and proves the privacy rules by querying, and starts running the SQL behaviour tests in CI.

Previously the four SQL behaviour tests in tests/sql/ never ran in CI, and enrichment-repair.sql had gone stale and failed. Now all five run in the Migration replay CI job and locally via npm run test:sql (a disposable Docker replay of schema.sql, never the live database), and tests/sql/** selects the database CI scope. The test proves browser roles hold no privilege on any public relation, that only five reviewed functions are callable by a signed-in user (four administrator ones refuse an ordinary doctor), and that every owner policy, including PUBLIC (no TO clause) ones, isolates the two doctors when a grant is simulated. A new browser-facing policy without coverage fails the test.

Hardening

• Repairs enrichment-repair.sql fixtures for the newer lease-tuple checks, and makes roster-behaviour.sql roll back its fixtures even on a direct psql -f run.
• Cross-owner inserts are probed on every covered table by cloning the actor's own row and pointing it at the other doctor; storage inserts are probed once per bucket, so a permissive policy scoped to one bucket is caught.
• Function signatures are checked exactly so a new overload is caught, and the administrator-refusal helper cannot mask a leak.
• Records a known gap: three policies check only owner_id, not parent document ownership, so the probe skips those; the test fails if the gap list drifts in either direction.

No migration or live database change; no policy or grant changed.

^{Written for commit 09bbbad. Summary will update on new commits.}

---

CodeAnt-AI Description

Run two-user database privacy checks in CI and locally

What Changed

• CI now runs every SQL behavior test against the replayed migration chain.
• A new two-user test checks that browser roles cannot access public tables directly, administrator functions reject ordinary doctors, and owner policies keep each doctor’s rows private.
• A local command runs the SQL tests against a disposable database using synthetic data; it does not contact the live database.
• Updated repair-test fixtures to include the lease details required by current database rules.

Impact

✅ Cross-user privacy regressions caught in CI
✅ SQL behavior failures block migration replay checks
✅ Local database checks avoid live data

💡 Usage Guide

Checking Your Pull Request

Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.

Talking to CodeAnt AI

Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:

@codeant-ai ask: Your question here

This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.

Example

@codeant-ai ask: Can you suggest a safer alternative to storing this secret?

Preserve Org Learnings with CodeAnt

You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:

@codeant-ai: Your feedback here

This helps CodeAnt AI learn and adapt to your team's coding style and standards.

Example

@codeant-ai: Do not flag unused imports.

Retrigger review

Ask CodeAnt AI to review the PR again, by typing:

@codeant-ai: review

Check Your Repository Health

To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.

Summary by CodeRabbit

• Tests
  • Added automated database behavior checks that run against a locally prepared database and report failures.
  • Added privacy checks covering access permissions and whether users can view or change one another’s records.
  • Database test changes are now included in change-scope detection, and the checks run during migration replay in CI.
  • Updated repair test data to include lease details required for processing rows.

[supabase]

This pull request has been ignored for the connected project sjrfecxgysukkwxsowpy because there are no changes detected in supabase directory. You can change this behaviour in Project Integrations Settings ↗︎.

---

Preview Branches by Supabase.
Learn more about Supabase Branching ↗︎.

[coderabbitai]

Warning

Review limit reached

Your organization has reached its usage spending cap. Adjust your spending cap in the billing tab.

Next included review available in 17 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available. Your 86 included PR review attempts over the past 7 days set your current allowance at 1 review per hour.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

• Configuration used: Repository: BigSimmo/PsychSift/.coderabbit.yaml
• Review profile: CHILL
• Plan: Team
• Run ID: 73c1e24d-169f-4a2b-8d52-33756ef37ab5

📥 Commits

Reviewing files that changed from the base of the PR and between a5d4ee9 and 09bbbad.

📒 Files selected for processing: 8

• .github/workflows/ci.yml
• docs/organisation/systems/data-platform.json
• package.json
• scripts/ci-change-scope.mjs
• scripts/test-sql-behaviour.ts
• tests/sql/enrichment-repair.sql
• tests/sql/roster-behaviour.sql
• tests/sql/two-user-privacy.sql

✨ Finishing Touches: 💡 1

🛠️ Fix failing CI checks: 💡

• Commit to this branch
• Create a new PR

📝 Generate docstrings

• Commit to this branch
• Create a new PR

🧪 Generate unit tests (beta)

• Commit to this branch
• Create a new PR

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

_{Comment @coderabbitai help to get the list of available commands.}

[codeant-ai]

🏁 CodeAnt Quality Gate Results

Commit: 09bbbade
Scan Time: 2026-10-15 16:44:46 UTC

✅ Overall Status: PASSED

Quality Gate Details

Quality Gate	Status	Details
Secrets	✅ PASSED	0 secrets found
Duplicate Code	✅ PASSED	0.0% duplicated
SAST	✅ PASSED	No security issues
Bugs	✅ PASSED	Rating S: No bugs
IAC	✅ PASSED	No IAC issues

View Full Results

[gitar-bot]

Gitar has auto-approved this PR (configure)

[greptile-apps]

BigSimmo has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.

[codeant-ai]

🤖 CodeAnt AI — Review Status

Status	Commit	Started (UTC)	Finished (UTC)
🔄 Reviewing your PR…	46ef4ea	Oct 15, 2026 · 15:24	—

[codeant-ai]

---

Thanks for using CodeAnt! 🎉

We're free for open-source projects. if you're enjoying it, help us grow by sharing.

Share on X ·
Reddit ·
LinkedIn

[chatgpt-codex-connector]

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review	Status	Commit	Review trigger
📝 Code Review	✅ Completed 2026-10-15T15:33:26.547155Z	46ef4ea	Draft marked ready

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

[qodo-code-review]

PR Summary by Qodo

Test two-user database privacy and run SQL behaviour tests in CI

🧪 Tests ⚙️ Configuration changes 🕐 40+ Minutes

AI Description

• Add two-user SQL tests for browser privileges, function access, and owner-policy isolation.
• Run all SQL behaviour tests locally and against the full migration chain in CI.
• Repair stale enrichment fixtures so the existing SQL test passes current lease constraints.

Diagram

graph TD
  Local["Local command"] --> Runner["SQL runner"] --> Scratch[("Scratch database")] --> Suite["SQL test suite"] --> Checks["Privacy assertions"]
  CI["Migration replay CI"] --> Emulator[("Replayed database")] --> Suite

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Use the Supabase CLI for local tests

• ➕ Local and CI tests would run against the same migration-chain setup.
• ➖ Local runs would require the heavier emulator and CLI instead of a disposable bare-image replay.

2. Share the drift replay bootstrap

• ➕ Would reduce duplicated Docker setup between the SQL runner and drift-manifest tooling.
• ➖ Requires refactoring an existing tool and broadens the scope of this test-focused PR.

Recommendation: Keep the separate local runner and CI migration-chain check: they make the tests easy to run locally while checking the deployed migration path in CI. Consider extracting shared replay setup if the two Docker runners need to evolve together.

Files changed (7) +588 / -4

Tests (2) +343 / -4

enrichment-repair.sqlBring enrichment fixtures into line with lease constraints +14/-4

Bring enrichment fixtures into line with lease constraints

• Adds the required lease tuple to synthetic processing-job rows so the existing enrichment-repair test can run against the current schema.

tests/sql/enrichment-repair.sql

two-user-privacy.sqlVerify browser access and two-user policy isolation +329/-0

Verify browser access and two-user policy isolation

• Tests that browser roles cannot access public relations or unapproved functions, and that administrator functions refuse an ordinary doctor. With simulated grants, it exercises browser-facing policies using two synthetic users and requires newly covered policy tables to be added to the test; all fixtures and grants roll back.

tests/sql/two-user-privacy.sql

Other (5) +245 / -0

ci.ymlRun SQL behaviour tests after migration replay +16/-0

Run SQL behaviour tests after migration replay

• Adds a CI step that executes every tests/sql SQL file against the replayed Supabase database, failing the job on a SQL error.

.github/workflows/ci.yml

data-platform.jsonAssign the SQL runner to Data platform +1/-0

Assign the SQL runner to Data platform

• Registers the new SQL behaviour runner in the Data platform system inventory.

docs/organisation/systems/data-platform.json

package.jsonExpose the local SQL test command +1/-0

Expose the local SQL test command

• Adds npm run test:sql as the entry point for the disposable-database SQL runner.

package.json

ci-change-scope.mjsInclude SQL tests in the database CI scope +1/-0

Include SQL tests in the database CI scope

• Classifies changes under tests/sql as database changes so the relevant CI checks are selected.

scripts/ci-change-scope.mjs

test-sql-behaviour.tsAdd a disposable-database SQL test runner +226/-0

Add a disposable-database SQL test runner

• Starts a worktree-owned, digest-pinned Postgres container, replays roles and schema, and runs selected or all SQL behaviour tests. It supplies the missing auth.jwt() definition for the bare image, reports per-file results, and removes the container unless asked to keep it.

scripts/test-sql-behaviour.ts

[claude]

Claude encountered an error after 2s —— View job

---

I'll analyze this and get back to you.

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

No findings for this group

No findings for this group

No findings for this group

Resolved findings

1. One storage bucket escapes checks ✓ Resolved

Description

check_isolation selects just one owned storage.objects row for its INSERT probe and leaves that
row's bucket_id unchanged. Because the fixtures contain objects in two buckets, a cross-owner
INSERT policy affecting only the unselected bucket can pass this test.

Code

tests/sql/two-user-privacy.sql[306]

+    execute format('select to_jsonb(t) from %s t where %s = %L::uuid limit 1', r.rel, v_expr, v_me) into v_row;

Relevance

●●● Strong

Accepted history favors strengthening tests against vacuous or incomplete fixture coverage; this
probe can miss bucket-specific policies.

PR-#2202
PR-#3485
PR-#2187

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The testing-review skill calls for tenant-isolation safety assertions, and the access skill calls
for cross-tenant denial proof. The fixture creates objects in both buckets, but the new probe takes
one unspecified row, changes its name and owner without changing its bucket, and makes only one
INSERT attempt per table.

tests/sql/two-user-privacy.sql[210-211]
tests/sql/two-user-privacy.sql[306-310]
tests/sql/two-user-privacy.sql[317-328]
Skill: testing-review
Skill: access

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The cross-owner storage INSERT probe chooses one fixture row without selecting a bucket, so it cannot reliably catch a permissive policy limited to the other bucket.
## Fix Focus Areas
- tests/sql/two-user-privacy.sql[301-328]
## Recommended Fix
For storage.objects, attempt the cross-owner INSERT separately with a fixture from each configured bucket. Preserve the existing denial assertion for each attempt and use distinct planted names.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. Administrator bypasses pass the test ✓ Resolved

Description

expect_admin_refusal catches its own PRIVACY LEAK exception because the message contains
administrator, which its handler accepts as a refusal. If any of the four administrator calls
succeeds for an ordinary doctor, the helper reports success instead of failing the privacy test.

Code

tests/sql/two-user-privacy.sql[R147-150]

+  raise exception 'PRIVACY LEAK: an ordinary doctor ran an administrator function: %', p_sql;
+exception when others then
+  if sqlerrm not like '%administrator%' then
+    raise exception 'expected an administrator refusal, got "%" from %', sqlerrm, p_sql;

Relevance

●●● Strong

The handler can swallow its own failure, a concrete false-positive privacy test; team accepts
stronger executable SQL assertions.

PR-#2187
PR-#3535

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The helper raises a success-path exception containing administrator, then its WHEN OTHERS
handler uses that same substring to recognize an expected refusal. Because the tested functions also
issue administrator-related refusal messages, the check cannot distinguish their exceptions from the
helper’s own assertion, defeating the non-admin denial proof.

tests/sql/two-user-privacy.sql[145-159]
supabase/schema.sql[30079-30092]
supabase/schema.sql[15075-15084]
Skill: access

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The administrator-refusal helper swallows its own failure assertion when a protected call succeeds, allowing an authorization bypass to pass the test.

## Fix Focus Areas
- tests/sql/two-user-privacy.sql[145-153]

## Recommended Fix
Catch and validate only exceptions from `execute p_sql`. Raise the privacy-leak assertion after that exception block if the call returns successfully, and verify that a deliberately successful call fails the test.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

3. Cross-owner inserts go untested ✓ Resolved

Description

check_isolation simulates an INSERT grant but probes planting data only by updating an existing
row’s owner column, leaving separate INSERT ownership checks untested. If an INSERT policy accepts a
new row owned by the other doctor, neither doctor’s checks exercise that path, even though the
simulated grant permits it.

Code

tests/sql/two-user-privacy.sql[R292-295]

+    -- Handing your own row to the other doctor (planting data in their name) must be refused.
+    if r.owner_col is not null and r.rel <> 'public.rag_aliases'::regclass then
+      begin
+        execute format('update %s set %I = %L::uuid where %I = %L::uuid', r.rel, r.owner_col, v_other, r.owner_col, v_me);

Relevance

●●● Strong

UPDATE reassignment does not test INSERT WITH CHECK; repository precedent accepts findings exposing
untested behavior paths.

PR-#2460
PR-#2413

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The simulated grant includes INSERT, but the isolation loop performs only reads, updates, and
deletes. The cross-tenant denial test therefore cannot verify the distinct INSERT WITH CHECK
restrictions on labels and user-owned data; an UPDATE reassignment probe does not exercise those
policies.

tests/sql/two-user-privacy.sql[250-255]
tests/sql/two-user-privacy.sql[266-304]
supabase/schema.sql[5368-5382]
supabase/migrations/20260719064735_user_account_data_and_admin_uploads.sql[32-52]
Skill: access

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The two-user isolation test does not exercise INSERT ownership checks despite simulating an INSERT grant. Its UPDATE reassignment probe cannot verify whether a doctor can plant a new row under the other doctor’s owner ID.

## Fix Focus Areas
- tests/sql/two-user-privacy.sql[250-255]
- tests/sql/two-user-privacy.sql[266-304]

## Recommended Fix
Add valid per-table INSERT fixtures for the relevant relations with browser insert policies. As each doctor, attempt to insert a row owned by the other doctor and assert that the attempt is refused or inserts no rows, without treating unrelated fixture or constraint errors as a successful denial. Verify that a deliberately permissive INSERT policy makes the test fail.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

View resolved (2)

4. Foreign storage edits escape the test ✓ Resolved

Description

check_isolation substitutes document_id for a missing owner column in its update probe, then
ignores undefined_column. storage.objects has no document_id, so its foreign-row update probe
always takes that ignored-error path, even if a policy permits the edit.

Code

tests/sql/two-user-privacy.sql[R279-283]

+      execute format('update %s set %s = %s where %s = %L::uuid',
+        r.rel, coalesce(r.owner_col, 'document_id'), coalesce(r.owner_col, 'document_id'), v_expr, v_other);
+      get diagnostics v_rows = row_count;
+      if v_rows > 0 then raise exception 'PRIVACY LEAK: doctor % changed % of doctor %''s rows in %', p_me, v_rows, p_other, r.rel; end if;
+    exception when insufficient_privilege or undefined_column then null;

Relevance

●●● Strong

Ignoring undefined-column errors makes storage coverage vacuous; similar absent-element and fallback
gaps were accepted.

PR-#2413
PR-#2460

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The storage entry has no owner column in owner_rule; the fallback therefore generates an update of
document_id. The scratch storage table's column list confirms that column does not exist, and the
test suppresses the resulting error.

tests/sql/two-user-privacy.sql[229-232]
tests/sql/two-user-privacy.sql[277-284]
scripts/sql/drift-replay-scaffold.sql[37-49]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The storage-object update probe references a nonexistent column and silently skips its check.
## Fix Focus Areas
- tests/sql/two-user-privacy.sql[207-232]
- tests/sql/two-user-privacy.sql[277-284]
## Recommended Fix
Give each relation a valid non-owner column for the update probe, including storage objects, and do not suppress `undefined_column` errors from the probe.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

5. Roster fixtures survive the test ✓ Resolved

Description

test:sql and CI run roster-behaviour.sql with psql -1, but the file ends without a ROLLBACK,
so its synthetic users and roster rows are committed. Subsequent SQL tests and CI steps reuse those
fixtures instead of starting from the replayed schema alone, and selecting the roster file twice
through --files runs it against its earlier fixtures.

Code

scripts/test-sql-behaviour.ts[R196-198]

+    for (const test of tests) {
+      try {
+        const output = psql("postgres", readFileSync(repoUrl(test), "utf8"), true);

Relevance

●●● Strong

Shared committed fixtures create order-dependent SQL tests; recent history accepts safeguards
against false-passing test coverage.

PR-#3535
PR-#2460

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

Both the local runner and the CI loop invoke SQL files with -1, which commits a file that finishes
without an explicit rollback. The roster test inserts users and roster data near its start, then
ends with a result query rather than a ROLLBACK; CI also runs the files against a shared replay
database.

scripts/test-sql-behaviour.ts[126-146]
scripts/test-sql-behaviour.ts[196-198]
tests/sql/roster-behaviour.sql[7-18]
tests/sql/roster-behaviour.sql[534-545]
.github/workflows/ci.yml[1504-1518]
tests/sql/roster-behaviour.sql[1-19]
tests/sql/roster-behaviour.sql[537-545]
scripts/test-sql-behaviour.ts[196-199]
Skill: testing-review

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The roster behavior test commits its synthetic fixtures when the local runner or CI executes it with psql's single-transaction option, leaving the shared replay database changed for later runs.

## Fix Focus Areas
- scripts/test-sql-behaviour.ts[196-198]
- .github/workflows/ci.yml[1513-1516]
- tests/sql/roster-behaviour.sql[1-8]
- tests/sql/roster-behaviour.sql[537-545]

## Recommended Fix
Make the roster test explicitly begin and roll back its fixture transaction, or run each file against a separately reset disposable database. Verify that local and CI execution leave the replay database unchanged after each file, including when `--files` selects the roster file twice.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

Context sources

✅ Compliance rules (platform): 9 rules

✅ Skills: 8 invoked

  testing-review

  supabase-postgres-best-practices

  supabase

  access

  review

  test

  security-review

  privacy

✅ Cross-repo context — repo relationships

Review mode: Auto: ⚖️ Balanced: Runtime timeout and interaction behavior changes carry meaningful operational and UX risk.

Tip of the day

💡 Did you know, you can hide the parts of a finding you never read, like the evidence or the agent prompt

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

Previous reviews

Review updated until commit 09bbbad ⚖️ Balanced

Results up to commit 46ef4ea ⚖️ Balanced

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

No findings for this group

No findings for this group

No findings for this group

Resolved findings

1. Administrator bypasses pass the test ✓ Resolved

Description

expect_admin_refusal catches its own PRIVACY LEAK exception because the message contains
administrator, which its handler accepts as a refusal. If any of the four administrator calls
succeeds for an ordinary doctor, the helper reports success instead of failing the privacy test.

Code

tests/sql/two-user-privacy.sql[R147-150]

+  raise exception 'PRIVACY LEAK: an ordinary doctor ran an administrator function: %', p_sql;
+exception when others then
+  if sqlerrm not like '%administrator%' then
+    raise exception 'expected an administrator refusal, got "%" from %', sqlerrm, p_sql;

Relevance

●●● Strong

The handler can swallow its own failure, a concrete false-positive privacy test; team accepts
stronger executable SQL assertions.

PR-#2187
PR-#3535

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The helper raises a success-path exception containing administrator, then its WHEN OTHERS
handler uses that same substring to recognize an expected refusal. Because the tested functions also
issue administrator-related refusal messages, the check cannot distinguish their exceptions from the
helper’s own assertion, defeating the non-admin denial proof.

tests/sql/two-user-privacy.sql[145-159]
supabase/schema.sql[30079-30092]
supabase/schema.sql[15075-15084]
Skill: access

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The administrator-refusal helper swallows its own failure assertion when a protected call succeeds, allowing an authorization bypass to pass the test.

## Fix Focus Areas
- tests/sql/two-user-privacy.sql[145-153]

## Recommended Fix
Catch and validate only exceptions from `execute p_sql`. Raise the privacy-leak assertion after that exception block if the call returns successfully, and verify that a deliberately successful call fails the test.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. Cross-owner inserts go untested ✓ Resolved

Description

check_isolation simulates an INSERT grant but probes planting data only by updating an existing
row’s owner column, leaving separate INSERT ownership checks untested. If an INSERT policy accepts a
new row owned by the other doctor, neither doctor’s checks exercise that path, even though the
simulated grant permits it.

Code

tests/sql/two-user-privacy.sql[R292-295]

+    -- Handing your own row to the other doctor (planting data in their name) must be refused.
+    if r.owner_col is not null and r.rel <> 'public.rag_aliases'::regclass then
+      begin
+        execute format('update %s set %I = %L::uuid where %I = %L::uuid', r.rel, r.owner_col, v_other, r.owner_col, v_me);

Relevance

●●● Strong

UPDATE reassignment does not test INSERT WITH CHECK; repository precedent accepts findings exposing
untested behavior paths.

PR-#2460
PR-#2413

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The simulated grant includes INSERT, but the isolation loop performs only reads, updates, and
deletes. The cross-tenant denial test therefore cannot verify the distinct INSERT WITH CHECK
restrictions on labels and user-owned data; an UPDATE reassignment probe does not exercise those
policies.

tests/sql/two-user-privacy.sql[250-255]
tests/sql/two-user-privacy.sql[266-304]
supabase/schema.sql[5368-5382]
supabase/migrations/20260719064735_user_account_data_and_admin_uploads.sql[32-52]
Skill: access

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The two-user isolation test does not exercise INSERT ownership checks despite simulating an INSERT grant. Its UPDATE reassignment probe cannot verify whether a doctor can plant a new row under the other doctor’s owner ID.

## Fix Focus Areas
- tests/sql/two-user-privacy.sql[250-255]
- tests/sql/two-user-privacy.sql[266-304]

## Recommended Fix
Add valid per-table INSERT fixtures for the relevant relations with browser insert policies. As each doctor, attempt to insert a row owned by the other doctor and assert that the attempt is refused or inserts no rows, without treating unrelated fixture or constraint errors as a successful denial. Verify that a deliberately permissive INSERT policy makes the test fail.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

3. Foreign storage edits escape the test ✓ Resolved

Description

check_isolation substitutes document_id for a missing owner column in its update probe, then
ignores undefined_column. storage.objects has no document_id, so its foreign-row update probe
always takes that ignored-error path, even if a policy permits the edit.

Code

tests/sql/two-user-privacy.sql[R279-283]

+      execute format('update %s set %s = %s where %s = %L::uuid',
+        r.rel, coalesce(r.owner_col, 'document_id'), coalesce(r.owner_col, 'document_id'), v_expr, v_other);
+      get diagnostics v_rows = row_count;
+      if v_rows > 0 then raise exception 'PRIVACY LEAK: doctor % changed % of doctor %''s rows in %', p_me, v_rows, p_other, r.rel; end if;
+    exception when insufficient_privilege or undefined_column then null;

Relevance

●●● Strong

Ignoring undefined-column errors makes storage coverage vacuous; similar absent-element and fallback
gaps were accepted.

PR-#2413
PR-#2460

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The storage entry has no owner column in owner_rule; the fallback therefore generates an update of
document_id. The scratch storage table's column list confirms that column does not exist, and the
test suppresses the resulting error.

tests/sql/two-user-privacy.sql[229-232]
tests/sql/two-user-privacy.sql[277-284]
scripts/sql/drift-replay-scaffold.sql[37-49]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The storage-object update probe references a nonexistent column and silently skips its check.
## Fix Focus Areas
- tests/sql/two-user-privacy.sql[207-232]
- tests/sql/two-user-privacy.sql[277-284]
## Recommended Fix
Give each relation a valid non-owner column for the update probe, including storage objects, and do not suppress `undefined_column` errors from the probe.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

View resolved (1)

4. Roster fixtures survive the test ✓ Resolved

Description

test:sql and CI run roster-behaviour.sql with psql -1, but the file ends without a ROLLBACK,
so its synthetic users and roster rows are committed. Subsequent SQL tests and CI steps reuse those
fixtures instead of starting from the replayed schema alone, and selecting the roster file twice
through --files runs it against its earlier fixtures.

Code

scripts/test-sql-behaviour.ts[R196-198]

+    for (const test of tests) {
+      try {
+        const output = psql("postgres", readFileSync(repoUrl(test), "utf8"), true);

Relevance

●●● Strong

Shared committed fixtures create order-dependent SQL tests; recent history accepts safeguards
against false-passing test coverage.

PR-#3535
PR-#2460

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

Both the local runner and the CI loop invoke SQL files with -1, which commits a file that finishes
without an explicit rollback. The roster test inserts users and roster data near its start, then
ends with a result query rather than a ROLLBACK; CI also runs the files against a shared replay
database.

scripts/test-sql-behaviour.ts[126-146]
scripts/test-sql-behaviour.ts[196-198]
tests/sql/roster-behaviour.sql[7-18]
tests/sql/roster-behaviour.sql[534-545]
.github/workflows/ci.yml[1504-1518]
tests/sql/roster-behaviour.sql[1-19]
tests/sql/roster-behaviour.sql[537-545]
scripts/test-sql-behaviour.ts[196-199]
Skill: testing-review

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The roster behavior test commits its synthetic fixtures when the local runner or CI executes it with psql's single-transaction option, leaving the shared replay database changed for later runs.

## Fix Focus Areas
- scripts/test-sql-behaviour.ts[196-198]
- .github/workflows/ci.yml[1513-1516]
- tests/sql/roster-behaviour.sql[1-8]
- tests/sql/roster-behaviour.sql[537-545]

## Recommended Fix
Make the roster test explicitly begin and roll back its fixture transaction, or run each file against a separately reset disposable database. Verify that local and CI execution leave the replay database unchanged after each file, including when `--files` selects the roster file twice.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

Results up to commit 590a31a ⚖️ Balanced

No changes from previous review

Results up to commit 3811823 ⚖️ Balanced

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

No findings for this group

No findings for this group

No findings for this group

Resolved findings

1. One storage bucket escapes checks ✓ Resolved

Description

check_isolation selects just one owned storage.objects row for its INSERT probe and leaves that
row's bucket_id unchanged. Because the fixtures contain objects in two buckets, a cross-owner
INSERT policy affecting only the unselected bucket can pass this test.

Code

tests/sql/two-user-privacy.sql[306]

+    execute format('select to_jsonb(t) from %s t where %s = %L::uuid limit 1', r.rel, v_expr, v_me) into v_row;

Relevance

●●● Strong

Accepted history favors strengthening tests against vacuous or incomplete fixture coverage; this
probe can miss bucket-specific policies.

PR-#2202
PR-#3485
PR-#2187

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The testing-review skill calls for tenant-isolation safety assertions, and the access skill calls
for cross-tenant denial proof. The fixture creates objects in both buckets, but the new probe takes
one unspecified row, changes its name and owner without changing its bucket, and makes only one
INSERT attempt per table.

tests/sql/two-user-privacy.sql[210-211]
tests/sql/two-user-privacy.sql[306-310]
tests/sql/two-user-privacy.sql[317-328]
Skill: testing-review
Skill: access

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The cross-owner storage INSERT probe chooses one fixture row without selecting a bucket, so it cannot reliably catch a permissive policy limited to the other bucket.
## Fix Focus Areas
- tests/sql/two-user-privacy.sql[301-328]
## Recommended Fix
For storage.objects, attempt the cross-owner INSERT separately with a fixture from each configured bucket. Preserve the existing denial assertion for each attempt and use distinct planted names.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

Results up to commit a295ac3 ⚖️ Balanced

No changes from previous review

[qodo-code-review]

Qodo Fixer

✅ Committed (4) · ☑ Fixed (4)

Commits pushed directly to this PR — no separate fix PR opened.

Process — 4 fixed

• ☑ Fixed: Administrator bypasses pass the test
• ☑ Fixed: Cross-owner inserts go untested
• ☑ Fixed: Foreign storage edits escape the test
• ☑ Fixed: Roster fixtures survive the test

[gitar-bot]

Gitar has auto-approved this PR (configure)

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 46ef4ea98b

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[qodo-code-review]

Code review by qodo was updated up to the latest commit 590a31a

[qodo-code-review]

Code review by qodo was updated up to the latest commit 3811823

[gitar-bot]

Gitar has auto-approved this PR (configure)

[gitar-bot]

Gitar has auto-approved this PR (configure)

[qodo-code-review]

Code review by qodo was updated up to the latest commit a295ac3

[BigSimmo]

Unit coverage partition (1) failed on a295ac3 in tests/work-screens-my-day-earlier-alerts.dom.test.tsx (2 tests, e.g. "says the day in each row's spoken label"). Not this PR's: this PR only touches SQL tests and CI wiring. The run started at 16:00:41 UTC, which is 00:00:41 Perth, so a notification stamped "3 minutes ago" landed on the previous Perth day ("Yesterday" instead of "Today"). The test uses the real clock without pinning it, so it fails for any run in the first few minutes after Perth midnight. No fix exists yet; the proper fix is to pin Date.now (e.g. vi.setSystemTime to a fixed Perth midday) in that file, as its own small PR. Re-running the failed job once.

---

Generated by Claude Code

[gitar-bot]

Gitar has auto-approved this PR (configure)

[gitar-bot]

Code Review ✅ Approved 3 closed / 3 findings

🟡 Medium risk · Adding privacy probes to migration-replay CI could block database PRs if emulator behavior differs from the test assumptions.

Adds a two-user database privacy test that signs in as two synthetic doctors and checks privacy rules by querying, and runs all SQL behaviour tests in the Migration replay CI job. Insert probing on the other doctor's document is resolved. One open minor finding: DISTINCT ON (1) in tests/sql/two-user-privacy.sql is read as a column position, so non-storage tables probe every owned row instead of one, slowing the run as fixtures grow. Replacing the integer with a non-integer constant such as true, or restoring limit 1 for non-storage tables, would fix it.

✅ 3 closed

✅ Edge Case: Insert probe never tries own owner_id on the other doctor's document

📄 tests/sql/two-user-privacy.sql:296-310
The new cross-owner insert probe changes owner_col and document_id to v_other together (lines 302-303). On tables that carry both columns, a policy that checks only auth.uid() = owner_id therefore refuses the clone, and the test passes. It never tries the row a doctor could actually plant: owner_id = me with document_id pointing at the other doctor's document. Several real policies have this gap: labels owner manual insert on document_labels (with check (auth.uid() = owner_id and source = 'manual')), and document sections owner all and document memory cards owner all, which check only owner_id. No trigger or composite FK ties owner_id to the parent document's owner either, since document_id references documents(id) alone. So once the browser grant is simulated, doctor A can attach sections, memory cards or manual labels to doctor B's document, and server routes that read by document_id would serve that content. The test reports this as isolated, and the comment on lines 296-300 says the parent-document case is covered when it isn't. Fix: when the row has both an owner column and document_id, run a second probe that keeps owner_col = v_me and sets only document_id = v_other, and expect a row-level security refusal there too. Expect that probe to fail against the current policies, which then need a document-ownership check.

Closed: Edge Case: Coverage guard misses policies with no TO clause (PUBLIC)

📄 tests/sql/two-user-privacy.sql:235-244
The guard at lines 235-244 only counts a policy as browser-facing when polroles contains anon or authenticated. A create policy with no TO clause gets polroles = {0} (PUBLIC). That policy still applies to anon and authenticated, but unnest(p.polroles) never matches it, so the test passes without any two-user coverage for it. This breaks the PR's promise that "a new browser-facing policy without coverage fails the test". Every existing policy in schema.sql names its roles, so nothing is uncovered today. The gap is the next policy someone writes without a TO clause, which is easy to do, especially on storage.objects. Fix: also treat 0 (PUBLIC) as a browser role in both the exists check and the error-message aggregate.

Closed: Edge Case: DISTINCT ON (1) means column 1, so non-storage tables probe every row

📄 tests/sql/two-user-privacy.sql:318-321
For tables other than storage the new query becomes select distinct on (1) to_jsonb(t) ... order by 1. Postgres reads an integer constant in DISTINCT ON and ORDER BY as an output-column position, not as a constant. So 1 points at to_jsonb(t), and the query returns every distinct own row instead of one. The old code had limit 1, and the comment says only storage objects are probed more than once. Now every table runs both insert variants for every fixture row the doctor owns, so the run gets slower as fixtures grow and no longer does what the comment says. Use a non-integer constant such as true, or keep limit 1 for non-storage tables.

Verification

Rules No rules evaluated

Functional validation Not enabled

Set up

Auto-approval Approved

_{For more verification details, comment gitar display:verbose.}

Options

Auto-apply is off → Gitar will not commit updates to this branch.
Display: compact → Showing compact verification results.

Comment with these commands to change the behavior for this request:

Auto-apply	Compact
gitar auto-apply:on	gitar display:verbose

---

Important

Your trial ends in 6 days — upgrade now to keep code review, CI analysis, auto-apply, custom automations, and more.

_{Was this helpful? React with 👍 / 👎 | Gitar}

[qodo-code-review]

Code review by qodo was updated up to the latest commit 09bbbad