fix(app-shell): compile a reference typed into ConditionBuilder's value box as a reference

.changeset/6293-conditionbuilder-reference-value.md

@@ -0,0 +1,39 @@
+---
+'@object-ui/app-shell': patch
+---
+
+`ConditionBuilder`'s row mode now compiles a value that is plainly a **reference** as one,
+instead of quoting it into a string literal (objectui#6293).
+
+`fmtValue` quoted anything that was not a number / `true` / `false` / `null`, and the value
+box is free text — so an author building "this field differs from its prior value", the idiom
+that *defines* a change-detection predicate, got `previous == 'previous.status'`. That is
+syntactically valid CEL, `previous` is a declared root, and a string literal's contents are
+deliberately not scanned for references by `flow-ref-check` or by the server-side validator.
+The predicate parsed, registered, evaluated — and was always false, with no author-time signal
+at any layer, at all five surfaces that mount this builder.
+
+A value matching a **declared root prefix** now emits as the reference: `record`, `previous`,
+`parent`, `user`, `current_user`, `org`. That set is this builder's own vocabulary — `record`
+/ `user` / `org` are exactly what its subject dropdown offers one control to the left,
+`previous` and `parent` are bound by `evalFieldPredicate` and by the server-side hook /
+validation evaluators, and `current_user` is the ADR-0068 spelling of the same identity object
+`user` names. Roots this builder never offers (`data`, `os`, `app`, `features`, `input`,
+`vars`, `page`) are deliberately excluded: `data.csv` is a plausible literal and `data` *is*
+bound, so capturing it would trade one silently-false predicate for another rather than for a
+loud one. Declaring which roots a mounting surface actually binds is caller-supplied
+vocabulary and belongs to objectui#6296.
+
+The test is "a dotted path under a declared root", not "contains a dot" — a version string
+(`1.2.3`), a filename, and a path under an unbound root all stay literal text. The literal and
+number controls are unchanged: `done` still compiles to `'done'`, `42` still to `42`.
+
+**Nothing already stored is rewritten.** A persisted `previous == 'previous.status'` no longer
+round-trips byte-for-byte, so the builder's existing safety rule hands it to the raw CEL editor
+rather than reinterpreting it — the author sees both readings and decides. In the other
+direction a hand-authored `record.status != previous.status` now round-trips *into* the row
+builder, which it could not before.
+
+The repair is at the authoring surface, where the ambiguity is: no consumer-side tolerance is
+added, and the emitted reference is now an identifier the existing reference checkers can see,
+where a string literal's contents were invisible to them.

packages/app-shell/src/views/metadata-admin/inspectors/ConditionBuilder.referenceValue.test.tsx

@@ -0,0 +1,124 @@
+// Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license.
+
+/**
+ * ConditionBuilder row mode — a value that is plainly a REFERENCE compiles as
+ * one, instead of being quoted into a string literal (objectui#6293).
+ *
+ * The observable pinned here is the emitted CEL, not the UI state: `fmtValue`
+ * quoted anything that was not a number / `true` / `false` / `null`, so
+ * "this field differs from its prior value" — the idiom that DEFINES a
+ * change-detection predicate — compiled to `previous == 'previous.status'`.
+ * That parses, registers and evaluates, and is always false; nothing at any
+ * layer objects, because `previous` is a declared root and a string literal's
+ * contents are deliberately not scanned for references.
+ *
+ * Mounted through the real component and read off `onCommit`, because the
+ * commit path above `fmtValue` is part of the defect — a unit test of the
+ * formatter alone would not have caught a builder that never reaches it.
+ */
+
+import * as React from 'react';
+import { describe, it, expect, afterEach, vi } from 'vitest';
+import { render, screen, cleanup, fireEvent } from '@testing-library/react';
+
+// objectui#4697 — ConditionBuilder calls useObjectFields(objectName)
+// unconditionally even when a `fields` prop is supplied, so stub the shared
+// client to keep the mount-time fetch off the network.
+const state = vi.hoisted(() => ({
+ metadataClient: { get: vi.fn(async () => undefined), list: vi.fn(async () => [] as unknown[]) },
+}));
+vi.mock('../useMetadata', () => ({
+ useMetadataClient: () => state.metadataClient,
+}));
+
+import { ConditionBuilder } from './ConditionBuilder';
+
+afterEach(cleanup);
+
+const FIELDS = [
+ { name: 'status', label: 'Status' },
+ { name: 'done', label: 'Done' },
+];
+
+/**
+ * Controlled harness — the hook-condition surface, opened on `previous == null`
+ * exactly as the card measured it (that round-trips, so the builder opens in
+ * ROW mode with a live value box).
+ */
+function Harness({ initial }: { initial: string }) {
+ const [v, setV] = React.useState(initial);
+ return (
+ <div>
+ <ConditionBuilder
+ label="Run only when"
+ value={v}
+ onCommit={setV}
+ objectName="task"
+ fields={FIELDS}
+ />
+ <pre data-testid="committed">{v}</pre>
+ </div>
+ );
+}
+
+/** Type into the row's live value box and return the CEL that was emitted. */
+function typeIntoValueBox(text: string): string {
+ const input = screen.getByPlaceholderText('value') as HTMLInputElement;
+ fireEvent.change(input, { target: { value: text } });
+ return screen.getByTestId('committed').textContent ?? '';
+}
+
+describe('ConditionBuilder row mode — reference vs. literal in the value box (#6293)', () => {
+ it('opens `previous == null` in ROW mode with a live value box (positive probe)', () => {
+ const { container } = render(<Harness initial="previous == null" />);
+ expect(container.querySelector('textarea')).toBeNull(); // not the raw CEL editor
+ expect((screen.getByPlaceholderText('value') as HTMLInputElement).value).toBe('null');
+ });
+
+ it('compiles a value under a declared root as the REFERENCE it plainly is', () => {
+ render(<Harness initial="previous == null" />);
+ expect(typeIntoValueBox('previous.status')).toBe('previous == previous.status');
+ });
+
+ it('compiles `record.<field>` on the value side as a reference too', () => {
+ render(<Harness initial="previous == null" />);
+ expect(typeIntoValueBox('record.status')).toBe('previous == record.status');
+ });
+
+ it('CONTROL — a literal is still quoted', () => {
+ render(<Harness initial="previous == null" />);
+ expect(typeIntoValueBox('done')).toBe("previous == 'done'");
+ });
+
+ it('CONTROL — a number is still emitted unquoted', () => {
+ render(<Harness initial="previous == null" />);
+ expect(typeIntoValueBox('42')).toBe('previous == 42');
+ });
+
+ it('CONTROL — a dotted value under an UNDECLARED root is still quoted', () => {
+ // The repair keys on the declared root vocabulary, not on "contains a dot":
+ // `foo` is not a root any mounting surface binds, and `1.2.3` is a version
+ // string. Both stay literal text.
+ render(<Harness initial="previous == null" />);
+ expect(typeIntoValueBox('foo.bar')).toBe("previous == 'foo.bar'");
+ expect(typeIntoValueBox('1.2.3')).toBe("previous == '1.2.3'");
+ });
+
+ it('the emitted reference round-trips — reopening it stays in ROW mode', () => {
+ // If it did not round-trip, the builder would flip to the raw CEL editor
+ // the moment the author reopened the record they had just authored.
+ const { container } = render(<Harness initial="record.status != previous.status" />);
+ expect(container.querySelector('textarea')).toBeNull();
+ expect((screen.getByPlaceholderText('value') as HTMLInputElement).value).toBe('previous.status');
+ });
+
+ it('is NOT retroactive — an already-stored quoted literal opens in RAW mode, never silently rewritten', () => {
+ // A predicate already persisted as text (whether the author meant the text
+ // or hit this defect) is not the builder's to reinterpret: it no longer
+ // round-trips, so the component's existing safety rule hands it to the raw
+ // CEL editor where the author can see both readings and decide.
+ const { container } = render(<Harness initial="previous == 'previous.status'" />);
+ expect(container.querySelector('textarea')).not.toBeNull();
+ expect(screen.getByTestId('committed').textContent).toBe("previous == 'previous.status'");
+ });
+});

packages/app-shell/src/views/metadata-admin/inspectors/ConditionBuilder.tsx

@@ -52,15 +52,72 @@ const CONTEXT_SUBJECTS = [
 
 const norm = (s: string) => s.replace(/\s+/g, ' ').trim();
 
-/** Quote a raw value for CEL unless it is a number / boolean / null. */
+/**
+ * Scope roots a value typed into the value box may plainly REFERENCE, rather
+ * than name as literal text (objectui#6293).
+ *
+ * Deliberately the roots this builder's own vocabulary already commits to —
+ * not every root the CEL engine advertises:
+ *
+ * - `record` / `user` / `org` — this component's own subject vocabulary
+ * (`record.<field>` from the field catalog, plus {@link CONTEXT_SUBJECTS}).
+ * A value under one of these is the same identifier the subject dropdown
+ * emits one control to its left.
+ * - `previous` — the prior persisted record, bound by `evalFieldPredicate`
+ * (`@object-ui/core`) and by the server-side hook / validation evaluators.
+ * This is the change-detection idiom the defect was measured on.
+ * - `current_user` — the canonical spelling of `user` (ADR-0068); the shell
+ * binds one identity object under both names, so which alias the author
+ * happened to type must not decide whether it reads as a reference.
+ * - `parent` — the header row an inline line-item cell compares against,
+ * bound through `evalFieldPredicate`'s `scope` extra.
+ *
+ * NOT included, on purpose: `data`, `os`, `app`, `features`, `input`, `vars`,
+ * `page`. Those are real roots at some surfaces, but this builder never offers
+ * them, and over-capturing there fails in the WRONG direction — `data.csv` is
+ * a plausible literal, and `data` IS bound, so reading it as a reference would
+ * produce another silently-false predicate instead of a loud one. Which roots
+ * a mounting surface actually binds is caller-supplied vocabulary
+ * (objectui#6296) and is that card's to declare, not this one's to guess.
+ */
+const REFERENCE_ROOTS = ['record', 'previous', 'parent', 'user', 'current_user', 'org'] as const;
+
+/**
+ * A dotted path under a declared root — i.e. plainly a reference.
+ *
+ * Anchored, and dotted identifiers only. "Contains a dot" is NOT the test: a
+ * version string (`1.2.3`), a filename, and a path under a root nothing binds
+ * all stay literal text.
+ */
+const REFERENCE_RE = new RegExp(
+ `^(?:${REFERENCE_ROOTS.join('|')})(?:\\.[A-Za-z_][A-Za-z0-9_]*)+$`,
+);
+
+/**
+ * Quote a raw value for CEL unless it is a number / boolean / null — or a
+ * reference (objectui#6293).
+ *
+ * Quoting a reference was silent in both directions: `previous ==
+ * 'previous.status'` is valid CEL, `previous` is a declared root, and a string
+ * literal's contents are deliberately not scanned for references by
+ * `flow-ref-check` or by the server-side validator — so the predicate parsed,
+ * registered, evaluated, and was always false, with no author-time signal at
+ * any layer. Emitting the reference is also what makes it CHECKABLE: it is now
+ * an identifier those existing checkers can see.
+ */
 function fmtValue(v: string): string {
  const t = v.trim();
  if (t === 'true' || t === 'false' || t === 'null') return t;
  if (t !== '' && !Number.isNaN(Number(t))) return t;
+ if (REFERENCE_RE.test(t)) return t;
  return `'${t.replace(/'/g, "\\'")}'`;
 }
 
-/** Inverse of fmtValue for display in the value input. */
+/**
+ * Inverse of fmtValue for display in the value input. A bare reference has no
+ * quotes to strip and passes through unchanged, which is what keeps an emitted
+ * `previous == previous.status` round-tripping back into the row builder.
+ */
 function unfmtValue(raw: string): string {
  const t = raw.trim();
  const m = /^'(.*)'$/.exec(t);