Repository navigation
Field inspector: Save is not gated on CEL errors — a parse-fault formula saves and publishes as the live field definition #4306
Description
Activity
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):
- Mirror the working precedent, measured: PermissionAdvancedFacets already threads
onCelErrorsChange→setCelErrorCountand 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. - 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.
- 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. - Scope stays the inspector:
PermissionAdvancedFacets.tsxandpermission-slice.tsare 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
- Mirror the working precedent, measured: PermissionAdvancedFacets already threads
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-gateat878140be9(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
CelPredicateFieldmounts inObjectFieldInspector.tsxare 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:CelPredicateFieldreports throughonLintChange(issues)(line 97, fired at 160).
onCelErrorsChangeis not a prop of it at all.onCelErrorsChangeis the aggregating parent's prop, declared on
PermissionAdvancedFacets(178) and consumed byPermissionMatrixEditor
(setCelErrorCountat 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
ObjectFieldInspectoris registered as theobjectinspector 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 istype, name, draft, selection, onPatch, onClearSelection, onSelectionChange, readOnly, locale— no error channel. There is also no context alternative: zero
createContexthits across all ofmetadata-adminandstudio-design.So gating Save requires editing
inspector-registry.tsplus at least one host, both outside
the dispatched hard-limit surface. Two further consequences the ruling did not price in:MetadataInspectorPropsis 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.- The prop's name decides a family.
ConditionBuilder(PageBlockInspector,
HookDefaultInspector, ActionDefaultInspector) andConditionalFormattingEditor
(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.saveBlockedalready exists in both language maps
(en 1148, zh 2935) and both hosts already importtfrommetadata-admin/i18n, so the RLS
message is reachable verbatim. Only itsperm.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
PermissionAdvancedFacetsdoes, 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
onPatchinside 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
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:
- 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.
- 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).
- Both hosts in the same PR (StudioDesignSurface + ResourceEditPage): hold the count, fold count-into-Save-disabled, reuse the existing
perm.cel.saveBlockedmessage verbatim — do NOT rename the key in this card; itsperm.*prefix becoming a misnomer is recorded as a naming smell, nothing more. - 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).
- 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.
- 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
- added a commit that references this issue
on Aug 13, 2026 - added a commit that references this issue
on Aug 17, 2026 - added a commit that references this issue
on Sep 1, 2026
Symptom
In the metadata-admin field inspector, a formula that fails to parse shows its inline error and saves anyway.
record.est_hours *in the formula box, the inline CEL error is displayed — and "Save draft" stays enabled.GETreads backexpression: '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.tsxmountsCelPredicateFieldfour times and passes noonCelErrorsChangeprop to any of them:formulaeditor (type === 'formula')visibleWhen/readonlyWhen/requiredWhen)onCelErrorsChangedoes not appear anywhere in that file. The control is one directory over:PermissionAdvancedFacets.tsxdeclares the prop (line 178), threads it through a ref (lines 253-258) and receivesonCelErrorsChange={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
objectuiorigin/main(33c32bf). All four mount sites are still present at the same lines, still with noonCelErrorsChange; thePermissionAdvancedFacetscontrol 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/formulafor the same expression and site — type inference, unknown-field with did-you-mean, bare-ref with therecord.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
formulafield.record.est_hours *into the formula box. The inline parse error appears.GETthe object →expressionreads backrecord.est_hours *.Source
Extracted from the QA run objectstack-ai/objectstack#7695 (framework 92f26f75, console 09987b6).