Uh oh!
There was an error while loading. Please reload this page.
fix(app-shell): report a faulting nav/area/field visible predicate in both builds - #6486
Merged
Merged
Conversation
…in both builds `ExpressionProvider.evaluateVisibility` is the gate behind a navigation item's `visible`, an area's derived visibility, and the field list `RecordFormPage` renders. It is fail-open, so a predicate that cannot be evaluated returns `true` and the item renders for everyone — including the role it was written to exclude — looking exactly like an entry the author meant to show. The fault was swallowed one layer down: `evaluateCondition` is fail-soft and answers an unevaluable predicate with `true` from its OWN catch without throwing, so this site's `try/catch` never saw a predicate fault at all. Measured per dialect at this site, the bare-string form — the one a live gate was measured breaking on — printed nothing whatsoever, in either build. Wires `EvaluationOptions.onFault` to `reportUnresolvableVisibilityPredicate` from `@object-ui/react`: the same reporter, message, severity, dedupe Set and rate limit the node gate and `page:tabs` already use, at the same single engine call (no `throwOnError` double evaluation). The reporter's `type` slot — the dedupe key, with the gate key and the predicate source — is the constant `app-shell:visible`, so the rate limit is one line per distinct authored predicate source rather than one per menu entry. Observability only: fail-open is unchanged on every path and dialect. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011SfZeFWrhGLHmfq61xbz4q
The cell that guards "this card does not flip fail-open to fail-closed" carried a `reports()` assertion, which made it red against `origin/main` for the same reason every discriminating cell is — so it could not do the job a control does. The console assertion moves to the bare-string cell, which now pins the log and the observable outcome together: a fault is reported AND the item is still visible to the role the predicate was written to exclude. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011SfZeFWrhGLHmfq61xbz4q
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
|
os-support-ai
marked this pull request as ready for review
August 26, 2026 04:54
This was referenced Aug 26, 2026
Uh oh!
There was an error while loading. Please reload this page.
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 freeto 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.
Fixes#6443
ExpressionProvider.evaluateVisibilityis the gate behind a navigation item'svisible, anarea's derived visibility, and the field list
RecordFormPagerenders. It is fail-open, so apredicate that cannot be evaluated returns
trueand the item renders for everyone —including the role it was written to exclude — looking exactly like an entry the author meant
to show.
The fault was swallowed one layer down.
evaluateConditionis fail-soft: it answers anunevaluable predicate with
truefrom its owncatchand does not throw, so this site'stry/catchnever saw a predicate fault at all.EvaluationOptions.onFault(the seam #6038landed) is the only channel that reaches it, and it costs no extra engine call.
The decision this card left to the dev: what stands in for node type/id
The reporter dedupes on
(type, key, predicate source)—idrides along in the printed linebut is not in the key. A nav item is not a schema node, so
typehad to be chosen, and itsets the rate limit. The choice is the constant
app-shell:visible,idomitted.Why a constant rather than the item's identity, against the three cases that can occur:
typetypesourceA per-item key is never better and sometimes much worse. Measured rather than asserted: the
rate-limit pin drives the real
AppSidebarcomposition —areas.filter(a => hasVisibleNavigationItems(a.navigation, …)), the derivation #3311 added — and counts 6evaluations of one faulting predicate across three areas over two passes, all collapsing to
one line. The count is asserted, so the cell cannot pass on a site that is entered once.
A per-item key is also unreachable without widening
VisibilityEvaluator(@object-ui/layout),whose whole signature is
(expression) => boolean— a cross-package public type change adiagnostics-only card does not get to make. The predicate source is the locator instead:
it is in both the key and the line, and it is the string an author greps their metadata for.
Not
nav:item, deliberately, even though the card is written about nav and area items:evaluateVisibilityis also the gate over an object's fieldvisiblepredicates inRecordFormPage, and labelling those "nav" would be false. Stated rather than hidden: a navitem and a form field carrying the identical broken predicate text share one dedupe entry.
The colon keeps the label out of the bare registry namespace —
app-shellis deliberately nota component key (#4841), so a diagnostic must not read as claiming one.
Fail-open is UNCHANGED — this is diagnostics only
The item still renders for everyone on a fault, on every path and dialect. Flipping that to
fail-closed would be a permission-boundary change wearing an observability costume, and it is
not this card's to make. A control cell carries no console assertion at all so it cannot go
red for the reason the discriminating cells do: it asserts the observable outcome — the nav item
survives the real
hasVisibleNavigationItemsguard for the excluded role — and it is greenagainst
mainand after.Measured per dialect, at this site
{ dialect: 'cel' }envelope${…}templateReverse verification — direction predicted before the run
Predicted: restoring
ExpressionProvider.tsxto the pinned base turns red exactly the 8discriminating cells (
reports()→ 0 in each), leaves the 4 controls green, and moves the${…}cell in a second, independent direction — total console lines 1 → 3.Observed, exactly:
Tests 8 failed | 8 passed (16), the 8 failures being the 8 discriminatingcells and the 8 passes being the 4 controls plus all 4 cells of the pre-existing suite. The
template cell's second direction was measured after reordering its assertions so it is not
short-circuited:
AssertionError: expected [ …(3) ] to have a length of 1 but got 3.Mutation proven on disk by grepping the deleted and the restored text (
onFault: report0,APP_SHELL_VISIBLE_SURFACE0,reportUnresolvableVisibilityPredicate0, old bare call 1) —never a diffstat, never an editor's exit code. Restore proven by blob hash
(
BACK_BLOB == HEAD_BLOB == 1c18358ca…) plus an emptygit diff HEAD, under anEXIT/INT/TERMtrap holding absolute paths. Vitest resolves workspace packages tosrcthrough the repoalias map, so no
diststaleness is involved on this leg.Why this pin has to exist, as a reading rather than a claim: the neighbouring suite
ExpressionProvider.evaluateVisibility.test.tspasses green against the defect — all 4 ofits cells passed in the ablation run. Its
fails open (visible) on an unevaluable predicatecase asserts the exact verdict the broken site returns. The observable difference was on the
console and nothing was looking there.
Degenerate controls, named: the positive/degenerate console-capture pair and the two
behaviour controls above pass both ways on purpose. They guard the future wrong shapes — a spy
that observes nothing (which would make every
toHaveLength(0)here vacuous), a reporter thatfires on a genuine
false, and a diagnostic change that also flips fail-open to fail-closed.Clause ② — published surface, stated in as many words
The surface is NOT widened.
git diffof every**/src/index.tsfrom the pinned base isempty, and — because an empty entry diff does not settle it on its own — the whole
packages/diff contains no added or removed line carrying anexportdeclaration (thesingle
exportmatch is a word inside a test comment). No member was added to any type theentry already exports by name: the emitted
dist/providers/ExpressionProvider.d.tsstilldeclares exactly
ExpressionContextValue,ExpressionProvider,useExpressionContextandevaluateVisibility, the last with a byte-identical signature. The one new module-level binding(
APP_SHELL_VISIBLE_SURFACE) is deliberately not exported and does not appear in thedeclaration face.
Gates — verdict lines, at
7ada702b8(the final commit)Exit codes captured before any pipe; each line below is the gate's own, not a shell
$?.Test Files 2 passed (2)/Tests 16 passed (16)Test Files 24 passed (24)/Tests 255 passed (255)type-check(tsc --noEmit && tsc -p tsconfig.test.json): exit 0, script name echoed. Thenew pin is in the program — confirmed with
--listFiles, not assumed.eslint .over the file set eslint's own configselected — 3811 files, 0 errors, 11046 pre-existing warnings. The changed source file adds
zero new warnings (its 10 are pre-existing
no-explicit-anyon the untouched interface andthe 2 pre-existing
react-refresh/only-export-components); the new test file adds 7no-explicit-any, matching its neighbour's idiom.check:control-bytes—✅ OK (scanned 5352 tracked text file(s); skipped 85 binary), plus adirect
grep -naPover exactly the 4 changed files: clean.check:phantom-deps—✅ Every in-scope import is declared by the package that publishes it.check:self-import—✅ No package names itself inside its own src/.check:doc-fences—✅ every TypeScript block in 223 document(s) is fenced ts/tsx/typescript…check:doc-types—✅ Every documented component type is registered.check:doc-snippets—Semantic phase: 267 of 267 block(s) judged, 0 failed.check:readme-exports—✅ OK (… 378 self-imports judged (378 real, 0 wrong-path, 0 fabricated) … 0 unbuilt …)check:esm-specifiers,check:node-esm-load,check:entry-guard,check:published-dist,check:pre-install-import-graph,check:vi-mock-specifiers,check:eager-closure,check:docs-route-closure,check:shell-escape-residue,check-doc-links— all exit 0.one-authority-per-exported-name(theKNOWN_COLLISIONSshrink-only baseline):Tests 11 passed (11). No baseline was extended — nothing here adds an exported name, and adding aline to a shrink-only baseline is a maintainer decision, not a remedy a dev may take even when
a gate prints it ready to paste.
✅ No changeset declares amajorbump.Two gates first came back non-zero as prerequisite-not-met, not verdicts, and are recorded
that way:
check:doc-snippetsexited 2 saying in as many words "This is 'I could not run',NOT 'I ran and found errors'", and
check:readme-exportslisted only "type entry … is not ondisk". Both were unbuilt-
distpreconditions; after building what they name, both are greenabove.
Scope
Only
evaluateVisibility's reporting changed. #6444 and #6445 — the other two of the #6038census trio — are untouched and keep their own cards; #6444 is not addressed here, and the
${…}reading above is that defect being transferred away from at this one site, not fixed.skills/**is not touched by this diff, so the published-skill budget clause does not apply.Generated by Claude Code