Skip to content

Pin the receiver/member-call form of the unknown-function rejection — and report where the #13594 hole actually is - #13819

Merged
zhuangjianguo merged 1 commit into
mainfrom
claude/issue-13594-validate-unknown-cel-function
Aug 31, 2026
Merged

Pin the receiver/member-call form of the unknown-function rejection — and report where the #13594 hole actually is#13819
zhuangjianguo merged 1 commit into
mainfrom
claude/issue-13594-validate-unknown-cel-function

Conversation

@zhuangjianguo

Copy link
Copy Markdown
Collaborator

Part of #13594.

The headline: the card's premise did not reproduce, and the hole is at a different surface

#13594 reports that validateExpression (@objectstack/formula) returns ok: true for a CEL
source calling a function the engine does not have, for both the global-call and the
receiver/member-call form.

Measured, twice, on the artifact the card names and on this tree — it rejects both.

Published @objectstack/formula@17.2.0 + @marcbachmann/cel-js@8.0.0, installed clean from npm
and driven through validateExpression('predicate', ...):

"totallyBogusFn(1,2)"             ok= false  errs= 1  warns= 0   found no matching overload for 'totallyBogusFn(int, int)'
"record.x.nosuchmethod('a')"      ok= false  errs= 1  warns= 0   found no matching overload for 'dyn.nosuchmethod(string)'
"current_user.can(object, verb)"  ok= false  errs= 1  warns= 0   found no matching overload for 'dyn.can(dyn, dyn)'
"upper('a')"                      ok= true   errs= 0  warns= 0

Same answers from this worktree's source at merge-base 936aa2d3a. Twenty-five unknown-function
shapes were probed — global, receiver-on-dyn, receiver-on-literal, inside a macro body, inside
both ternary branches, behind and / or short-circuits, nested, in an index expression, wrong
arity — and every one is refused, with all five stdlib/builtin controls staying clean. The
mechanism is celEngine.compile reading cel-js's check() verdict, which has been in place since
the #1877 repair; its own comment says so.

So the reported defect is not at this entry point. It is at
@objectstack/lint's validate-visibility-predicates
— the gate that actually judges the
visibleWhen / action-predicate surface the card's evidence comes from. Measured side by side on
the same sources:

                                   lint gate                       validateExpression
totallyBogusFn(1,2)                CLEAN                           ok=false
record.x.nosuchmethod('a')         CLEAN                           ok=false
country === "USA"                  visibility-predicate-syntax     ok=false   (control: caught)
status == 'active'                 visibility-bare-identifier      ok=true    (control: caught)

That gate is parse-only by design, not by oversight. Its module note records a maintainer
ruling and explains that routing it through compile() / validateExpression "would silently
overturn" a deliberate decision, and a dedicated case pins it: "does NOT widen to type-checking
— the CEL-type blind spot stays a blind spot"
. Widening an error-level authoring gate from "does
not parse" to "does not type-check", on a surface whose predicates are overwhelmingly dyn, is a
contract decision with a ruling already on record pointing the other way. It is escalated, not
taken here.

What this PR does ship

The one thing that was genuinely missing and is not a contract question: a pin for the
receiver/member-call form.

Every unknown-function pin in packages/formula spelled the call as a global one — PRIOR(...)
in validate.test.ts (twice) and in cel-engine.test.ts, size(1) in the fault-classification
suite. Nothing anywhere asserted the receiver form. A regression that dropped only cel-js's
rcall arm — precisely the half the card warned would be easy to miss — would have kept the
entire suite green while validateExpression waved through a predicate the runtime then faults
on. On the action surfaces that fault is fail-closed and near-silent: ActionEngine.getActionsForLocation
and DeclaredActionsBar both evaluate with throwOnError: true and hide the action for every
user including ones holding the grant, leaving one deduped console.warn as the only signal.

Three cases added to packages/formula/src/validate.test.ts:

  1. the receiver/member-call form, pinned as the author sees it — cel-js names the receiver's
    static type, so the message reads dyn.nosuchmethod, not the source spelling;
  2. the invented capability method on current_user (the objectui#4421 predicate), which matters
    because current_user is a declared SCOPE_ROOT — the unbound-root check cannot catch it, so
    only the unknown call does;
  3. the global form and the upper('a') control in one case, deliberately: a pin asserting
    only the rejection would stay green under a change that rejected every call.

The block also records the measurement above, so the next reader does not re-derive it.

Tests only. Zero production bytes — the whole diff is one .test.ts file, so the publish
gate's accept set is byte-for-byte unchanged and no consumer behaviour moves.

Contract-review clause: NOT fired

The dispatch order made needs:contract-review conditional on shipping ERROR, because that would
narrow what the publish gate accepts. Nothing is narrowed here: ERROR is what already ships, and
this PR changes no production code. Declared from what was actually built.

Verification — all at final commit 29a596f7

  • pnpm --filter @objectstack/formula test25 files, 662 tests passed.
  • pnpm --filter @objectstack/lint test87 files, 2397 passed, 5 skipped (after
    pnpm --filter '@objectstack/lint^...' build; an unbuilt @objectstack/sdui-parser was
    failing 23 files to load beforehand, unrelated to this diff).
  • Ablation — neutered the single arm the pins ride on in celEngine.compile
    (if (checkResult && checkResult.valid === false) to if (false and ...)). Mutation confirmed
    on disk by anchored greps in both directions (injected marker 1, removed text 0) and a changed
    blob hash; the subject is reached by relative import (./validate, ./cel-engine), so no
    dist is on the resolution path and no rebuild leg applies. Result: 62 passed goes to 5
    failed / 57 passed
    — all three new cases red, alongside the two pre-existing [P1] Flow trigger conditions with unknown functions are silently skipped (no error, passes build) #1877 ones.
    Restore proved byte-identical to HEAD (git diff HEAD empty, blob 5170cfdc equals the HEAD
    blob).
  • Gate family re-derived from the real change set via scripts/pm/dispatch-gates.mjs --repo objectstack-ai/objectstack:
    21 families. 19 green by their own verdict lines (check-nul-bytes: OK (7594 text files),
    check-test-source-alias OK, check:published-files 69 packages, where-matcher 320 matchers,
    and the rest). 2 NOT MEASURED, by the gates' own words, not red:
    check-test-completeness (PREREQUISITE NOT MET — needs a saved turbo run test log) and
    check:dual-build-cjs-loads (PREREQUISITE NOT MET — needs a full pnpm build).
  • pnpm --filter @objectstack/formula typecheck passes, but does not cover this diff:
    packages/formula/tsconfig.json carries "exclude": ["**/*.test.ts"], and tsc --listFiles
    confirms 0 of 25 test files enter the program. Type-checked separately with tests included —
    src/validate.test.ts clean; the 17 errors that surface there are pre-existing and live in
    five other test files. Filed as a finding rather than repaired here.

skip-changeset: the diff publishes nothing from any package.


Generated by Claude Code

…ion rejection (#13594)

Every unknown-function pin in this package spelled the call as a GLOBAL one
(`PRIOR(...)`, `size(1)`), so a regression that lost only cel-js's receiver
arm would have kept the whole suite green while `validateExpression` passed a
predicate the runtime then faults on — fail-closed and near-silent on the
ObjectUI action surfaces.

Measured on the source tree and on the published `@objectstack/formula@17.2.0`
/ `@marcbachmann/cel-js@8.0.0`: both call forms are already rejected here and
`upper('a')` is still clean. Tests only; no production behaviour changes.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01F3jdziLbAPGeceVNmSox5L
@claude claude Bot added the skip-changeset PR has no user-facing published change; bypasses the changeset gate label Aug 31, 2026
@github-actions

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

Nothing in this diff resolved to a documentable surface (no symbol, route or SDK anchor derived from 0 changed package(s)), so this run has no opinion about the docs.

What this run could not see
  • a page that states a rule by its inputs shares no identifier with the emitter that implements the rule, so an emitter-only diff cannot list it — not on this run and not on any run. Measured on fix(driver-sql): emit varchar(maxLength) for a text field a declared index keys on #11430: content/docs/protocol/objectql/types.mdx documents the text-family column mapping by the ObjectQL type names it maps FROM (text / textarea / html) while the diff changed createColumn; it went unlisted, and it was the page that diff falsified, in four places. No shared token exists to detect this on, so a rule your change carries has to be re-read by hand in the pages that restate it.

Coarse fallback — 0 page(s) merely mention a changed package (the pre-#9192 predicate, kept for the deliberately-wide backstop): node scripts/docs-audit/affected-docs.mjs --json e72ded02381dd2704a30b01e680faf486e85ea97packageMentionDocs.

@github-actions github-actions Bot added the tests label Aug 31, 2026

Copy link
Copy Markdown
Collaborator Author

PM verdict: ACCEPT, pending green — and the falsification is the deliverable

Reviewed against the dispatch order (#13594 comment 5478562500). CI is still running; this review is independent of it.

⭐⭐ The card's premise did not reproduce, and that is the most valuable thing in this PR

#13594 asserts validateExpression returns ok: true for an unknown function, in both call forms. Measured two independent ways — the published @objectstack/formula@17.2.0 + @marcbachmann/cel-js@8.0.0 installed clean from npm, i.e. the exact artifact the card names, and this tree at merge-base 936aa2d3a — it rejects both, with upper('a') staying clean as the control. 25 unknown-function shapes probed (global, receiver-on-dyn, receiver-on-literal, inside a macro body, both ternary branches, behind and/or short-circuits, nested, index expression, wrong arity); every one refused; all five stdlib controls clean.

Measuring the published artifact — not just the working tree — is what makes this decisive. A tree-only reading would have left "maybe it's fixed since" open. This closes it: the card's own named version rejects.

⭐ It then located the actual defect instead of stopping at "premise falsified"

Stopping there would have been a defensible STOP. Going further is what makes the card actionable: the hole is at @objectstack/lint's validate-visibility-predicates — the gate that judges the visibleWhen / action-predicate surface the card's evidence actually came from. The side-by-side is the proof, and it carries its own controls:

source lint gate validateExpression
totallyBogusFn(1,2) CLEAN ok=false
record.x.nosuchmethod('a') CLEAN ok=false
country === "USA" visibility-predicate-syntax ok=false (control: caught)
status == 'active' visibility-bare-identifier ok=true (control: caught)

Two rows where the lint gate does catch things prove the gate was reached — so the two CLEAN rows are readings, not a dead harness.

⇒ The card diagnosed a real symptom (publish clean → runtime fault) and attributed it to the wrong entry point. Same class as #13416's wrong-file, one layer larger.

⭐⭐ Escalating rather than fixing was correct, and the evidence for that is on record

The lint gate is parse-only by design: its module note records a maintainer ruling and states that routing it through compile() / validateExpression "would silently overturn" a deliberate decision, with a dedicated case pinning "does NOT widen to type-checking — the CEL-type blind spot stays a blind spot."

Widening an error-level authoring gate from "does not parse" to "does not type-check", on a surface whose predicates are overwhelmingly dyn, is a contract decision with a ruling already pointing the other way. ⛔ A seat taking that on its own would have been exactly the overreach the dispatch order's STOP conditions exist to prevent. Escalated, not taken — correct. I am routing it to the maintainer on the card.

What ships is the one non-contract thing that was genuinely missing

Every existing unknown-function pin in packages/formula spelled the call as globalPRIOR(...) twice in validate.test.ts and once in cel-engine.test.ts, size(1) in the fault-classification suite. Nothing asserted the receiver form. A regression dropping only cel-js's rcall arm — precisely the half the card warned is easy to miss — would have kept the entire suite green while the runtime faulted fail-closed and near-silent.

Two details worth naming:

  • ⭐ Case 3 pins the global rejection and the upper('a') control in one case, deliberately: "a pin asserting only the rejection would stay green under a change that rejected every call." Control discipline inside a single test.
  • ⭐ Case 2 pins current_user.can(object, verb) — the objectui#4421 predicate — because current_user is a declared SCOPE_ROOT, so the unbound-root check structurally cannot catch it and only the unknown-call check can. That is a pin chosen for what it can uniquely fail on.

Clause ② — correctly declared NOT fired

The order made needs:contract-review conditional on shipping ERROR. Nothing is narrowed: ERROR is what already ships, and the diff is one .test.ts file with zero production bytes. Declared from what was actually built, which is the rule.

Ablation and the incidental finding

Ablation neutered the single arm the pins ride on in celEngine.compile, proved the mutation on disk in both directions plus a changed blob hash, and got 62 passed → 5 failed / 57 passed — all three new cases red alongside the two pre-existing #1877 ones. Restore proved blob-identical to HEAD. ⭐ Correctly notes no rebuild leg applies: the subject is reached by relative import, so no dist is on the resolution path.

⚠️ Incidental finding worth carrying: packages/formula/tsconfig.json excludes **/*.test.tstsc --listFiles confirms 0 of 25 test files enter the program, and type-checking them separately surfaces 17 pre-existing errors across five other test files (validate.test.ts itself clean). That is the same defect class as #13676, which I filed this round for packages/objectql. Reported rather than repaired here — right call, and the two should be graded together.


Generated by Claude Code

@zhuangjianguo
zhuangjianguo marked this pull request as ready for review August 31, 2026 14:19
@zhuangjianguo
zhuangjianguo added this pull request to the merge queue Aug 31, 2026
Merged via the queue into main with commit dbc3321 Aug 31, 2026
37 checks passed
@zhuangjianguo
zhuangjianguo deleted the claude/issue-13594-validate-unknown-cel-function branch August 31, 2026 14:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/s skip-changeset PR has no user-facing published change; bypasses the changeset gate tests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants