Skip to content

fix: guard upsert tenant scope and SQLite rebuild dependencies - #130

Open
artile wants to merge 2 commits into
MapleTechLabs:mainfrom
artile:fix/tenant-upsert-sqlite-rebuild
Open

artile wants to merge 2 commits into
MapleTechLabs:mainfrom
artile:fix/tenant-upsert-sqlite-rebuild

Conversation

@artile

@artile artile commented Oct 26, 2026 •

Copy link
Copy Markdown
Contributor

A tenant-pinned insert could update a different tenant through ON CONFLICT(id) DO UPDATE, while requireTenantScope accepted it as single-tenant. The compiler now proves the existing conflict row's tenant as well as the inserted values and tenant-changing SETs. A tenant-inclusive column target or an explicit tenant predicate can provide that proof; an unproved path is cross-tenant and the execution guard refuses it. crossTenant() remains an explicit opt-out.

A generated SQLite rebuild could fire an external foreign key's CASCADE or SET NULL action when its old table was dropped inside a transaction. Generated SQL now checks live inbound references before copying or dropping the table. A short-lived NOT NULL assertion refuses referenced tables and rolls back with the migration in a D1 batch. It also works with ignore_check_constraints enabled. The catalog prefilter avoids D1's protected FK-free metadata tables without excluding user tables by name.

Closes #127
Closes #128

Behavior and limits

  • Rebuilds refuse any inbound FK, including self-references and empty child tables. A tested custom migration must handle those dependencies. Ordinary rebuilds still run.
  • A guard-table name collision fails without reusing or dropping the pre-existing object. Apply the migration as one transaction/batch and roll back on failure.
  • The guard is in newly generated SQL. Review or regenerate older unapplied rebuilds. It does not preserve unmodelled triggers, views or other constraints.
  • Named conflict constraints alone do not prove their columns. Tables whose name resolves to excluded are conservatively cross-tenant for conflict updates.

Validation

  • Regressions failed before their fixes, then passed: compiler scope cases, actual guarded D1 execution and row state, and actual transaction-wrapped SQLite/D1 rebuilds.
  • D1 coverage includes refusal before dispatch, same-tenant updates, an explicit tenant equality preserving the other org, crossTenant(), ambiguous Excluded names, mixed-case FK parent names, CASCADE/SET NULL, metadata tables, rollback/retry, guard cleanup and ignore_check_constraints=ON.
  • bun run build, bun run typecheck, doc citations, export catalog, checked doc examples and isolated tarball/package consumer checks pass.
  • FUZZ_SEED=1 bunx vitest run src tests --maxWorkers=2 --testTimeout=60000 --hookTimeout=60000: 76 files, 1,493 tests passed, zero skipped, against local ClickHouse 26.8.2.7.
  • Node 24.18.0, Bun 1.4.0, Effect 4.0.6, local ClickHouse 26.8.2.7. D1 tests use real local Miniflare bindings; remote D1 was not run.

Existing query fuzz failure

The initial full run passed 1,492 tests and found one unrelated Postgres generated-fixture UNION failure: an empty fixture still evaluates a constant modulo/division-by-zero expression. I reproduced the identical seed and shrink path after restoring all tracked source to upstream b57faf18e45a3a3268e6d44a5be206bde2107891 and rebuilding. Then I restored/rebuilt this PR head; the worktree is clean. Exact baseline replay:

FUZZ_SEED=-1388060287 FUZZ_PATH=57:0:4:1:1:2:4:5:4:5:5:5:0:0:0:0:0:0:0:0:0:0 \
  bunx vitest run tests/fuzz/queries.fuzz.test.ts \
  -t 'generated fixtures: union results agree \(postgres\)' --maxWorkers=2

View with [code]smith Autofix with [code]smith
Need help on this PR? Tag @codesmith-bot with what you need.

Summary by CodeRabbit

  • Bug Fixes

    • Tenant-scoped upserts now require evidence that conflict updates stay within the same tenant; otherwise, they’re treated as cross-tenant and may be rejected.
    • SQLite table rebuilds now stop before modifying data when inbound foreign keys are detected, including references from the table itself.
  • Documentation

    • Updated guidance on tenant-scoped upserts and SQLite rebuild safety, including transactional migrations and reviewing or regenerating rebuilds created by older versions.

@coderabbitai

coderabbitai Bot commented Oct 26, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

No actionable comments were generated in the recent review.

Recent review info
Run configuration
  • Source mode: Auto
  • Repository: Unavailable
  • Branch: Unavailable
  • File: No configuration file from this repository applied
  • Commit: Unavailable
  • Layers: defaults
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: c81fd949-7073-4450-b948-9c7ebd894376
Commits

Reviewing files that changed from the base of the PR and between b57faf1 and 0f086e1.

Files selected for processing: 11
  • .changeset/safe-conflict-tenant.md
  • .changeset/safe-sqlite-rebuild.md
  • docs/inserts.md
  • docs/migrations.md
  • src/ch/compile.ts
  • src/ch/insert-tenant.test.ts
  • src/ch/insert.test.ts
  • src/kit/sqlite-generate.test.ts
  • src/schema/sqlite-ops.ts
  • src/schema/sqlite-rebuild-safety.test.ts
  • src/sqlite/d1.test.ts

Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.

Walkthrough

The PR adds tenant-scope checks for upsert conflict updates and a SQLite table-rebuild guard that rejects tables with live inbound foreign keys. It also adds regression tests, documentation, and patch changesets for both changes.

Changes: Tenant-Scoped Upserts

Layer / File(s) Summary
Conflict-row tenant proof
src/ch/compile.ts, src/ch/insert-tenant.test.ts, src/ch/insert.test.ts
The compiler now checks whether the conflict target or update predicates prove the existing row’s tenant. Tests cover tenant-matching and ambiguous or unsafe cases.
D1 execution and upsert contract
src/sqlite/d1.test.ts, docs/inserts.md, .changeset/safe-conflict-tenant.md
D1 tests check rejection before dispatch and the behavior of matching predicates and explicit cross-tenant writes. The documentation and changeset describe the upsert tenant-scope rules.

SQLite Rebuild Safety

Layer / File(s) Summary
Live-reference rebuild guard
src/schema/sqlite-ops.ts, src/kit/sqlite-generate.test.ts
Generated rebuild SQL checks the live catalog for inbound foreign keys before continuing. The generation test checks guard creation, the foreign-key check, and guard removal.
Rebuild validation and migration guidance
src/schema/sqlite-rebuild-safety.test.ts, docs/migrations.md, .changeset/safe-sqlite-rebuild.md
Tests check that referenced-table rebuilds fail without changing rows and that a later safe rebuild succeeds. The documentation describes transactional application, rollback on failure, and coverage limits.

Priority: Normal

Estimated code review effort: 3 (Moderate) | ~25 minutes

Change: Bug fix · Severity of issue fixed: Medium

Sequence Diagram(s)

sequenceDiagram
  participant InsertQuery
  participant Compiler
  participant requireTenantScope
  participant D1
  InsertQuery->>Compiler: compile conflict target and update predicates
  Compiler-->>InsertQuery: return classified tenant scope
  InsertQuery->>requireTenantScope: validate write scope
  alt write scope is accepted
    requireTenantScope->>D1: dispatch write
  else write scope is rejected
    requireTenantScope-->>InsertQuery: return TenantScopeViolation before dispatch
  end
Loading
sequenceDiagram
  participant RebuildSQL
  participant SQLiteCatalog
  participant RebuildGuard
  participant TableRebuild
  RebuildSQL->>RebuildGuard: create guard with NOT NULL column
  RebuildGuard->>SQLiteCatalog: inspect live inbound foreign keys
  alt inbound reference exists
    RebuildGuard->>RebuildGuard: insert NULL and fail the assertion
  else no inbound reference exists
    RebuildGuard->>RebuildGuard: insert NULL and drop guard
    RebuildSQL->>TableRebuild: continue rebuild statements
  end
Loading

Suggested reviewers: makisuo

Merge Risk: Minimal · up to 0f086

No actionable merge-blocking issue remains on the supplied evidence. The change is ready for normal merge checks.

Pre-merge checks: Passed 5
Check name Status Explanation
Description Check Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check Passed The title clearly and concisely identifies both main changes: tenant-scope protection for upserts and dependency guards for SQLite rebuilds.
Linked Issues check Passed The PR addresses both active coding requirements. For #127, src/ch/compile.ts proves the existing conflict row from a tenant-inclusive target or tenant predicates, marks unproved updates `cross-tena…
Out of Scope Changes check Passed The changed compiler logic, SQLite guard, documentation, changesets, and regression tests directly support #127 or #128. The test updates adjust expectations for the new generated guard and tenant pro…
Docstring Coverage Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 7 files. (4 skipped: 4 …
Full details: Linked Issues check

Explanation

The PR addresses both active coding requirements. For #127, src/ch/compile.ts proves the existing conflict row from a tenant-inclusive target or tenant predicates, marks unproved updates cross-tenant, and preserves crossTenant() as an opt-out. src/sqlite/d1.test.ts verifies refusal before dispatch, preservation of the other tenant, safe same-tenant updates, and the explicit opt-out. For #128, src/schema/sqlite-ops.ts adds a live inbound-FK guard before copy or drop. The guard detects case-insensitive references, including self-references and empty child tables, and fails before destructive statements. The reported tests cover CASCADE, SET NULL, rollback, guard collisions, D1 batches, and ignore_check_constraints behavior.

Full details: Out of Scope Changes check

Explanation

The changed compiler logic, SQLite guard, documentation, changesets, and regression tests directly support #127 or #128. The test updates adjust expectations for the new generated guard and tenant proof behavior. No unrelated product behavior or unrelated files are shown in the whole-PR summary.

Full details: Docstring Coverage

Explanation

Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 7 files. (4 skipped: 4 unsupported.)

Finishing Touches: 💡 1
🛠️ Fix failing CI checks: 💡
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

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

@artile

artile commented Oct 26, 2026

Copy link
Copy Markdown
Contributor Author

CI follow-up: the two ClickHouse failures are reproducible on clean upstream, without this PR.

I replayed the exact seeds and shrink paths from CI run 40382705371 against both:

  • clean upstream b57faf18e45a3a3268e6d44a5be206bde2107891 (all tracked source restored, then rebuilt);
  • PR head 0f086e156efaafc13647e48aab6a4de544153a92 (restored and rebuilt afterward).
ClickHouse CI seed Clean upstream PR head
26.8.2.7 -653094940 Division by zero Same failure
26.2.19.43 1164859907 Division by zero Same failure

For each version, the replay produced the same counterexample and byte-identical compiled SELECT SQL on both commits. The failure is in generated fixtures: union results agree (clickhouse-join-nulls).

The cause is the empty-fixture source: it uses a typed placeholder row with Id = 0, followed by WHERE 1 = 0. ClickHouse evaluates the modulo expression involving that zero during optimization and returns ILLEGAL_DIVISION, while the model expects [] because the logical fixture is empty.

As a separate diagnostic, I sent the exact SQL from each CI job directly to the matching ClickHouse server, without the ORM. It returned the same division-by-zero error. Changing only the empty fixture's placeholder ID from 0 to 1 returned the expected empty result on both versions. This identifies a fixture/oracle problem; that placeholder change alone is not a proposed complete generator fix.

The tenant-scope, local D1 and rebuild-safety regression suites passed in both CI jobs. Each job reports 1,444 passed tests and this one fuzz failure. The failing empty-fixture case also exists on upstream, so it was not introduced by #127/#128's fixes. CI remains red until that test-harness case is addressed.

Exact replay commands

Build the selected commit first with bun run build. Run each command against the indicated local ClickHouse version (and set the user/password variables if your server requires them).

ClickHouse 26.8.2.7:

EFFECT_ORM_CLICKHOUSE_URL=http://127.0.0.1:8123 \
FUZZ_SEED=-653094940 \
FUZZ_PATH=53:0:0:4:4:4:4:0:0:0:0:0:0:0:0:0:0:0:0:0:0:0:0:0:0:0:0:0:0:0:0:0:0:0:1:1:1:1:1:1:1:1:1:1:0:0:0:0:0:0:0:0:0:0:0:0:0:0 \
bunx vitest run tests/fuzz/queries.fuzz.test.ts \
  -t 'generated fixtures: union results agree [(]clickhouse-join-nulls[)]' \
  --maxWorkers=1 --testTimeout=60000 --hookTimeout=60000

ClickHouse 26.2.19.43:

EFFECT_ORM_CLICKHOUSE_URL=http://127.0.0.1:8123 \
FUZZ_SEED=1164859907 \
FUZZ_PATH=32:0:0:3:3:3:0:0:0:0:0:0:0:0:0:0:0:0:0:0:0:0:0:0:0:0:0:0:0:0:0:0:0:1:1:1:1:0:0:0:0:0:0:0 \
bunx vitest run tests/fuzz/queries.fuzz.test.ts \
  -t 'generated fixtures: union results agree [(]clickhouse-join-nulls[)]' \
  --maxWorkers=1 --testTimeout=60000 --hookTimeout=60000

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

1 participant