fix(fields): emit the spec's $notContains, and keep secret out of inline edit (#2901) (#2940)

Two defects the #2901 audit turned up that need no design decision.

**`$ncontains` was never a real operator.** The sharing-rule criteria builder
emitted `{ $ncontains: v }` for "does not contain" — a token that appears zero
times in `@objectstack/spec`, and that objectui's own `convertFiltersToAST`
throws on while naming the correct spelling (`$notContains`) in its error
message. Every such rule validated in the UI and was rejected downstream.
`kvToCondition` keeps reading the old spelling so criteria saved before this
still load instead of failing as "can't be represented".

**`secret` was inline-editable as plaintext.** It sat in neither `EDIT_WIDGETS`
nor `INLINE_EXCLUDED_FIELD_TYPES`, so the grid fell back to a plain text input —
two lines below `password`, which is correctly excluded. Both are masked on
read, so that input renders the mask as the value and writes it back; `secret`
additionally round-trips through an encrypted store (ADR-0100), meaning the cell
holds an opaque ref, not the value.

Adding it tripped the exclusion-set staleness guard, which is itself
mis-specified: it treats `Object.keys(fieldWidgetMap)` as the set of real field
types, but spec spellings that reach a widget through the alias table
(`secret` → `field:password`, `autonumber` → `field:auto_number`, …) are not
keys — as index.tsx's own `resolveFormWidgetType` docs note. Made the check
alias-aware so it still catches typos without rejecting real types.

New `FilterConditionField.operators` test pins every builder token to a
`FieldOperatorsSchema` spelling and pins the round trip; verified it fails
against the old `$ncontains`.

Refs #2901

Co-authored-by: Jack Zhuang <277994282+os-zhuang@users.noreply.github.com>
Co-authored-by: Claude <noreply@anthropic.com>

Author: os-zhuang

packages/fields/src/FieldEditWidget.test.ts

@@ -11,8 +11,27 @@ import {
   FORM_FIELD_TYPES,
   INLINE_EXCLUDED_FIELD_TYPES,
   hasFieldEditWidget,
+  mapFieldTypeToFormType,
 } from './index';
 
+/**
+ * A field type the form can render — either a direct widget-map key, or a spec
+ * spelling that resolves onto one through the alias table (`secret` →
+ * `field:password`, `autonumber` → `field:auto_number`, …).
+ *
+ * `FORM_FIELD_TYPES` alone is `Object.keys(fieldWidgetMap)`, which does NOT
+ * include the alias-only spellings — see the note at index.tsx's
+ * `resolveFormWidgetType`. Using it as the definition of "a real type" made the
+ * staleness check below reject `secret`, a genuine spec field type the form
+ * renders, purely because it arrives via an alias.
+ */
+function isRenderableFormType(t: string): boolean {
+  // `text` is the alias table's fallback, so a non-`field:text` result means an
+  // explicit entry exists. `text` itself is a direct widget key, so it is
+  // already covered by the first clause and no real type is missed here.
+  return FORM_FIELD_TYPES.includes(t) || mapFieldTypeToFormType(t) !== 'field:text';
+}
+
 /**
  * Drift-guard: the inline cell editor reuses the form's field widgets, but the
  * two lists were hand-maintained separately and drifted — `lookup` (and other
@@ -34,11 +53,21 @@ describe('inline editor ↔ form widget parity', () => {
   });
 
   it('the exclusion set lists only real form types (no stale entries)', () => {
-    const known = new Set(FORM_FIELD_TYPES);
-    const stale = [...INLINE_EXCLUDED_FIELD_TYPES].filter((t) => !known.has(t));
+    const stale = [...INLINE_EXCLUDED_FIELD_TYPES].filter((t) => !isRenderableFormType(t));
     expect(stale).toEqual([]);
   });
 
+  it('credential types are never inline-editable', () => {
+    // The grid's fallback for a type with no inline editor is a PLAIN TEXT input.
+    // Both of these are masked on read, so that input would show the mask as the
+    // value and write it straight back; `secret` also round-trips through an
+    // encrypted store (ADR-0100), so the cell holds an opaque ref, not the value.
+    for (const t of ['password', 'secret']) {
+      expect(hasFieldEditWidget(t), `${t} must not have an inline editor`).toBe(false);
+      expect(INLINE_EXCLUDED_FIELD_TYPES.has(t), `${t} must be explicitly excluded`).toBe(true);
+    }
+  });
+
   it('relational fields use the standard picker inline (regression: lookup was a text box)', () => {
     for (const t of ['lookup', 'master_detail', 'user', 'owner']) {
       expect(hasFieldEditWidget(t)).toBe(true);

packages/fields/src/FieldEditWidget.tsx

@@ -102,7 +102,13 @@ export const INLINE_EXCLUDED_FIELD_TYPES = new Set<string>([
   // Binary / attachment — edited from the record form, shown read-only in the grid.
   'file', 'image', 'avatar', 'signature',
   // Heavy / full editors — better in the record form than a cell.
-  'markdown', 'html', 'richtext', 'password',
+  'markdown', 'html', 'richtext',
+  // Credentials — never inline-editable. Both are masked on read, so the cell has
+  // no real value to seed an editor with, and the grid's fallback is a PLAIN TEXT
+  // input: it would render the mask as if it were the value and write it straight
+  // back. `secret` additionally round-trips through an encrypted store (ADR-0100,
+  // data/field.zod.ts) — the cell holds an opaque ref, not the secret.
+  'password', 'secret',
   // Containers / non-authorable — a sub-form / sub-grid / embedding vector
   // doesn't belong in a single cell.
   'object', 'grid', 'vector',

packages/fields/src/widgets/FilterConditionField.tsx

@@ -102,7 +102,13 @@ function toArray(value: any): any[] {
   return value == null ? [] : [value];
 }
 
-function condToMongo(c: BuilderCondition, typeOf: (f: string) => string | undefined): Record<string, any> | null {
+/**
+ * Builder condition → `$`-operator criteria. Exported for tests: this is the
+ * chokepoint where a builder token becomes a spec `FieldOperatorsSchema` key,
+ * and a wrong spelling here is rejected downstream by `convertFiltersToAST`
+ * rather than at authoring time. @internal
+ */
+export function condToMongo(c: BuilderCondition, typeOf: (f: string) => string | undefined): Record<string, any> | null {
   const { field, operator, value } = c || ({} as BuilderCondition);
   if (!field) return null;
   const t = typeOf(field);
@@ -111,7 +117,11 @@ function condToMongo(c: BuilderCondition, typeOf: (f: string) => string | undefi
     case 'equals': return { [field]: cv };
     case 'notEquals': return { [field]: { $ne: cv } };
     case 'contains': return { [field]: { $contains: value } };
-    case 'notContains': return { [field]: { $ncontains: value } };
+    // `$notContains` is the spec spelling (FieldOperatorsSchema, data/filter.zod.ts).
+    // This emitted `$ncontains` — a token that appears nowhere in @objectstack/spec and
+    // that convertFiltersToAST throws on, so every "does not contain" rule authored here
+    // was rejected downstream. See kvToCondition for reading the old spelling back.
+    case 'notContains': return { [field]: { $notContains: value } };
     case 'isEmpty': return { [field]: { $in: [null, ''] } };
     case 'isNotEmpty': return { [field]: { $nin: [null, ''] } };
     case 'greaterThan':
@@ -146,7 +156,13 @@ function arraysEqual(a: any, b: any[]): boolean {
   return Array.isArray(a) && a.length === b.length && a.every((v, i) => v === b[i]);
 }
 
-function kvToCondition(field: string, v: any, idx: number): BuilderCondition | null {
+/**
+ * Criteria → builder condition (the reverse of {@link condToMongo}). Returning
+ * `null` makes the builder refuse to load the rule ("criteria can't be
+ * represented"), so this must keep accepting spellings previously written.
+ * @internal
+ */
+export function kvToCondition(field: string, v: any, idx: number): BuilderCondition | null {
   const id = `c_${idx}_${field}`;
   if (v === null || typeof v !== 'object' || Array.isArray(v)) {
     return { id, field, operator: 'equals', value: v };
@@ -158,6 +174,10 @@ function kvToCondition(field: string, v: any, idx: number): BuilderCondition | n
     switch (op) {
       case '$ne': return { id, field, operator: 'notEquals', value: val };
       case '$contains': return { id, field, operator: 'contains', value: val };
+      // `$ncontains` is the pre-fix spelling this widget used to emit. Criteria saved
+      // before the fix still carry it, so keep reading it — dropping it here would make
+      // those rules fail to load ("criteria can't be represented") instead of migrating.
+      case '$notContains':
       case '$ncontains': return { id, field, operator: 'notContains', value: val };
       case '$gt': return { id, field, operator: 'greaterThan', value: val };
       case '$lt': return { id, field, operator: 'lessThan', value: val };

packages/fields/src/widgets/__tests__/FilterConditionField.operators.test.ts

@@ -0,0 +1,100 @@
+/**
+ * ObjectUI
+ * Copyright (c) 2024-present ObjectStack Inc.
+ *
+ * This source code is licensed under the MIT license found in the
+ * LICENSE file in the root directory of this source tree.
+ */
+
+/**
+ * Operator spelling guard for the sharing-rule criteria builder (#2901).
+ *
+ * `condToMongo` emitted `$ncontains` — a token that appears nowhere in
+ * `@objectstack/spec` and that objectui's own `convertFiltersToAST` throws on,
+ * naming the correct spelling (`$notContains`) in its error message. So every
+ * "does not contain" rule authored here validated in the UI and was then
+ * rejected downstream. Nothing caught it because the builder's operator list is
+ * hand-written and never compared against the spec vocabulary.
+ *
+ * These tests pin the emitted spellings against `FieldOperatorsSchema`'s keys
+ * and pin the round-trip, including the pre-fix spelling that stored criteria
+ * still carry.
+ */
+import { describe, it, expect } from 'vitest';
+import { condToMongo, kvToCondition } from '../FilterConditionField';
+
+/** The `$`-prefixed operator keys of the spec's `FieldOperatorsSchema`. */
+const SPEC_OPERATORS = new Set([
+  '$eq', '$ne', '$gt', '$gte', '$lt', '$lte', '$in', '$nin',
+  '$between', '$contains', '$notContains', '$startsWith', '$endsWith',
+  '$null', '$exists',
+]);
+
+const noTypes = () => undefined;
+
+/** Pull the operator keys out of a `{ field: { $op: v } }` fragment. */
+function operatorsOf(frag: Record<string, any> | null): string[] {
+  if (!frag) return [];
+  return Object.values(frag).flatMap((v) =>
+    v !== null && typeof v === 'object' && !Array.isArray(v) ? Object.keys(v) : [],
+  );
+}
+
+describe('condToMongo emits only spec operator spellings', () => {
+  const builderOperators = [
+    'equals', 'notEquals', 'contains', 'notContains', 'isEmpty', 'isNotEmpty',
+    'greaterThan', 'lessThan', 'greaterOrEqual', 'lessOrEqual', 'in', 'notIn',
+    'before', 'after',
+  ];
+
+  it.each(builderOperators)('%s emits a spec-defined operator', (operator) => {
+    const frag = condToMongo(
+      { id: 'c1', field: 'name', operator, value: operator === 'in' || operator === 'notIn' ? ['a'] : 'a' } as any,
+      noTypes,
+    );
+    const unknown = operatorsOf(frag).filter((op) => !SPEC_OPERATORS.has(op));
+    expect(unknown, `${operator} emits an operator @objectstack/spec does not define`).toEqual([]);
+  });
+
+  it('notContains emits $notContains, not the pre-fix $ncontains', () => {
+    const frag = condToMongo({ id: 'c1', field: 'name', operator: 'notContains', value: 'x' } as any, noTypes);
+    expect(frag).toEqual({ name: { $notContains: 'x' } });
+  });
+
+  it('between emits a range the spec defines', () => {
+    const frag = condToMongo({ id: 'c1', field: 'age', operator: 'between', value: [1, 5] } as any, noTypes);
+    expect(operatorsOf(frag).every((op) => SPEC_OPERATORS.has(op))).toBe(true);
+  });
+});
+
+describe('kvToCondition round-trips what condToMongo writes', () => {
+  const cases: Array<[string, unknown]> = [
+    ['notEquals', 'a'],
+    ['contains', 'a'],
+    ['notContains', 'a'],
+    ['greaterThan', 1],
+    ['lessThan', 1],
+    ['greaterOrEqual', 1],
+    ['lessOrEqual', 1],
+  ];
+
+  it.each(cases)('%s survives the round trip', (operator, value) => {
+    const frag = condToMongo({ id: 'c1', field: 'f', operator, value } as any, noTypes)!;
+    const [[field, v]] = Object.entries(frag);
+    expect(kvToCondition(field, v, 0)).toMatchObject({ field: 'f', operator });
+  });
+
+  it('still reads the pre-fix $ncontains so stored criteria keep loading', () => {
+    // Dropping this would make rules saved before the fix fail to load entirely
+    // ("criteria can't be represented") rather than migrate.
+    expect(kvToCondition('name', { $ncontains: 'x' }, 0)).toMatchObject({
+      field: 'name',
+      operator: 'notContains',
+      value: 'x',
+    });
+  });
+
+  it('rejects an operator it cannot represent rather than guessing', () => {
+    expect(kvToCondition('name', { $nope: 'x' }, 0)).toBeNull();
+  });
+});