Uh oh!
There was an error while loading. Please reload this page.
fix(app-shell): diagnose in with a path on the right in metadata-admin predicates - #6652
Merged
Merged
Conversation
…min predicates The predicate evaluator matches membership as `path in ['a','b']` — the right side must be a bracketed literal set. A membership test whose right side is a PATH never matched that branch, carried no `==`/`!=` either, and fell to the bare-truthy tail where the WHOLE text was evaluated as one operand. `'admin' in current_user.positions` — ADR-0068's own headline example and the spelling `SelectOptionSchema`'s docblock names as the canonical use of the key — leads with a quote, so `parseLiteral`'s tail handed it back verbatim as a non-empty string: TRUE for every user, whatever `positions` held. Fail-OPEN, so a gate meant for admins rendered for everyone. `data.roles in current_user.positions` walked off the draft mid-path and read FALSE for every row instead. Both were silent: objectstack#6936's warning hangs on `resolveValue`'s path branch, which quote-leading text never enters, and objectui#4049's `PATH_SHAPED_LITERAL` only matches text starting with an identifier character. Diagnose only, zero semantic change — the third time this file takes the posture #4049 and #4266 took. The hook sits at the bare-truthy tail, AFTER the `in` branch has declined the text, and asks one question: does the text nonetheless carry a top-level `in`? Nothing new is resolved and no operand handling is added. The whole executable footprint is a fourth warn-once Set, a pure `carriesTopLevelIn` predicate, a void warn function, and one `if` in `evalExpr`. The detection reuses `splitTopLevel`'s existing quote-aware `inStr` walk rather than a second scanner, so a predicate that is itself a quoted literal containing the word (`'plug in adapter'` — correct code, a truthy string) is never accused; a naive `includes(' in ')` would report it as broken, which is a worse defect than the bug. Both directions pinned. The message names the spelling, names the supported subset, and states plainly that a path on the right of `in` cannot be written on this surface today, rather than implying some other punctuation would work. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CRJge11jso9TpXRWFt1Z49
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-sales
marked this pull request as ready for review
August 28, 2026 09:28
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#6617
Direction 1 (diagnose only), as graded by triage. Zero verdict change — no expression that evaluates today reaches a different answer, and that is measured below rather than asserted.
The gap
evalExpr'sinbranch matches/^(.+?)\s+in\s+(\[.*\])$/: the right side must be a bracketed literal set. A membership test whose right side is a path therefore never reaches that branch. Carrying no==/!=either, it falls all the way to the bare-truthy tail, where the whole text is handed toresolveValueas one operand:'admin' in current_user.positionsresolveValue's literal shortcut hands it toparseLiteral; the quoted-string branch declines it (starts with a quote, does not end with one) and the tail returns it verbatimpositionsholdsdata.roles in current_user.positionsresolveValuesplits on dots intodata/roles in current_user/positionsand walks off the draft at segment twoThe first is ADR-0068's own headline example and the spelling
SelectOptionSchema's docblock names as the canonical use of the key, and its failure direction is permissive: an option, field or section gated to admins renders for everyone.Both were silent. objectstack#6936's warning hangs on
resolveValue's path branch, which quote-leading text never enters; objectui#4049'sPATH_SHAPED_LITERALonly matches text starting with an identifier character.Premise re-verified on the current ref, not inherited
The feasibility note was measured on
origin/main@bac7ba43. I re-measured all of it on9101be57(this branch's base) with a throwaway probe before writing a line — 20 rows, direction predicted before each run, 20/20 confirmed, probe then deleted.The PM's correction on PR #6618 holds, and I measured it directly. These four rows are identical —
TRUE, zero warnings — in all four worlds:'admin' in current_user.positionswith…current_user.positions = ['admin']current_user.positions = ['viewer']current_user.positions = []current_usernot bound at allThe root is never resolved, so binding or not binding
current_usercannot change the outcome andPATH_SHAPED_LITERALnever fired for it either. This gap is not a regression from #6247; it is a property of the operator's grammar, not of which names the scope declares.The change
The hook sits at the bare-truthy tail, after the
inbranch has already declined the text, and asks one question: does the text nonetheless carry a top-levelin? The entire executable footprint is:Set(+ its line inresetPredicateWarnings),const IN_OPERATOR = ' in ',carriesTopLevelIn(expr),voidwarn function guarded byisDev()and the memo,evalExpr:if (carriesTopLevelIn(expr)) warnInWithoutLiteralSet(expr, source);Nothing new is resolved. No operand handling is added.
git diffshows exactly one removed line inpredicate.ts— the comment// Bare truthy check, replaced by a longer one — and no existing executable line changed. That is triage's distinguishing test for direction 2 answered structurally: the evaluator is not taught to resolve anything.The trap: the detection is quote-aware by REUSE
A naive
expr.includes(' in ')is wrong.'plug in adapter'is a bare quoted literal — correct code, a truthy string — and accusing it would be a false statement about the author's code, the precise failure this file's diagnostics exist to prevent.carriesTopLevelInissplitTopLevel(expr, IN_OPERATOR).length > 1— the file's existinginStr/depthwalk, the same one the&&/||splitters andfindUnparseableSetElementalready run. Inside a quoted run it never even tests the operator. No second scanner, which is also what keeps this a detection rather than a parser.Why there is no other false positive: an expression reaching the bare-truthy tail that is correct is either a path (which cannot contain a space) or a literal — and the only literal that can contain
' in 'is a quoted string, which theinStrwalk protects.Two spellings are deliberately not detected and fail to silence (the status quo), never to a false claim: a tab-separated
in, and one nested atdepth > 0inside parentheses. Both are pinned as silent.The message
Names the offending text and the predicate that carried it, names the supported subset
path in ['a','b'], names the fail-open direction, and states plainly that a path on the right ofincannot be written on this surface today — "not with different punctuation, not with a different spelling, not at all" — rather than implying some other spelling would work. The author's next question is "then how do I write it?", and the honest answer here is "you cannot".No overlap with #4266, and neither can mask the other
Mutually exclusive by construction: #4266 fires from
parseLiteral's array branch, reachable only for text that already matched an operator branch; this one fires at the bare-truthy tail, reachable only when every operator branch declined. Pinned four ways in §9.6 — a #4266 predicate fires #4266 only, a #4049 predicate fires #4049 only, a #6617 predicate fires #6617 only, and one predicate carrying two gaps reports both.toContain('objectui#4049')is true of #6617's message too. Written the naive way the exclusivity test passed for the wrong reason in one direction and failed spuriously in the other — it did, on first run, and that is why the constants exist.Verification — union run at
ac5a59c2Every result below was produced at the final commit; each exit code was captured before any pipe, and each line quotes the gate's own verdict.
Suites — repo root, all vitest projects (the package-scoped form is a known false-green here).
--project unitalone silently ran only 1 of the 3 files, since the twoSchemaFormsuites are.tsxand live in another project; re-run without it:That is the full blast radius:
./predicatehas exactly three importers in the repo. The 94 pre-existingpredicate.test.tstests and bothSchemaFormsuites pass untouched — the test file diff is306 insertions, 0 deletions.Gates (each printed its own verdict line):
@object-ui/app-shelltype-check> tsc --noEmit && tsc -p tsconfig.test.jsondist/*.d.tscheck:changeset-presence✅ 2 source file(s) of 1 released package(s) changed, and this change declares 1 changeset(s)check:changeset-no-major✅ No changeset declares a major bump.check:changeset-fixed✅ All workspace packages are in the changeset fixed group.check:changeset-overwrite✅ No pre-existing changeset was modified or deleted.check:control-bytes✅ OK (scanned 5496 tracked text file(s))check:vi-mock-specifiers✅ OKcheck:i18n-keyscheck:self-import✅ No package names itself inside its own src/.check:shell-escape-residue✅ OKThe changeset gate ruled the bump:
packages/app-shell/src/**is guarded, so a declaration is required, and it is an honestpatchon@object-ui/app-shell. No gate refused anything.type-check actually covers the edited test file — not assumed:
tsc -p tsconfig.test.json --listFilesreportspredicate.test.ts1 occurrence andpredicate.ts1. (A packagetypecheckthat excludes**/*.test.tswould have been a true statement about nothing.)Lint — a declared narrowing, with its three measurements. Repo-wide
pnpm lintstays CI's run. Locally: eslint's own file selection forpackages/app-shell/src/views/metadata-admin/chose 380 files, count read from--format json, 0 errors; the two changed files show 0 errors and 1 pre-existingno-explicit-anywarning atpredicate.ts:680(let cur: any = ctxinresolveValue, untouched by this diff — confirmed absent fromgit diff). Invariance: this config enables no type-aware linting (noprojectService, noproject:, notypeChecked), so each file's verdict is a pure function of its own source plus the shared config — neither of which this diff changes for any file it does not contain.Proving zero-verdict-change instead of asserting it
In words. The single new statement is a
voidcall placed on a path that already existed. It reads nothing fromctx, writes only its own memoSet, returns nothing, andsplitTopLevelcannot throw on a string — so it cannot reach thecatchinevaluatePredicatethat would flip a verdict to fail-open. No operator branch was touched, no operand handling was added,resolveValueandparseLiteralare byte-identical. There is no channel by which an evaluated expression could reach a different answer.Measured — ablation B. Removing the diagnostic call entirely (mutation proved on disk by grep counts on both the injected and the removed text; restored via
git checkout HEAD -- PATHwith an absolute PATH, and proved bygit hash-objectequalling the HEAD blob22f014afandgit diff HEADempty):15 diagnostic tests go red and zero verdict rows do. The §9.3 table is genuinely independent of the diagnostic. Together with the pre-change probe on
9101be57(20/20 verdicts confirmed before the change), the verdicts are pinned identical on both sides of it.Measured — ablation A, that the quote-aware reuse is load-bearing. Swapping
splitTopLevel(...)for the naiveexpr.includes(IN_OPERATOR):Exactly the quote- and depth-protected controls, and nothing else. The trap pin discriminates: it would catch the naive implementation.
The both-directions pin
'admin' in current_user.positions(bound, unbound, empty and populated),'x' in data.tags,data.roles in current_user.positions, inside||, and with extra spaces aroundin.'plug in adapter',"plug in adapter",' in ',data.label == 'plug in adapter',data.label in ['plug in adapter','x'],"it in that",!'plug in adapter', and every supported literal-set form.Warn-once memo keying is
(sub-expression, predicate source)— the same${text}::${source}scheme as the three existing sets, for the stated reason: keyed on the text alone, a form with fifteen gates spelling the same membership test would report one and hide the rest. No fourth scheme invented.Out of scope, deliberately
Direction 3 (producer-side publish-time validation of predicate expressions) belongs to the objectstack#7010 family — another repo, another lane. No card opened from this surface, per the dispatch order; a note on whether it wants one is in my report.
Generated by Claude Code