Field inspector: Save is not gated on CEL errors — a parse-fault formula saves and publishes as the live field definition

[huangyiirene]

Symptom

In the metadata-admin field inspector, a formula that fails to parse shows its inline error and saves anyway.

• With record.est_hours * in the formula box, the inline CEL error is displayed — and "Save draft" stays enabled.
• Clicking it PUTs → 200 with a success toast.
• Publishing makes the malformed expression the live field definition: GET reads back expression: 'record.est_hours *'.

Reproduced twice.

The RLS editor on the same build does the right thing — it disables Save with "Fix the CEL syntax errors before saving." — which is what makes this an inconsistency inside one console rather than a missing feature.

Root cause

packages/app-shell/src/views/metadata-admin/inspectors/ObjectFieldInspector.tsx mounts CelPredicateField four times and passes no onCelErrorsChange prop to any of them:

• line 478 — the formula editor (type === 'formula')
• lines 615 / 628 / 641 — the three conditional rules (visibleWhen / readonlyWhen / requiredWhen)

onCelErrorsChange does not appear anywhere in that file. The control is one directory over: PermissionAdvancedFacets.tsx declares the prop (line 178), threads it through a ref (lines 253-258) and receives onCelErrorsChange={setCelErrorCount} from its host — which is exactly why the RLS editor's Save is correctly gated. So the error-count channel exists and works; this inspector simply is not connected to it.

Stale-premise check: re-verified on objectui origin/main (33c32bf). All four mount sites are still present at the same lines, still with no onCelErrorsChange; the PermissionAdvancedFacets control still has it. Zero-hit confirmed against a known-present neighbour, so this is an absence, not a search miss.

Context

The editor/engine parity these editors exist for is otherwise clean. Nine probes compared the editor's verdict against the framework's own @objectstack/formula for the same expression and site — type inference, unknown-field with did-you-mean, bare-ref with the record. fix, parse faults, the RLS pushdown warning, and the test-run allow/deny/non-boolean triple — with no divergence in tier or text. All five CEL sources are md5-identical between the served pin and objectui HEAD, so the verdicts apply to the running bundle. The gap is purely that the field inspector does not act on a verdict it already renders.

Related: #2413 landed the CEL authoring safety net (lint + autocomplete + test-run) for RLS policies, and #1582 brought the CEL editor to the field conditional rules. The Save gate came with the first and not with the second.

Reproduction

1. Boot the showcase with a writable runtime package; open metadata-admin → an object → a formula field.
2. Type record.est_hours * into the formula box. The inline parse error appears.
3. Observe "Save draft" is still enabled; click it → 200 + success toast.
4. Publish the draft; GET the object → expression reads back record.est_hours *.
5. Control: do the same in the RLS policy editor — Save is disabled with "Fix the CEL syntax errors before saving."

Source

Extracted from the QA run objectstack-ai/objectstack#7695 (framework 92f26f75, console 09987b6).

[yinlianghui]

CLAIM — session_017Qqyix2QcnpUC9XeYVDzx3, branch claude/issue-4306-field-inspector-cel-gate. Dispatching a dev agent now.

PM ruling (delegated decision authority; maintainer veto window open — record objections here):

1. Mirror the working precedent, measured: PermissionAdvancedFacets already threads onCelErrorsChange → setCelErrorCount and gates Save; measure that aggregation shape first and mirror it. With FOUR mount sites (478 formula, 615/628/641 conditional rules), state must be a per-site error map (keyed by site), not one shared count that later sites overwrite — the classic multi-source aggregation bug. Save draft disables while any site's count is nonzero.
2. Same user-facing message as the RLS editor ("Fix the CEL syntax errors before saving.") through the same channel/key if it is reachable from the inspector; add one defaults-map row per language only if measured unavoidable.
3. Red-first with the card's exact repro: record.est_hours * in the formula box currently leaves Save draft enabled (→ 200 + toast). Pins: a valid formula keeps Save enabled; each of the three conditional-rule editors also gates; clearing the error re-enables Save; the RLS editor's own gate unchanged.
4. Scope stays the inspector: PermissionAdvancedFacets.tsx and permission-slice.ts are READ-ONLY references here ([security] An RLS policy authored in Studio's package door is silently discarded — Save succeeds, no row filter is ever persisted #4302 owns that surface concurrently).

Mutual exclusion / sequencing: gate worktree creation on PR #4525 (issue #4518) being visible in origin/main (shared metadata-admin i18n.ts tail risk; #4525 is armed on green CI, so the wait is minutes). Disjoint from #4302 (permission-slice/PermissionMatrixEditor), #4490 (plugin-grid), #4040 (plugin-dashboard).

---

Generated by Claude Code

---

Generated by Claude Code

[yinlianghui]

Dev measurement — the defect is real, the stated root cause is not, and the fix cannot fit the dispatched surface

Worktree claude/issue-4306-field-inspector-cel-gate at 878140be9 (the #4518 / PR #4525
squash, so the sequencing gate is satisfied). No code written — escalating per the ruling's
veto window before guessing at a public contract.

What holds

All four CelPredicateField mounts in ObjectFieldInspector.tsx are still at 478 (formula)
and 615 / 628 / 641 (visibleWhen / readonlyWhen / requiredWhen), and none of them reports its
lint verdict upward. The symptom in the card stands.

What does not hold

The card says "the error-count channel exists and works; this inspector simply is not
connected to it". Measured, that is two different channels:

• CelPredicateField reports through onLintChange(issues) (line 97, fired at 160).
  onCelErrorsChange is not a prop of it at all.
• onCelErrorsChange is the aggregating parent's prop, declared on
  PermissionAdvancedFacets (178) and consumed by PermissionMatrixEditor
  (setCelErrorCount at 1036, disabled={saving || celErrorCount > 0} at 801).

Both ends of that channel live inside the permission editor, which owns its own Save
button. Nothing about it is reachable from a registry-mounted inspector.

Why the fix cannot stay inside the dispatched surface

ObjectFieldInspector is registered as the object inspector and mounted by hosts that each
own the Save button that must be disabled:

• StudioDesignSurface.tsx — the object designer, React.createElement(inspector, ...) around
  2704; its "Save draft" button (2262) is gated !current || !dirty || !!saving || readOnly.
  This is the button in the card's repro.
• ResourceEditPage.tsx — the metadata-admin editor, inspector at 2301, its own Save at 1719.

The only interface between them is MetadataInspectorProps (inspector-registry.ts), whose
full prop set is type, name, draft, selection, onPatch, onClearSelection, onSelectionChange, readOnly, locale — no error channel. There is also no context alternative: zero
createContext hits across all of metadata-admin and studio-design.

So gating Save requires editing inspector-registry.ts plus at least one host, both outside
the dispatched hard-limit surface. Two further consequences the ruling did not price in:

1. MetadataInspectorProps is re-exported from the package entry (app-shell/src/index.ts),
   so adding a prop is entry-reachable additive API growth — minor by the stated grading
   precedent, not the "expected patch" in the dispatch.
2. The prop's name decides a family. ConditionBuilder (PageBlockInspector,
   HookDefaultInspector, ActionDefaultInspector) and ConditionalFormattingEditor
   (ViewVariantInspector) drop the same verdict — filed as CEL blocking errors are dropped on the floor in ConditionBuilder and ConditionalFormattingEditor too — same ungated-Save family as #4306 #4527, blocked by this decision.

Good news on ruling item 2

No i18n work is needed. perm.cel.saveBlocked already exists in both language maps
(en 1148, zh 2935) and both hosts already import t from metadata-admin/i18n, so the RLS
message is reachable verbatim. Only its perm. prefix is a naming smell once a field
inspector uses it.

The decision I need

A. Extend the contract (recommended). Add one optional reporter to
MetadataInspectorProps, have the inspector aggregate a per-site map exactly as
PermissionAdvancedFacets does, and let each host hold the count and gate its own Save with
perm.cel.saveBlocked. Name it for blocking issues rather than CEL so #4527 is wiring, not a
rename. Costs a minor changeset and edits to two hot shared files.

B. Keep the malformed value out of the draft (guard onPatch inside the inspector).
Fits the surface and grades patch, but Save stays enabled and silently persists the last valid
expression while the author believes their text was saved — consumer-side tolerance, and it
does not deliver the ruling's own acceptance criterion.

I recommend A, wiring the object designer (the card's repro) and ResourceEditPage
together so the contract has no half-live consumer. It needs an explicit surface extension to
inspector-registry.ts + StudioDesignSurface.tsx + ResourceEditPage.tsx, and re-checking
mutual exclusion — the dispatch's disjointness analysis assumed this card touched only the
inspector.

---

Generated by Claude Code

---

Generated by Claude Code

[yinlianghui]

PM ruling on the measured escalation above — Option A, with sub-decision A (host-owned reset). Delegated decision authority; maintainer veto window open — record objections here before this lands.

Why A, on product-stability grounds:

• The host owns Save, so the host must learn Save is unsafe. That is the contract-first shape; a typed optional reporter on MetadataInspectorProps makes "this inspector can block Save" something every future inspector sees, instead of a private convention.
• Option B (guard onPatch so the malformed expression never reaches the draft) is rejected as the classic lenient-consumer workaround: it silently persists the last valid expression while the author believes their text saved — it converts a loud error into silent data loss, which is worse than the bug.
• Option C is rejected: a half-live contract with one wired host and one silently ungated Save is exactly the two-doors divergence this repo keeps paying for ([security] An RLS policy authored in Studio's package door is silently discarded — Save succeeds, no row filter is ever persisted #4302's card is the same shape one directory over).
• The harm is measured and live (a parse fault publishes as the live field definition), and the same gap reaches four more inspectors via ConditionBuilder / ConditionalFormattingEditor (CEL blocking errors are dropped on the floor in ConditionBuilder and ConditionalFormattingEditor too — same ungated-Save family as #4306 #4527) — one channel serves the family.

Binding details:

1. The prop is named for blocking issues generally, not CEL (e.g. an optional count reporter; match the naming idiom of the existing MetadataInspectorProps callbacks). Optional, so every existing inspector stays valid.
2. The inspector aggregates a per-site map over its four mounts exactly on the PermissionAdvancedFacets precedent — identity-preserving writes, and prune on field-type change (a formula error must not survive switching the field away from type formula, or Save wedges).
3. Both hosts in the same PR (StudioDesignSurface + ResourceEditPage): hold the count, fold count-into-Save-disabled, reuse the existing perm.cel.saveBlocked message verbatim — do NOT rename the key in this card; its perm.* prefix becoming a misnomer is recorded as a naming smell, nothing more.
4. Sub-decision A: the host resets the count to 0 on selection change and on inspector unmount — writability must never depend on a child's teardown running (the precedent's own prune effect exists for this failure mode).
5. Scope stays: contract + ObjectFieldInspector + the two hosts. CEL blocking errors are dropped on the floor in ConditionBuilder and ConditionalFormattingEditor too — same ungated-Save family as #4306 #4527's five other inspectors are a follow-up card gated on this landing — do not wire them here.
6. Changeset: minor for @object-ui/app-shell — MetadataInspectorProps is entry-reachable, so this is additive public API growth per the standing precedent line.

Mutual-exclusion re-check (done): the widened surface (inspector-registry.ts, ObjectFieldInspector.tsx, StudioDesignSurface.tsx, ResourceEditPage.tsx) is disjoint from #4302's write surface (permission-slice.ts, PermissionMatrixEditor.tsx, PermissionAdvancedFacets seed) — ResourceEditPage is read-only for #4302. No other in-flight card touches app-shell.

---

Generated by Claude Code

---

Generated by Claude Code