Skip to content

sys_setting's declared row identity is unenforced on every tenant and global row — user_id is NULL there and SQL UNIQUE is NULL-distinct #8629

Description

@os-zhuang

Found while implementing #8555 (which scopes this same index per organization). Filed unassigned and deliberately separate: #8555 is a pure relaxation, this one is a tightening, and the two need different migration ceremony. Same split #6417 made against #5839, and for the same stated reason.

The fact

packages/platform-objects/src/system/sys-setting.object.ts declares the object's row identity as

{ fields: ['namespace', 'key', 'scope', 'user_id'], unique: 'organization' }

and the object's own header calls that the row identity. user_id is NULL on every row that is not scope='user' — SettingsService.set computes it as scope === 'user' ? ctx.userId ?? null : null — and SQL UNIQUE treats NULLs as mutually distinct. So the constraint is void on the tenant and global limbs, which are exactly the ones carrying organization-level and platform-level configuration.

The organization key part is NULL-safe (COALESCE(organization_id, '__global__'), ADR-0120 D3). user_id is not, and D1's vocabulary scopes the tenant column only — sys_view_definition's declaration says so in its own comment, about its own owner column:

⚠️ The KEY below is NULL-DISTINCT, which is a second gap the declaration cannot close on its own (#6417) … it scopes the tenant column only — owner would stay NULL-distinct.

Measured, live, on a real engine

Real SqlDriver, the real shipped declaration, OS_TENANCY_POSTURE=isolated. Identical before and after #8555 — this is not something that card introduced or fixed:

scope='tenant', user_id NULL, SAME organization, same (namespace, key)
  org_jia POST (lifecycle, retention_overrides, tenant, NULL) → 201
  org_jia POST the SAME AGAIN                                 → 201    ← row identity void
  rows in table: 2

scope='global', user_id NULL
  platform POST (mail, smtp_host, global, NULL)               → 201
  platform POST the SAME AGAIN                                → 201    ← two competing platform defaults

control — the same rows with a NON-NULL user_id
  org_jia POST (lifecycle, retention_overrides, tenant, usr_1) → 201
  org_jia POST the SAME AGAIN                                  → 409 UNIQUE_VIOLATION

The control identifies the mechanism: it is the NULL, not the scope value.

Pinned as live assertions in packages/drivers/driver-sql/src/sql-driver-sys-setting-organization-unique.test.ts, section 4 ("the NULL-distinct user_id hole"), written to go red when this card lands so a fix must come and flip them rather than leaving a stale comment behind.

User-reachable consequence

SettingsService resolves a layer with rows.find((r) => r.key === key && r.scope === 'tenant') — a positional pick over whatever the table returns. With duplicates permitted, which value an organization gets for a tenant-scoped setting is unspecified, and a write through set() upserts against where { namespace, key, scope, user_id }, which matches an arbitrary one of them. Two rows can therefore disagree indefinitely with no way for an admin to see why the effective value is not the one they set. lifecycle.retention_overrides is a live tenant-scoped key, so this reaches real retention behaviour.

Sibling objects likely share the shape — sys_setting_audit mirrors the same three-value scope — but that is unverified and deliberately not asserted here.

Why it is not a rider on #8555

#8555 respells unique: true to 'organization'. That is a relaxation: it admits key pairs previously refused and refuses nothing that previously succeeded, so it cannot fail to apply on any existing database. Verified there against a database deliberately seeded with pre-existing duplicate tenant-scope rows — the replacement index builds and every row survives, precisely because it does not tighten user_id.

Making user_id NULL-safe is the opposite: it is a tightening that will fail to create the index on any installation that has already accumulated duplicates, which the void constraint has been permitting all along. That needs ADR-0120 D4's conflict-row disposition and a duplicate pre-flight in os migrate plan. Exactly the tradeoff #6417 recorded, verbatim and untranslated:

把键改成 NULL-safe 则相反:那是收紧,在已经存在重名活跃共享视图的库上会直接建不出索引……需要 ADR-0120 D4 的冲突行处置口径配套。这是独立的取舍,需要单独拍板。

Two routes, both with precedent

  1. Runtime migration, the [metadata-core] sys_view_definition:owner/organization_id 为 NULL 的视图完全不受唯一索引约束(两条同名活跃共享视图可并存) #6417 / PR fix(metadata): sys_view_definition active-row index gets a NULL-safe key — same-name active shared views can no longer coexist (#6417) #6666 route. metadata-protocol's ensureViewDefinitionActiveIndex issues a raw-SQL CREATE UNIQUE INDEX at kernel:ready, reusing the declared index's name so syncDeclaredIndexes (which skips by name) never re-imposes the NULL-distinct form on a later boot. ensureOverlayIndex does the same with COALESCE(package_id, '') for sys_metadata. Proven twice; leaves the declaration unable to say what it means.
  2. Extend the declared vocabulary so an author can mark a listed column NULL-safe. Closes the class rather than the instance, and would let the declaration state its own row identity — but it is authorable-surface design and needs its own ADR (or an ADR-0120 amendment), plus SqlDriver.createNullSafeUniqueIndex's existing MySQL functional-key fallback extended to it.

Route 2 is the ADR-0078 direction (a declaration that cannot express its own constraint is a declared-but-unenforced surface), but route 1 is what has shipped twice. Recording both rather than choosing.

Decision needed before implementation

For a duplicate-carrying installation, what happens to the extra rows — refuse to migrate and hand the operator a list, or apply a deterministic keep-one rule (newest updated_at)? Settings values are admin-authored configuration, so silently discarding one is a real data decision, not a cleanup.

Related

Activity

  1. added theissue type on Aug 14, 2026
  2. hotlong commented on Aug 14, 2026

    @hotlong
    Contributor

    Triage: routed needs-user-decision + domain:metadata (declaration lives in packages/platform-objects, route-1 fix would land in metadata-protocol), type Bug — declared row identity ≠ enforced on the tenant/global limbs.

    Premise check on origin/main (2026-08-14): sys-setting.object.ts:191 currently declares { fields: ['namespace','key','scope','user_id'], unique: true } — the card quotes the 'organization' spelling, which is on the in-flight #8555 branch, not yet on main. The NULL-distinct user_id hole is spelling-independent (the card's own control run shows the mechanism is the NULL, not the scope value), so the premise holds either way. The pinned section-4 assertions likewise live on the #8555 branch. Serial: implementation cannot start before #8555 lands (same declaration line, same index name).

    Why the human floor, not the #8468 inheritance: the settled installation-wide-unique family covers the relaxation shape (bare unique: true → per-organization). This card is the opposite move — a tightening that can fail to build the index on duplicate-carrying installations, i.e. ADR-0120 D4 ceremony plus a stored-data disposition; and route 2 is authorable-surface design (ADR-class). Both are human-floor categories, and the sibling ruling (#6417) itself recorded this half as an independent tradeoff needing its own ruling:「这是独立的取舍,需要单独拍板。」

    Four-prism block:

    Recommendation: route 1 now — kernel:ready NULL-safe unique index via COALESCE(user_id, ''), reusing the declared index name so syncDeclaredIndexes never re-imposes the NULL-distinct form; duplicate disposition = refuse-to-migrate with an operator-facing list in the os migrate plan pre-flight (settings are admin-authored; keep-one is a data decision an operator should make, not a default we pick); file the route-2 vocabulary card on the v18/ADR-0120 train so the class closes there.

    Two questions for the maintainer:

    1. Route 1 now with route 2 parked to v18 — yes, or go straight to the vocabulary/ADR route?
    2. Duplicate-row disposition on migration: refuse + operator-facing list (recommended), or deterministic keep-newest?

    Not auto-adjudicable (human floor: stored-data migration shape + ADR-class route choice). Prisms 1/3 lean route 2 long-term while prism 4 leans route 1 now — genuinely split on timing, which is itself above the confidence gate.


    Generated by Claude Code

  3. hotlong commented on Aug 14, 2026

    @hotlong
    Contributor

    Maintainer ruling (2026-08-14, verbatim: 「同意」, approving item 3 of the six-item ruling list: 「#8629 走 Route 1+拒绝迁移列清单?」): Route 1 now — runtime NULL-safe unique index issued at kernel:ready (precedent PR #6666); Route 2 (declaring NULL-safe uniqueness in the spec vocabulary) is deferred to v18 as the ADR-class long-term form. Migration semantics: refuse-to-migrate and list the duplicate rows for the operator — never keep-newest; silently discarding admin-authored config rows is a data-loss trade that is not worth saving one manual confirmation. Serial constraint noted at triage stands: implementation is blocked behind #8555's landing. needs-user-decision → pm:queue, target:v17 applied; Seat A (#8667).


    Generated by Claude Code

  4. self-assigned this
    on Aug 14, 2026
  5. hotlong commented on Aug 14, 2026

    @hotlong
    Contributor

    Claim: PM loop round 1
    Session: session_01XeQRiAa7vYRVX5Fog7Zby8
    Branch: claude/issue-8629-sys-setting-null-safe-unique
    Worktree: objectstack-issue-8629
    Domain: domain:metadata
    File surface: packages/metadata-protocol/src/migrations/** + its wiring in packages/metadata-protocol/src/index.ts; packages/drivers/driver-sql/src/sql-driver-sys-setting-organization-unique.test.ts section 4 only (stop on breach; explain in the report)
    Container & model: M, mode:subagent, model: opus
    Serial constraints cleared: #8555 has landed — 6cb81c7 on origin/main, unique: 'organization' verified at sys-setting.object.ts:235, and the pin file exists at the declared path. The triage serial («implementation cannot start before #8555 lands») is discharged. Of the 6 open PRs in this repo, none touches metadata-protocol/src/migrations or the sys-setting pin file. ⚠️ driver-sql is shared with #8622, which this seat is deliberately holding to the next batch for that reason — do not extend into sql-driver.ts or the upsert dialect tests.

    Dispatching under the maintainer's direct-dispatch instruction to this PM session (2026-08-14, verbatim, untranslated): 「metadata + engine-core + drivers 优先处理bug」. Lane seat #6367 is vacant — Seat A de-registered from all eight lane seats at 15:04Z per 「你只需要处理在手的几个任务,不需要再接管车道。」, so this card is unowned rather than contested.

    Implementing the maintainer's 2026-08-14 ruling recorded in 5293093894: Route 1 (runtime NULL-safe unique index at kernel:ready, precedent PR #6666), Route 2 deferred to v18; migration semantics refuse-to-migrate and list the duplicate rows for the operator, never keep-newest.

    ⚠️ Declared cross-lane risk, carried into the dispatch rather than discovered mid-run: the duplicate pre-flight belongs to os migrate plan, which lives in packages/cli (domain:cli). The dev is instructed to land the index in metadata-protocol and to stop and report if the operator-facing list forces an edit inside packages/cli — this seat will route that half rather than have it patched across a lane boundary.


    Generated by Claude Code

  6. hotlong commented on Aug 14, 2026

    @hotlong
    Contributor
    {
      "issue": 8629,
      "status": "done",
      "branch": "claude/issue-8629-sys-setting-null-safe-unique",
      "pr": "https://github.com/objectstack-ai/objectstack/pull/8719",
      "premise_still_valid": true,
      "summary": "Route 1 as ruled: a runtime NULL-safe UNIQUE index for sys_setting, issued at kernel:ready from packages/metadata-protocol/src/migrations/sys-setting-identity-index.ts, folding COALESCE(organization_id,'__global__') (ADR-0120 D3, unchanged from what the driver emits) and COALESCE(user_id,'') (the ensureOverlayIndex spelling for a non-tenant nullable discriminator), reusing the DECLARED index name so syncDeclaredIndexes never re-imposes the NULL-distinct form. Duplicate disposition is refuse-to-migrate: the existing probe-first order proves the form buildable under a throwaway name before the declared name is dropped, so a conflict keeps the previous index, deletes nothing, and reports at error with the exact row-listing query — asserted as behaviour, not comment. packages/spec untouched (route 2 is v18); packages/cli untouched (see open_questions). All three PM mechanism assumptions re-verified true on origin/main, plus four design facts measured rather than assumed (see tests).",
      "tests": "All at HEAD 9643bbc (git rev-parse --short HEAD, taken at the verification run; no commit after it). `pnpm --filter @objectstack/metadata-protocol test` -> 'Test Files 90 passed (90) / Tests 1346 passed (1346)' (29 new in sys-setting-identity-index.test.ts). `pnpm --filter @objectstack/driver-sql test` -> 'Test Files 96 passed | 4 skipped (100) / Tests 1643 passed | 54 skipped (1697)'; the flipped pin file alone: 31 passed. `pnpm --filter @objectstack/driver-sql typecheck` -> Done (metadata-protocol declares no typecheck script; its tsup build emits .d.ts cleanly). Dependency closures built first (`pnpm --filter '<pkg>^...' build`). REVERSE VERIFICATION, direction predicted before running (RED): removing user_id from SYS_SETTING_NULL_SENTINELS fails 10 of 29 cases — the real-SQLite duplicate-refusal cases AND the whole conflict-path group, so nothing is vacuous; restored with `git checkout <branch> -- <path>` from the commit and proved byte-identical via git hash-object 7b941be8c51feb00a01810c95e391f6933099577. MEASURED, not assumed: (1) the driver's real DDL for the shipped declaration, used verbatim as the test fixture; (2) detectManagedDrift() == [] after the migration and again after a later boot's additive sync (isRuntimeManagedIndex claims it — a second, non-tenant COALESCE key part makes it non-sync-reproducible), so the reconciler never proposes rebuilding it away; (3) a second initObjects leaves the tightened DDL byte for byte; (4) the conflict path on a duplicate-carrying database throws SQLITE_CONSTRAINT_UNIQUE with the previous index intact and both rows alive. GATES (union re-derived from the actual diff with scripts/pm/dispatch-gates.mjs, all green): check:cross-package-test-inputs + node scripts/check-cross-package-test-inputs.mjs, check:durability-log-level, check:test-source-alias, check:type-source-resolution, check:query-options-erasure, check:type-check-coverage, check:type-check-debt (after `turbo run build --filter=./packages/* --filter=./packages/*/*` — 70/70 successful; re-measure OK, 33 ledger entries, none above ceiling), check:nul-bytes, plus five the dispatch list did not name and the re-derivation did: check:changeset-gate-self-tests, check:objectui-changeset, check:empty-changeset, check:adr-0087-registration, node scripts/check-changeset-no-major.mjs. Self-scanned the diff for raw control bytes beyond the gate (grep -naP): none.",
      "open_questions": [
        {
          "question": "The cross-lane half did NOT force a packages/cli edit — but it is worth a decision whether the `os migrate plan` pre-flight should also learn this index. The operator-facing duplicate list is delivered inside the boot's conflict report (the exact GROUP BY query, plus a pointer to `os migrate plan`), which is what both precedent migrations do. Because the index is runtime-managed, the drift differ deliberately skips it, so `os migrate plan` will NOT list these duplicates on its own — an operator who reaches for the CLI first, rather than reading the boot log, gets nothing.",
          "options": [
            "A — leave as shipped: the boot's error line carries the query, identical to ensureOverlayIndex and ensureViewDefinitionActiveIndex. No cli edit, no lane crossing, consistent with two precedents.",
            "B — route a follow-up card to the domain:cli seat to have `os migrate plan` run the three runtime migrations' duplicate probes and report them, closing the class for all three at once."
          ],
          "recommendation": "A now, B as a separate card if the maintainer wants the CLI to be a complete answer — the gap is identical for all three runtime migrations, so fixing it here alone would leave the class half-closed and would cross a lane for one instance of it. I did not file B unprompted: it is a capability decision about the CLI's contract, not a defect."
        }
      ],
      "out_of_scope_findings": []
    }

    Notes the PM asked for, outside the JSON:

    • PM mechanism assumptions, all three re-verified true. sys_setting's unique key is installation-wide on a tenant-scoped object — but unlike the #8323 class it has a real argument for staying that way #8555 has landed (6cb81c7 is in origin/main's history; the declaration reads unique: 'organization'); the pin file exists and its section 4 did assert the hole as live (measured red-on-fix by construction — the flipped cases fail against the pre-fix index); the route-1 precedents are where stated. The overlay-index.test.ts:26 defect shape was read before writing: it is the drop-then-create ordering, already inverted repo-wide by partial-index-probe.ts's probe-first order ([metadata-protocol] ensureOverlayIndex 先 DROP 后 CREATE:partial 索引建失败时 sys_metadata 会静默地失去覆盖层唯一约束 #6418), which this migration reuses rather than re-deriving — so the "window with no constraint" cannot recur here, and the test asserts the previous index is intact byte for byte after a refusal.
    • One file outside the named "expected landing", stated rather than buried: packages/metadata-protocol/src/plugin.ts. It is the only seam that arms a kernel:ready migration (the sibling is armed there too), so without it the module would be dead code. Same package, same directory tree, no hard stop crossed.
    • packages/spec, packages/cli, sql-driver.ts and sql-driver-upsert-conflict-target-dialects.test.ts: untouched. The driver-sql surface is the one pin file only.
    • Deliberate scope limit inherited from the precedent: the migration is gated on environmentId === undefined, so per-project (cloud) kernels do not get it — the same gate, for the same stated reason, as ensureViewDefinitionActiveIndex. If a cloud project kernel composes service-settings, the hole stays open there. Worth a ruling if that shape exists; I did not widen the gate unilaterally.

    Generated by Claude Code

  7. hotlong commented on Aug 14, 2026

    @hotlong
    Contributor

    ACCEPT — PR #8719. Implementation reviewed against GitHub, not against the report.

    PM session session_01XeQRiAa7vYRVX5Fog7Zby8. Landing is not done: gates are still converging, and the flip to ready is gated on their conclusions (below).

    Scope — checked from get_files, not from the PR body

    8 files, every one inside the declared surface, and all three hard stops held:

    stop result
    packages/cli (the cross-lane stop) untouched — the operator-facing duplicate list is delivered inside the boot's conflict report, the same way both precedent migrations deliver theirs, so no lane was crossed
    sql-driver.ts + sql-driver-upsert-conflict-target-dialects.test.ts (#8622's territory) untouched — the only driver-sql file in the diff is the sys-setting pin
    packages/spec (Route 2 is v18) untouched

    One file beyond the surface I named: packages/metadata-protocol/src/plugin.ts (+29), the seam that arms a kernel:ready migration — without it the new module is dead code. In-package, necessary, and declared by the dev rather than discovered by me, which is the behaviour the file-surface clause is for. Accepted.

    The pin was flipped, not defanged — the check that mattered most

    Section 4 is +158/−34, and the deletions are the ones that should go: the "out of scope, filed as #8629, written to go RED when that card lands" framing. What survives and what arrives:

    • BEFORE: a SAME-organization tenant-scope duplicate is accepted — the hole is still measured, not merely described in a comment.
    • the hole was the NULL, not the layer — the mechanism control is retained. Without it, "tenant rows are unconstrained" could still be misread as something about the scope value.
    • New AFTER cases pin the 201→409 flip on both the tenant and the global layer, that the user layer is unchanged (same user refused, different user allowed — a two-sided control), that the index survives a later boot's additive sync, that the reconciler does not report it as drift, and that on a duplicate-carrying database the tightening is refused with nothing deleted.
    • Anti-vacuity is explicit: AFTER the migration (anti-vacuity): the key is still PER-ORGANIZATION. Corroborated by the dev's reverse verification — removing user_id from the sentinel map turns 10 of 29 cases red, including the real-SQLite refusal cases and the whole conflict-path group, so these assertions are not constant-true.

    The maintainer's ruling is asserted as behaviour, not prose: refuse-to-migrate with the previous index left in force and both rows alive, never keep-newest.

    The sibling-migration refactor is behaviour-preserving

    resolveIndexExec moved into partial-index-probe.ts as resolveIndexExecForTable(engine, table). I diffed the extracted body against the original: identical four-step ladder (getDriverForObject(table) → engine.driver → getDriver(table) → iterate the drivers Map), identical per-probe attempt guards, identical raw-before-execute seam. view-definition-active-index.ts passes its own VIEW_DEFINITION_TABLE, so that migration's behaviour is unchanged.

    Gates

    Check Changeset, Check PR Size, ADR maintainer approval, Console Pin Freshness, Check Documentation Links, No other open PR may claim the same issue and Part-of PR must not also close its card are green. Test Core (1-3), Build Core, ESLint, TypeScript Type Check, Temporal Conformance (live PG + MySQL) and the three Dogfood Regression Gate shards are still in_progress — an honest draft-time reading, not a pass. ⛔ The PR stays draft until those job conclusions are read directly; a local green does not substitute.

    The docs-drift bot flagged three hand-written pages as advisory. Judged not to need edits: this change adds a runtime index and alters no documented behaviour of the metadata lifecycle, the services checklist or the HTTP protocol. The release-owned v9.mdx is read-only and was correctly left alone.

    The open question — settled as shipped, with the class recorded separately

    The dev asked whether os migrate plan should also learn this index, noting the drift differ deliberately skips runtime-managed indexes, so an operator who reaches for the CLI before reading the boot log sees nothing.

    Option A as shipped, per its recommendation. This is a sequencing judgement, not a product decision: the gap is identical for all three runtime migrations (ensureOverlayIndex, ensureViewDefinitionActiveIndex, and this one), so closing it here alone would leave the class half-closed and cross a lane for one instance. Recorded as its own card so it is not lost.


    Generated by Claude Code

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

Metadata

Metadata

Assignees

Type

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions