feat(cli): lint the contradictory uniqueness double-declaration (#3991) (#4034)

os-zhuang · web-flow · commit ec36ba82e8f5 · 2026-07-30T14:35:34.000+08:00
Closes #3991. The authoring-side close-out of #3955/#3981: that PR stopped the runtime churn a column with two uniqueness declarations caused in drift detection; the metadata that produces the combination was still accepted silently. Field-level `unique: true` is tenant-scoped since #3696 — it materializes as `(organization_id, col)` — while a declared index is materialized over exactly its `fields`, i.e. platform-wide. Each is legitimate alone; together on one column exactly one takes effect. On a tenant-scoped object the global index wins and the per-tenant constraint is unreachable, which also masks the #3696 semantic change entirely: the switch has no observable effect while the declared index still enforces the old behaviour, so the author only finds out when a second tenant is rejected. On a tenancy-less object the two are the same index declared twice. New advisory rule `unique/double-declaration` in `lint/data-model-rules.ts`, so it reaches `os lint` and the metadata-generation scorer; exported standalone and wired into `os build` and `os validate`, which the #3782 build/validate parity gate enforces. Tenancy is deliberately not inferred at authoring time, so the message names both readings and the fix spells out the choice; `unique: 'global'` is exempt. Never fails a build. Also documents the distinction where an author actually meets it (`data-modeling/indexing.mdx`), which said nothing about the two spellings, the tenant scoping, or the autonumber trap — so the page led straight to the bug the lint now reports. Verified against the real HotCRM stack: fires on exactly the three sites a manual audit found and stays quiet on the three objects that declare only an index. The HotCRM-side fix is objectstack-ai/hotcrm#550.
diff --git a/.changeset/lint-unique-double-declaration.md b/.changeset/lint-unique-double-declaration.md
@@ -0,0 +1,37 @@
+---
+"@objectstack/cli": patch
+---
+
+feat(cli): lint the contradictory uniqueness double-declaration (#3991)
+
+New advisory rule `unique/double-declaration`, reported by `os lint` and
+`os build`. It fires when one column carries BOTH a field-level `unique: true`
+and an object-level single-column unique index:
+
+```ts
+email: Field.email({ unique: true }),           // per-tenant since #3696
+indexes: [{ fields: ['email'], unique: true }], // platform-wide, verbatim
+```
+
+The two spellings deliberately mean different things (see `IndexSchema`), and
+each is legitimate alone. Together on one column they never are:
+
+- On a **tenant-scoped** object they contradict. The stricter one wins
+  physically, so the global index enforces uniqueness and the per-tenant
+  composite becomes a constraint nothing can trip — one of the two authored
+  intents is silently discarded. Worse, it hides the #3696 semantic change:
+  the switch from global to per-tenant has *no observable effect* while the
+  declared index still enforces the old behaviour, so the author never learns
+  their tenancy model and their real constraint disagree — until a second
+  tenant reuses the value and is rejected.
+- On a **tenancy-less** object they are the same index declared twice.
+
+Tenancy is deliberately not inferred at authoring time (`organization_id` is
+injected by the kernel at registration, not authored), so the message names
+both readings and the fix spells out the choice: `unique: 'global'` plus
+dropping the index for platform-wide, or dropping the index for per-tenant
+(or writing it out as `fields: ['organization_id', 'email']`).
+
+A field already declared `unique: 'global'` is exempt — the index restates
+that intent rather than losing it. Advisory only: the artifact is well-defined,
+so this never fails a build.
diff --git a/content/docs/data-modeling/indexing.mdx b/content/docs/data-modeling/indexing.mdx
@@ -24,6 +24,51 @@ indexes: [
 ]
 ```
 
+## Two ways to say "unique" — and they mean different things
+
+Uniqueness can be declared in two places, and the choice is not cosmetic:
+
+| Declaration | Materializes as | Scope |
+|:---|:---|:---|
+| Field-level `unique: true` | `(organization_id, field)` | Unique **within** an organization |
+| Field-level `unique: 'global'` | `(field)` | Platform-wide |
+| Declared index `{ fields: ['email'], unique: true }` | `(email)` — exactly the listed columns | Platform-wide |
+
+A field-level `unique: true` is **tenant-scoped**. It has no syntax for a
+composite, so the platform supplies the tenant column for you — which is what
+a multi-tenant application almost always wants: two organizations may each
+have a contact `john@acme.com`.
+
+A **declared index is taken verbatim**. No tenant column is injected, because
+many declared indexes are legitimately platform-wide (a DNS hostname, a
+reserved slug, an external provider id). To scope one per tenant, list the
+column yourself: `{ fields: ['organization_id', 'email'], unique: true }`.
+
+<Callout type="warn">
+**Do not declare both on the same column.** The stricter one wins physically,
+so the platform-wide index enforces uniqueness and the per-tenant constraint
+can never be reached — one of the two intents you wrote is silently discarded:
+
+```typescript
+// ⚠️ contradictory — the global index wins, the per-tenant scope is dead
+email: Field.email({ unique: true }),
+indexes: [{ fields: ['email'], unique: true }],
+```
+
+`os lint` / `os build` report this as `unique/double-declaration`. Pick one:
+set `unique: 'global'` on the field and drop the index for platform-wide
+uniqueness, or drop the index for per-tenant uniqueness (the field-level
+declaration already builds the composite).
+</Callout>
+
+<Callout type="warn">
+**Never put a platform-wide unique index on an `autonumber` field.** The
+autonumber sequence is per tenant — every organization counts from `1` — so a
+global unique index rejects the second organization's `CASE-00001` on insert.
+Use `{ fields: ['organization_id', 'case_number'], unique: true }` so the
+constraint matches the sequence that feeds it.
+</Callout>
+
 ### When to Add Indexes
 
 ✅ **Add indexes for:**
diff --git a/packages/cli/src/commands/compile.ts b/packages/cli/src/commands/compile.ts
@@ -19,6 +19,7 @@ import { validateSecurityPosture, validateOrgAxisRedLines, buildAccessMatrix, di
 import { validateReadonlyFlowWrites } from '@objectstack/lint';
 import { lintFlowPatterns } from '../utils/lint-flow-patterns.js';
 import { lintAutonumberFormats } from '../utils/lint-autonumber-formats.js';
+import { lintUniqueDeclarations } from '../lint/data-model-rules.js';
 import { lintLivenessProperties } from '../utils/lint-liveness-properties.js';
 import { lintViewRefs } from '../utils/lint-view-refs.js';
 import { preflightRequiredCapabilities, renderCapabilityMessage } from '../utils/capability-preflight.js';
@@ -499,6 +500,28 @@ export default class Compile extends Command {
         }
       }
 
+      // 3d-quinquies. Contradictory uniqueness declarations (#3991). A column
+      //     carrying BOTH a field-level `unique: true` and a single-column
+      //     declared unique index has two intents, of which exactly one takes
+      //     effect: since #3696 the field-level form is per-tenant while a
+      //     declared index is platform-wide, so the global index wins and the
+      //     tenant composite becomes unreachable. Advisory — the artifact is
+      //     well-defined; the cost is a declaration that does nothing. Shares
+      //     `lintUniqueDeclarations` with `os lint` so both agree.
+      const uniqueLint = lintUniqueDeclarations(
+        Array.isArray((result.data as Record<string, unknown>).objects)
+          ? ((result.data as Record<string, unknown>).objects as any[])
+          : [],
+      );
+      if (uniqueLint.length > 0 && !flags.json) {
+        console.log('');
+        for (const f of uniqueLint) {
+          printWarning(`${f.path}: ${f.message}`);
+          if (f.fix) console.log(chalk.dim(`    ${f.fix}`));
+          console.log(chalk.dim(`    rule: ${f.rule}`));
+        }
+      }
+
       // 3d-quater. View-reference lint (#2554) — resolves form action targets
       //     and view-key collisions at build time. A `type:'form'` target that
       //     names a missing view or a LIST view opens a broken/blank form at
diff --git a/packages/cli/src/commands/validate.ts b/packages/cli/src/commands/validate.ts
@@ -24,6 +24,7 @@ import { validateFlowTriggerReadiness } from '@objectstack/lint';
 import { validateReadonlyFlowWrites } from '@objectstack/lint';
 import { lintFlowPatterns } from '../utils/lint-flow-patterns.js';
 import { lintAutonumberFormats } from '../utils/lint-autonumber-formats.js';
+import { lintUniqueDeclarations } from '../lint/data-model-rules.js';
 import { lintLivenessProperties } from '../utils/lint-liveness-properties.js';
 import { lintViewRefs } from '../utils/lint-view-refs.js';
 import { preflightRequiredCapabilities, renderCapabilityMessage } from '../utils/capability-preflight.js';
@@ -592,12 +593,24 @@ export default class Validate extends Command {
       const viewRefErrors = viewRefLint.filter((f) => f.severity === 'error');
       const viewRefWarnings = viewRefLint.filter((f) => f.severity !== 'error');
 
+      // Contradictory uniqueness declarations (#3991) — a column carrying both a
+      // field-level `unique: true` and a single-column declared unique index has
+      // two intents, of which exactly one takes effect. Advisory. Mapped into the
+      // `{ where, hint }` shape the shared renderer below expects; the rule lives
+      // in `lint/data-model-rules.ts` so `os lint` reports the same finding.
+      const uniqueLintWarnings = lintUniqueDeclarations(
+        Array.isArray((result.data as Record<string, unknown>).objects)
+          ? ((result.data as Record<string, unknown>).objects as any[])
+          : [],
+      ).map((f) => ({ where: f.path, message: f.message, hint: f.fix ?? '', rule: f.rule, severity: 'warning' as const }));
+
       const authoringLintErrors = [...flowLintErrors, ...autonumberErrors, ...viewRefErrors];
       const authoringLintWarnings = [
         ...flowLintWarnings,
         ...livenessLint,
         ...autonumberWarnings,
         ...viewRefWarnings,
+        ...uniqueLintWarnings,
       ];
       if (authoringLintErrors.length > 0) {
         if (flags.json) {
diff --git a/packages/cli/src/lint/data-model-rules.ts b/packages/cli/src/lint/data-model-rules.ts
@@ -69,6 +69,92 @@ function refOf(def: any): string | undefined {
   return def?.reference || def?.reference_to;
 }
 
+// ─── Uniqueness declarations ────────────────────────────────────────
+
+export const UNIQUE_DOUBLE_DECLARATION = 'unique/double-declaration';
+
+/** Is `unique` declared at all? Mirrors `isUniqueDeclared` in @objectstack/spec/data. */
+function uniqueDeclared(u: unknown): boolean {
+  return u === true || u === 'global';
+}
+
+/**
+ * R10 — the same column carries BOTH a field-level `unique: true` and an
+ * object-level single-column unique index (#3991).
+ *
+ * The two spellings are deliberately different (see `IndexSchema`): field-level
+ * `unique: true` is tenant-scoped since #3696 — it materializes as
+ * `(organization_id, col)`, unique *within* the tenant — while a declared index
+ * is materialized over exactly the columns listed, i.e. platform-wide. Both are
+ * legitimate on their own; together on one column they are never right:
+ *
+ *   - On a tenant-scoped object they CONTRADICT. The stricter one wins
+ *     physically, so the global index enforces uniqueness and the tenant
+ *     composite becomes a constraint nothing can ever trip. One of the two
+ *     intents the author wrote is silently discarded.
+ *   - On a tenancy-less object they are exactly REDUNDANT — both describe the
+ *     same single-column unique index, under the same generated name.
+ *
+ * Tenancy is deliberately NOT inferred here: `organization_id` is injected by
+ * the kernel at registration rather than authored, so an authoring-time guess
+ * would be wrong half the time. The combination is worth flagging either way,
+ * and the message names both readings so the author picks the one they meant.
+ *
+ * A field declared `unique: 'global'` is exempt: it already says
+ * platform-wide, so the declared index restates the same intent rather than
+ * contradicting it (still redundant, but not a silent loss of meaning).
+ *
+ * Advisory. The resulting stack is well-defined — the cost is an intent that
+ * never takes effect, not a broken artifact — so this never fails a build.
+ */
+export function lintUniqueDeclarations(objects: any[]): LintIssue[] {
+  const issues: LintIssue[] = [];
+  if (!Array.isArray(objects) || objects.length === 0) return issues;
+
+  for (let i = 0; i < objects.length; i++) {
+    const obj = objects[i];
+    if (!obj?.name) continue;
+    const declaredIndexes = Array.isArray(obj.indexes) ? obj.indexes : [];
+    if (declaredIndexes.length === 0) continue;
+
+    // Columns covered by a declared SINGLE-column unique index. A composite
+    // (`['organization_id', 'email']`) is the explicit tenant-scoped spelling —
+    // it agrees with the field-level default rather than fighting it.
+    const singleColumnUniqueIndexes = new Map<string, any>();
+    for (const idx of declaredIndexes) {
+      if (!uniqueDeclared(idx?.unique)) continue;
+      const cols = Array.isArray(idx?.fields) ? idx.fields.filter((f: unknown) => typeof f === 'string') : [];
+      if (cols.length !== 1) continue;
+      if (!singleColumnUniqueIndexes.has(cols[0])) singleColumnUniqueIndexes.set(cols[0], idx);
+    }
+    if (singleColumnUniqueIndexes.size === 0) continue;
+
+    for (const { name, def } of fieldEntries(obj.fields)) {
+      if (!uniqueDeclared(def?.unique)) continue;
+      if (def.unique === 'global') continue; // already says platform-wide — no lost intent
+      const idx = singleColumnUniqueIndexes.get(name);
+      if (!idx) continue;
+      const indexLabel = typeof idx?.name === 'string' && idx.name.trim() ? ` '${idx.name.trim()}'` : '';
+      issues.push({
+        severity: 'warning',
+        rule: UNIQUE_DOUBLE_DECLARATION,
+        message:
+          `"${obj.name}.${name}" declares field-level \`unique: true\` AND a single-column unique index${indexLabel} on the same column. ` +
+          `Since #3696 the field-level form is scoped per tenant — \`(tenant, ${name})\` — while a declared index is materialized ` +
+          `over exactly its \`fields\`, i.e. platform-wide. On a tenant-scoped object the global index wins and the per-tenant ` +
+          `constraint can never be reached; on a tenancy-less object the two are the same index declared twice. Either way one of ` +
+          `the two declarations has no effect.`,
+        path: `objects[${i}]`,
+        fix:
+          `Pick the intent: for platform-wide uniqueness set \`unique: 'global'\` on '${name}' and drop the duplicate index; ` +
+          `for per-tenant uniqueness drop the index (the field-level declaration already builds the tenant composite), ` +
+          `or spell the index out as \`fields: ['organization_id', '${name}']\` if you want it explicit.`,
+      });
+    }
+  }
+  return issues;
+}
+
 // ─── Rule engine ────────────────────────────────────────────────────
 
 /**
@@ -77,7 +163,9 @@ function refOf(def: any): string | undefined {
  * metadata-generation scorer.
  */
 export function lintDataModel(objects: any[]): LintIssue[] {
-  const issues: LintIssue[] = [];
+  // R10 lives in its own exported function so `os build` can run that ONE rule
+  // without pulling in the whole best-practice sweep (#3991).
+  const issues: LintIssue[] = lintUniqueDeclarations(objects);
   if (!Array.isArray(objects) || objects.length === 0) return issues;
 
   // Index: parent object name → child relationships pointing at it.
diff --git a/packages/cli/test/data-model-rules.test.ts b/packages/cli/test/data-model-rules.test.ts
@@ -1,5 +1,5 @@
 import { describe, expect, it } from 'vitest';
-import { lintDataModel } from '../src/lint/data-model-rules';
+import { lintDataModel, lintUniqueDeclarations } from '../src/lint/data-model-rules';
 import { lintConfig } from '../src/commands/lint';
 
 const rulesOf = (issues: { rule: string }[]) => issues.map((i) => i.rule);
@@ -173,3 +173,119 @@ describe('lintConfig integration', () => {
     expect(dataModel.filter((i) => i.severity !== 'suggestion')).toEqual([]);
   });
 });
+
+// #3991 — the same column declared unique twice, in two spellings that mean
+// different things. One of the two intents is always discarded.
+describe('lintUniqueDeclarations — contradictory uniqueness (#3991)', () => {
+  const RULE = 'unique/double-declaration';
+
+  const withBoth = [
+    {
+      name: 'crm_contact',
+      fields: { email: { type: 'email', unique: true } },
+      indexes: [{ fields: ['email'], unique: true }],
+    },
+  ];
+
+  it('returns [] for empty input', () => {
+    expect(lintUniqueDeclarations([])).toEqual([]);
+    expect(lintUniqueDeclarations(undefined as any)).toEqual([]);
+  });
+
+  it('flags field-level unique + a single-column unique index on the same column', () => {
+    const issues = lintUniqueDeclarations(withBoth);
+    expect(issues).toHaveLength(1);
+    expect(issues[0].rule).toBe(RULE);
+    expect(issues[0].severity).toBe('warning'); // advisory — never fails a build
+    expect(issues[0].message).toContain('crm_contact.email');
+    // The message must name BOTH readings, since tenancy is not inferred here.
+    expect(issues[0].message).toMatch(/per tenant|tenant/i);
+    expect(issues[0].message).toMatch(/platform-wide/i);
+    // And the fix must spell out both ways to resolve it.
+    expect(issues[0].fix).toContain("unique: 'global'");
+    expect(issues[0].fix).toContain('organization_id');
+  });
+
+  it('surfaces through lintDataModel too, so `os lint` reports it', () => {
+    expect(has(lintDataModel(withBoth), RULE)).toBe(true);
+  });
+
+  // ── Shapes that must stay quiet ──────────────────────────────────────
+
+  it("exempts unique: 'global' — the index restates the intent, it does not lose it", () => {
+    const issues = lintUniqueDeclarations([
+      {
+        name: 'runtime',
+        fields: { hostname: { type: 'text', unique: 'global' } },
+        indexes: [{ fields: ['hostname'], unique: true }],
+      },
+    ]);
+    expect(issues).toEqual([]);
+  });
+
+  it('exempts an explicit tenant COMPOSITE index — that agrees with the field-level default', () => {
+    const issues = lintUniqueDeclarations([
+      {
+        name: 'crm_contact',
+        fields: { email: { type: 'email', unique: true } },
+        indexes: [{ fields: ['organization_id', 'email'], unique: true }],
+      },
+    ]);
+    expect(issues).toEqual([]);
+  });
+
+  it('ignores a NON-unique index on the same column', () => {
+    const issues = lintUniqueDeclarations([
+      {
+        name: 'crm_contact',
+        fields: { email: { type: 'email', unique: true } },
+        indexes: [{ fields: ['email'] }],
+      },
+    ]);
+    expect(issues).toEqual([]);
+  });
+
+  it('ignores a unique index on a DIFFERENT column', () => {
+    const issues = lintUniqueDeclarations([
+      {
+        name: 'crm_contact',
+        fields: { email: { type: 'email', unique: true }, code: { type: 'text' } },
+        indexes: [{ fields: ['code'], unique: true }],
+      },
+    ]);
+    expect(issues).toEqual([]);
+  });
+
+  it('is quiet when only one of the two spellings is used', () => {
+    expect(lintUniqueDeclarations([
+      { name: 'a', fields: { email: { type: 'email', unique: true } } },
+    ])).toEqual([]);
+    expect(lintUniqueDeclarations([
+      { name: 'b', fields: { email: { type: 'email' } }, indexes: [{ fields: ['email'], unique: true }] },
+    ])).toEqual([]);
+  });
+
+  it('names the declared index when it carries an explicit name', () => {
+    const issues = lintUniqueDeclarations([
+      {
+        name: 'crm_product',
+        fields: { sku: { type: 'text', unique: true } },
+        indexes: [{ name: 'uniq_product_sku', fields: ['sku'], unique: true }],
+      },
+    ]);
+    expect(issues[0].message).toContain("'uniq_product_sku'");
+  });
+
+  it('reports each offending column once, across several objects', () => {
+    const issues = lintUniqueDeclarations([
+      ...withBoth,
+      {
+        name: 'crm_lead',
+        fields: { email: { type: 'email', unique: true }, sku: { type: 'text', unique: true } },
+        indexes: [{ fields: ['email'], unique: true }, { fields: ['sku'], unique: true }],
+      },
+    ]);
+    expect(issues.map((i) => i.message.match(/"([^"]+)"/)?.[1]).sort())
+      .toEqual(['crm_contact.email', 'crm_lead.email', 'crm_lead.sku']);
+  });
+});
