fix(app-shell): lint conditional-formatting conditions in the record scope - #8164
Draft
os-justin wants to merge 3 commits into
Draft
fix(app-shell): lint conditional-formatting conditions in the record scope#8164os-justin wants to merge 3 commits into
os-justin wants to merge 3 commits into
Conversation
…scope
The conditional-formatting editor authored its CEL in the `flattened` scope,
where any bare identifier is legal, and advertised `data` in its autocomplete
roots. Phase 2 of the row-predicate canon retired both spellings on runtime
record surfaces: `evalRowPredicate` binds the row as `record.*` and nothing
else, so `status == 'overdue'` and `data.status == 'overdue'` fault at runtime
while the editor linted them green.
- `CelPredicateField` authors in `scope="record"`, the scope the field
conditional rules already use, so a bare field ref is an ERROR carrying the
`record.<field>` fix.
- `ROW_PREDICATE_ROOTS` drops `'data'`.
- The docblock and the inline comment describing the old three-way binding are
rewritten to the one binding that survives.
The shared `hint.scope ?? 'flattened'` default is untouched: RLS predicates and
flow conditions are not row surfaces.
Tests: the pin that asserted "a bare field lints clean" is turned to assert the
`record.<field>` diagnostic — its own comment predicted this edit. The
roots-to-runtime pin is repaired: it looped every advertised root asserting
`size(<root>) >= 0` against a host scope that itself carried `data: {}`, so for
`data` the probe hit the host's own empty object and could not fail. Each root
is now checked against the binder that is supposed to supply it, in both
directions, and `data` and `os` get their own pins against a scope that does
carry them.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01YBWFb5YgMU5dw8p2VKj16S
Contributor
✅ Console Performance Budget
The eager closure is every chunk the entry reaches through static imports — what the browser fetches and parses before the app renders. The entry chunk on its own is a small fraction of it. 📦 Bundle Size Report
Size Limits
|
Collaborator
Author
PM review — verdict on the diff, and why the PR is held rather than landedReviewed by the PM seat (session Two framings in the report I have to correct① "the exported I checked this the way I should have checked A star re-export would have put the name on the published face without the literal ever appearing in This does not un-bind Clause-②: the clause has two legs, and the accept/reject leg fires hard here — the editor's lint verdict flips for a whole class of input. ② The The report and #8155 both frame return { current_user: user, user, ctx: { user }, os: { user }, app, data, features };
Mitigating, and measured: zero in-tree uses of One gap neither the dispatch nor the report namesThe PR closes the bare-field half of the silent-death hole and leaves the From #8155's own measurement, the engine's accept set at
That is not a defect this PR introduces — it is pre-existing and unchanged — and it is not a reason to hold. But the changeset currently reads as though the editor now catches the retired spellings, and it catches one of two. The changeset's "Known gap" section should name this alongside the What was verified and stands
Held, and on whatStaying draft pending a maintainer ruling on #8155, where I have put the ship/hold question with a fourth option the report did not enumerate. My reasoning: this PR trades a silent wrong answer for a hard block with no workaround. The trade is probably right — a silent dead rule is the worse failure — but knowingly shipping a no-escape-hatch block to an unmeasurable population is a call for the maintainer, not for me. The producer-side fix is one string in one array; if it is fast-tracked, the question dissolves. ⛔ Not folded in, correctly: #7728, #7835. Generated by Claude Code |
This was referenced Sep 6, 2026
Collaborator
Author
Contract review returned PASS-WITH-FINDINGS — two blockers. Patch round in progress.Tier note: the designated contract-review tier was measured unavailable this session ( Correcting myself: in my review above I wrote "The diff is sound" and separately vouched for the changeset. The implementation logic is indeed sound and is not changing. The changeset is not — it carries a false claim, and I did not catch it. Blocker 1 — "the last place still teaching a spelling the runtime had already retired" is false
Blocker 2 — the PR leaves its own hole open while implying it is closedAt That hole turns out to be live on main today, independent of this PR, on Also going into the patch round
Confirmed clean, so nobody re-checks itThe Still held in draft, now on two counts: this patch round, and the unresolved ruling on #8155. Generated by Claude Code |
… pin the data half Contract-review follow-up on the conditional-formatting scope flip. No implementation logic changes; this is coverage and text. - The roots-to-runtime suite no longer writes the app-shell predicate bag out by hand. It calls `buildExpressionScope`, derives the advertised-root expectation from `Object.keys(...)` minus an explicit curated-exclusion list, and runs the `os` and `data` pins against that same bag. A literal cannot disagree with its producer, so it silently absorbs drift -- and it already had: the literal omitted `os`, which the producer really does bind, so the old `os` assertion "proved" it unbound. The previous `os` pin also handed `os` in by hand, which showed only that `evalRowPredicate` forwards `scope`; it now reads the producer and carries an unbound-root control. - New characterization pin: a `data.*` condition still lints CLEAN. Dropping `data` from the advertised roots stops recommending it, not accepting it -- the engine's `SCOPE_ROOTS` lists `data`, so the record-scope lint waves it through while the runtime pin one suite lower asserts it is false. Green here plus false there is the defect, and the pair is the referent. - The `app` pin's comment now says which of its card's two candidate fixes it is a tripwire for; the closure assertion covers the other. - The host-roots test no longer says "advertised host roots must survive" while sitting above a test proving one of them does not. The changeset drops the false "last place" claim, states that this closes the bare-field half of the retirement and not the `data.*` half, spells out that a saved view with a legacy condition becomes unsavable in the designer until it is rewritten, and records the bare-position autocomplete change. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YBWFb5YgMU5dw8p2VKj16S
Contributor
✅ Console Performance Budget
The eager closure is every chunk the entry reaches through static imports — what the browser fetches and parses before the app renders. The entry chunk on its own is a small fraction of it. 📦 Bundle Size Report
Size Limits
|
`ROW_PREDICATE_ROOTS` is an `export const`, which the release note read as an API change. Measured: `packages/app-shell/src/index.ts` has 0 `export *` lines and names neither the const nor `ConditionalFormattingEditor`, and the package `exports` map is `"."` plus `./styles.css` with no deep subpath -- so nothing outside the package can import it. The behavioural narrowing is real and is what the note is about; the published surface is unchanged. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YBWFb5YgMU5dw8p2VKj16S
Contributor
✅ Console Performance Budget
The eager closure is every chunk the entry reaches through static imports — what the browser fetches and parses before the app renders. The entry chunk on its own is a small fraction of it. 📦 Bundle Size Report
Size Limits
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Part of #7727
⛔ Deliberately not a closing keyword. All six of triage's deliverables for #7727 are done here, but the PM review asked that the card not be shut as fully done, and the ship decision is parked on a maintainer ruling in #8155 — a keyword close would take the card out of the open-card filters while that is still being decided. What is left of #7727's family after this PR: the
data.*half of the retirement (#8166) and theConditionBuildermounts (#8167). Leave #7727 open; a human can shut it once those are triaged.⛔ DRAFT, and it carries
needs:contract-review. Clause-2 applies on its accept/reject leg: the editor's lint verdict flips for a whole class of input — a bare field ref that lints clean today becomes a blocking ERROR. It does not apply on a published-surface leg, and an earlier revision of this description said it did. Measured:packages/app-shell/src/index.tshas 0export *lines and names neitherROW_PREDICATE_ROOTSnorConditionalFormattingEditor; the packageexportsmap is"."plus./styles.csswith no deep subpath.ROW_PREDICATE_ROOTSis anexport constthat nothing outside the package can import, so@object-ui/app-shell's published face is unchanged. The changeset now says so too.Contract review returned PASS-WITH-FINDINGS; the commits after the first answer it. No implementation logic changed —
ConditionalFormattingEditor.tsxis byte-identical across all three commits (blob5caa9119). What changed is coverage and text, and the two corrections are worth reading before the diff.Correction 1 — this editor was NOT "the last place still teaching the retired spelling"
The changeset said so and the claim was false. #7727, its triage and my own sweep all searched the explicit spelling
scope="flattened".CelPredicateFieldalso reaches the flattened lint by callers passing noscopeprop at all (celAuthoring.tsreadshint.scope ?? 'flattened'). Every mount in the repo, measured:scope=in that fileConditionalFormattingEditor.tsx:341record— this PRinspectors/ObjectFieldInspector.tsx:760, :874, :888, :902recordPermissionAdvancedFacets.tsx:356, :372inspectors/ConditionBuilder.tsx:368ConditionBuilderhas noscopeon its own props either, so no caller can override it.ActionDefaultInspector.tsx:671-672mounts it for an action'svisible/disabled, whichrowPredicateCanon.ts:16-18names as a row predicate in as many words, anduseExpression.ts:153-158binds as{ record }only. So a barestatus == 'done'in an action guard lints clean and never matches: this card's exact defect, untouched here.Filed as #8167 with all six
ConditionBuildercallers classified — two defects, one likely (ObjectValidationsPanel, whose sibling validator already runsscope: 'record'atclientValidation.ts:792), one correct as-is (the flow tier), three needing a tier verdict. ⛔ Not folded in: the fix is not "flip ConditionBuilder", it is "make the scope a prop and rule on three surfaces".Correction 2 — the fix leaves half the retirement open, and now says so
At⚠️ silently accepted". Dropping
scope:'record'the engine still acceptsdata.status == 'x':@objectstack/formula'sSCOPE_ROOTSlistsdata, so the bare-reference check waves it through.rowPredicateCanon.ts:36-38already records exactly that for the server oracle —data.status, "datafromROW_PREDICATE_ROOTSstops recommending it; it does not stop accepting it.So after this PR the editor lints
data.status == 'overdue'green while the runtime pin one suite lower asserts the same predicate isfalse. That pair is now pinned on both sides as a characterization test, labelled not-desired-behaviour, reddening when the acceptance is fixed. Tracked as #8166 (not a duplicate of #8155 — opposite failure from the same root list: #8155 is a root bound-but-refused, #8166 is a root accepted-but-dead).The changeset now states that this shuts the bare-field half of the retirement only.
The rest of the review
hostScopeandHOST_BOUND_ROOTSwere hand-written literals — the same artefact as thedataprobe, one level up: a literal cannot disagree with its producer, so it silently absorbs drift. It already had drifted (the literal omittedos, whichbuildExpressionScopereally binds, so the old assertion "proved"osunbound). The suite now importsbuildExpressionScopefrom../../providers/ExpressionProvider.js, derives the bag andObject.keys(...)from it, and subtracts an explicitCURATED_EXCLUSIONS = ['os','data']. Theospin previously handedosin by hand, which showed only thatevalRowPredicateforwardsscope; it now reads the producer and carries an unbound-root control (size(zzz) >= 0against the same bag is false).app.*pin only tripwires one of Therecord-scope CEL lint refuses theapproot, which app-shell's predicate scope DOES bind —@objectstack/formula'sSCOPE_ROOTShas noapp#8155's two fixes. Its comment now says so: it reads on option A (addapptoSCOPE_ROOTS); option B (stop bindingapp) is caught by the closure assertion, which reads the advertised list againstbuildExpressionScope.CelPredicateField.tsx:216-225buildsbareCandidateswithfields: []underscope === 'record', so typingstain bare position no longer offersstatus; fields come as member completion afterrecord..onBlockingIssuesChange(CEL blocking errors are dropped on the floor in ConditionBuilder and ConditionalFormattingEditor too — same ungated-Save family as #4306 #4527), so a saved view carrying a legacy bare condition becomes unsavable in the designer until rewritten — including when opened for an unrelated edit.appdoes not. It now coverscurrent_user,user,ctxandfeaturesand is titled by what they have in common: the engine knows them.introspectScopereturns byte-identicalfieldsandrootsforrecordandflattenedagainst@objectstack/formula@17.2.0— it echoes the caller'sfieldshint rather than deriving one per scope (["status","amount"]both ways;[]both ways with no hint). So member completion afterrecord.does not move; only the bare-position catalog does, and that isCelPredicateField's ownfields: []arm, not the engine's.What changed in the first commit (unchanged by the review)
CelPredicateFieldauthors inscope="record", the scopevisibleWhen/readonlyWhen/requiredWhenalready use.ROW_PREDICATE_ROOTSdrops'data'.:34-44docblock (which claimed the row is bound bare, underrecord.*and underdata.*) and the:329-332inline comment are rewritten to the one binding that survives.test.tsx:135-143is turned — its own comment predicted this edit verbatim: flipping this editor toscope="record"would break this test.test.tsx:178-200is repaired: it looped every advertised root assertingsize(ROOT) >= 0against ahostScopethat itself carrieddata: {}, so fordatathe probe hit the host's own empty object and could not fail.CelTestRunDialog.tsx:18's bind recipe was re-read and is still true — it describestestRunCelPredicate's own bag (celAuthoring.ts:461-464), notevalRowPredicate's, and that dialog is RLS-only.appis bound at runtime (ExpressionProvider.tsx:88, handed toresolveConditionalFormattingbyObjectGrid.tsx:2086/ListView.tsx:639) and advertised by this editor, butSCOPE_ROOTShas noapp, so underscope="record"the lint refuses it with the nonsense fixrecord.app. Measured accept set atscope:'record': acceptedrecord previous parent input os current_user user vars features ctx data env settings; refusedapp tenant org. Underflattenedeverything bare is accepted — evenzzz.buildExpressionScopebindsctx: { user }andos: { user }— each carriesuserand nothing else — soctx.app/os.appare not reachable spellings today; they are candidate rebindings under #8155 option B, not escape hatches. An author who needsapp.namehas no spelling that both lints clean and resolves. Save stays shut.⛔ I did not suppress the diagnostic in
celAuthoring.ts: that is the lenient-fallback shape AGENTS.md #0.1 bans, and the accept set belongs to the engine. This is the thing to decide in contract review.Proof the pins are lit
Six ablations across the commits. Each mutated only after the implementation was committed; each proved to have reached disk by an anchor count before/after (a no-op edit exits 0 and would otherwise read as a run); each restored with
git checkout HEAD -- PATHinside atrap ... EXIT INT TERMon absolute paths; each restore proved by state —git hash-objectequal togit rev-parse HEAD:PATHfor every touched file, plus an emptygit diff HEAD— never by an exit code.data: {}fromhostScoperoot "data" should be bound at runtime: expected false to be true. Its green depended entirely on the host's object.'data'back intoROW_PREDICATE_ROOTSadvertised root "data" is unaccounted for,expected [...] to not include 'data'scope="record"toscope="flattened"appknown-gap pindata.*incelAuthoring.tsdatacharacterization pin. It is not vacuous: it reddens the day #8166 is fixed.osfrombuildExpressionScopeexpected [ 'current_user', 'user', 'ctx', …(3) ] to include 'os'. Theospin reads the producer now; the old hand-injected version would have stayed green.buildExpressionScopeexpected [ 'app', 'ctx', 'current_user', …(3) ] to deeply equal [ 'ablation_new_root', 'app', …(5) ]. That is what catches #8155 option B.Legs A and E are the point of the exercise: in both, the pin claimed a fact that was already false, and was green.
Verification
Repo root, on the head commit; every heavy run through the shared verify lock, verdicts read from its
VERDICTline and never from a bare$?.pnpm exec vitest run --reporter=verbose .../ConditionalFormattingEditor.test.tsx— 25 passed, each named individually.pnpm exec vitest run packages/app-shell/— 635 files, 6116 passed, 1 skipped, 0 failed.turbo run type-check --filter=@object-ui/app-shell— 30 tasks successful, including dependency builds andtsc -p tsconfig.test.json.eslint .inpackages/app-shell(the package's ownlintscript) — 1090 files, 0 errors, exit 0 captured before any pipe.pnpm check:control-bytes— OK, 6512 files;check:shell-escape-residue— OK;check-changeset-presence.mjs— OK;check-governed-queue-guard.mjs --test— NOT GOVERNED.apps/console+ the two examples (90 files, 1061 passed) were verified on the first commit and not re-run after it.git diff 35e49ace1 HEAD --name-onlyis one.test.tsxand one changeset.md; the implementation blob is identical; nothing imports that test file. Their inputs are byte-identical. CI runs them anyway.Findings filed rather than folded in: #8155, #8166, #8167. ⛔ #7728 and #7835 remain separate cards.
Generated by Claude Code