Skip to content

fix(spec): refuse requiresFeature composed with a blank-source CEL visible (#17631) - #18575

Merged
os-bill merged 2 commits into
mainfrom
claude/issue-17631-blank-source-requiresfeature
Sep 17, 2026
Merged

os-bill merged 2 commits into
mainfrom
claude/issue-17631-blank-source-requiresfeature

Conversation

@os-bill

@os-bill os-bill commented Sep 17, 2026

Copy link
Copy Markdown
Collaborator

Fixes #17631

Clause-②: no

lowerRequiresFeature lowers the requiresFeature: 'FLAG_NAME' sugar into the canonical visible CEL predicate, and its own docblock states the ADR-0078 rule it enforces: a composition that could never take effect is a loud parse error, not a silent one. The guard enforcing that rule tested the type of sourcetypeof existing.source !== 'string' — so a whitespace-only source, legal on ExpressionSchema (the persistence contract, whose min(1) whitespace clears), passed it and the feature gate was composed around a blank operand. The guard produced exactly the shape it exists to reject.

Measured, this branch, on f962be9d08

The card's four-row probe on ActionSchema, reproduced before touching anything — all four rows came out as filed:

case before (f962be9d08) after
ast-only visible, NO requiresFeature parses=true, envelope verbatim unchanged
ast-only visible, WITH requiresFeature parses=false (AST-only refusal) unchanged
blank-src visible, NO requiresFeature parses=true, {"dialect":"cel","source":" "} unchanged — the persistence contract is not narrowed
blank-src visible, WITH requiresFeature parses=true, source: "( ) && features.organization != false" parses=false, loud refusal at requiresFeature

And the composed predicate, through @objectstack/formula celEngine.evaluate, with a control leg differing only in the source:

"(   ) && record.x == 1"             ok=false  kind=parse  "Unexpected token: RPAREN"
"(record.x == 1) && record.x == 1"   ok=true   value=true        # CONTROL, identical composition, real source

So at render the gate faults instead of gating: fail-soft surfaces show the element regardless of the flag, fail-closed surfaces hide it regardless of the flag. Either way the flag decides nothing.

The repair, and why it is this one and not the wider one

Two repairs were on the table and they differ in blast radius. This PR takes the guard, on the merits, not because it is the smaller diff:

What changed

  • packages/spec/src/kernel/public-auth-features.ts — the lowering refuses a source that is blank after trimming, on the same leg as the AST-only refusal above it. The notion of blank is source.trim(), the one the engine's own helpers apply, so a source merely padded around real text still composes verbatim. The refusal names the composition it would have produced and both exits (drop the blank visible and the sugar emits the gate alone; or put the predicate the gate should compose with in source).
  • packages/spec/src/kernel/public-auth-features.test.ts — the new refusal is pinned in the same shape as the AST-only pin beside it (issue kind and subject, never the wording), across three blank spellings, plus the assertion that no envelope is produced. A second pin fixes the boundary the refusal must not cross: a padded-but-authored source still composes.
  • .changeset/17631-requires-feature-blank-source.mdpatch.

Reverse verification

Committed first, then mutated, then restored — the mutation proven on disk by occurrence count and by blob hash, the restore proven by blob hash equality with HEAD plus an empty git diff HEAD, under a trap ... EXIT INT TERM with absolute paths. The test resolves its subject through a relative ./public-auth-features import, not through the package exports, so no rebuild sits between the edit and the verdict.

Mutation: if (existing.source.trim().length === 0) { -> if (false) { (the pre-fix behaviour). Predicted direction: red.

anchor occurrences BEFORE mutation: 1
anchor occurrences AFTER mutation: old=0 new=1
MUTATED_BLOB=81814bc9d6e408eae96650bac701fda5ee38841f (differs from HEAD: yes)

 FAIL  src/kernel/public-auth-features.test.ts > lowerRequiresFeature
       > rejects a CEL visible whose source is blank after trimming (ADR-0078)
 AssertionError: expected [] to have a length of 1 but got +0
 Test Files  1 failed (1)
      Tests  1 failed | 23 passed (24)

RESTORED ok (blob 59e27e146a0ecf5d0dfa61f151320640878cb2d8, git diff HEAD empty)

Exactly the new pin went red and the other 23 stayed green, so the mutation is targeted and the pin is discriminating.

One gate finding this change caused, and where it was fixed

The refusal was first written interpolating the composed gate. That put featureGatePredicate — and through it the whole PUBLIC_AUTH_FEATURES registry — in a customer-facing message: position, and check:doc-authoring's per-module fixed point then swept that registry's internal notes / exempt.reason prose as customer-facing text, flagging three pre-existing strings (:197, :220, :230).

Measured with a control leg rather than assumed: the same gate is green with packages/spec/src/kernel/public-auth-features.ts reverted to f962be9d08 (blob equality asserted both ways), and red with the interpolation in. So the red was this diff's, not a pre-existing one.

Fixed at the cause — the sentence now names the composed shape without interpolating the gate, which is also how the two refusals beside it are written — rather than by editing three unrelated strings in the registry. pnpm check:doc-authoring is green again: 15502 customer-facing strings across 993 spec sources clean.

Acceptance notes

  • Noted, not filedcheck:doc-authoring's hoisted-const fixed point is a per-module over-approximation: one field of a const reaching a message: position (semantics, here) makes every string in that const customer-facing, including registry notes no customer can ever see. The gate's own output declares this spelling ("hoisted text-sink consts (fixed-point, per module)"), so it is designed behaviour rather than a contract violation, and it is cheap to work around at the call site. Successor: none identified — no queued card touches this gate's climb.
  • Declared gap, not a deviation hidden — the AST-only leg is pinned twice, in kernel/public-auth-features.test.ts and in ui/action.test.ts. This PR adds the first of those two. ui/action.test.ts is declared read-only for this card, so the schema-level sibling pin is not in this diff; the schema-level behaviour is measured in the table above instead. A seat holding that file can add it in one it block.

Gates

Derived for the actual changed paths with node scripts/pm/dispatch-gates.mjs --repo objectstack-ai/objectstack --commands (three-dot against the merge base; committed + working tree + untracked), then reconciled with --ran carrying every exit code. Each exit code landed to disk before being read — never through a pipe.

Run reconciliation — 83 derived, 83 run, 0 NOT-MEASURED, 0 UNRUN.
✓ dispatch-gates --ran: 83 derived famil(ies) accounted for — 83 run,
  0 NOT-MEASURED (a DERIVED zero — all 83 recorded an exit code and none of them is 3).
  • 82 of 83 green, including check:api-surface, check:authorable-surface, check:docs, check:liveness, check:adr-0087-registration, check:changeset-no-major, check:nul-bytes, check:doc-authoring, check:type-check-coverage, check:type-check-debt, check:test-source-alias, check:dispatcher-error-vocabulary, check:where-matcher.
  • Four of them first answered exit 3 (PREREQUISITE NOT MET — not a finding) because they read built output: check:doc-formula-expressions, check:dual-build-cjs-loads, check:lean-entry-closure, check:type-check-debt. The workspace closure was built (turbo run build --filter='./packages/*' --filter='./packages/*/*', 72/72 successful) and all four then exited 0. They are measured, not declared away.
  • pnpm --filter @objectstack/spec typecheck green; pnpm --filter @objectstack/spec test green — 483 files, 13775 tests — both re-run at the final head, after the refusal sentence changed.
  • pnpm --filter @objectstack/spec check:generatedall 15 generated artifacts up to date, nothing to regenerate.
  • pnpm lint (eslint . --no-inline-config, the whole repo, no narrowing) green at ec0eaed2d2, the final commit.
  • node scripts/pm/check-clause2-carriers.mjs --pair 18575exit 0: the clause-② declaration is readable in the fixed spelling, both carriers agree, and the diff carries no widening tell. No needs:contract-review label is on this PR; hanging or clearing it is the seat's, not this PR's.
  • The one red: pnpm check:cross-package-test-inputs exit 1, signature @objectstack/cli descends a directory tree from packages/spec/dist/. That is the known defect already carried by [finding] check:cross-package-test-inputs passes in CI and fails on a built tree — its verdict is a function of gitignored build state #18353 and [finding] check:cross-package-test-inputs answers 1 or 0 depending on whether packages/spec has been BUILT — the author who follows AGENTS.md is the only one who sees the red, and CI never does #18440 — it exits 1 on any tree where packages/spec has been built and 0 when it has not, ablated both ways by two devs this session. Not filed again, and unrelated to this diff.

维护者速读(草稿)

改了什么 —— requiresFeature 这个语法糖在和一个 source 只有空白字符的 visible 组合时,会生成 ( ) && features.X != false 这种任何作用域下都解析不了的谓词。现在它当场报错拒绝,而不是悄悄合成一个永远跑不起来的门。

为什么改 —— 这正是 lowerRequiresFeature 自己文档里援引 ADR-0078 要拒绝的形态:声明解析通过、却什么都不改变。作者以为加了特性开关,实际上渲染时谓词直接解析失败 —— 容错的界面把元素照常显示出来(开关形同虚设),严格的界面无论开关如何都把它藏起来。两种结局都是开关不起作用。

风险与代价(含回滚) —— 这是一次接受集收窄:今天能解析通过的一种写法,以后会被拒绝。但被拒绝的那种写法在运行时本来就必然失败,所以没有任何能正常工作的写法被破坏,也就没有迁移负担 —— 拒绝信息本身就是处方。ExpressionSchema / ExpressionInputSchema(持久化契约)一个字节没动,不带 requiresFeature 的空白 source 照旧解析通过。回滚 = 撤销这个 PR,没有数据迁移、没有生成物、没有已发布的注册表条目参与。

席位意见 ——

你要做的 —— 无需操作。这是一个 patch 级修复,不触及受管面,按常规队列落地即可。


Generated by Claude Code

…L `visible`

`lowerRequiresFeature`'s guard tested the TYPE of `existing.source`, so a
whitespace-only source — legal on the persistence contract, whose `min(1)`
whitespace clears — passed it and the feature gate was composed around a blank
operand, yielding `(   ) && features.X != false`. That predicate parses on no
scope at all, so the gate faults at evaluation instead of gating: the flag
decides nothing whichever way the consuming surface fails. That is the
parses-clean-changes-nothing arrival the guard's own docblock cites ADR-0078 to
reject, produced by the composition step itself.

The lowering now refuses a `source` that is blank after trimming, on the same
leg as the AST-only refusal, with a refusal naming the composition it would
have produced and both exits. The notion of blank is `source.trim()`, the one
the engine's own helpers apply, so a merely padded source still composes.

Claude-Session: https://claude.ai/code/session_01JbZnqu8bt6YqfJsr9vaFb3
Co-authored-by: Claude <noreply@anthropic.com>
…g`'s sink closure

Interpolating the composed gate into the refusal put `featureGatePredicate` —
and through it the whole `PUBLIC_AUTH_FEATURES` registry — in a customer-facing
`message:` position, and that gate's per-module fixed point then swept the
registry's internal `notes` / `exempt.reason` prose as customer-facing text.
Measured: `pnpm check:doc-authoring` is green at the base commit and flagged
three pre-existing strings with the interpolation in.

The sentence now names the composed shape without interpolating the gate, which
is also how the two refusals beside it are written. No test pinned the wording.

Claude-Session: https://claude.ai/code/session_01JbZnqu8bt6YqfJsr9vaFb3
Co-authored-by: Claude <noreply@anthropic.com>
@github-actions github-actions Bot added size/s documentation Improvements or additions to documentation tests tooling labels Sep 17, 2026
@github-actions

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

This PR changes 1 package(s): @objectstack/spec, touching 2 documentable anchor(s).

3 hand-written doc(s) NAME something this change touched and may need an implementation-accuracy re-verification:

  • content/docs/deployment/troubleshooting.mdx (via requiresFeature (literal, a string literal in lowerRequiresFeature))
  • content/docs/protocol/objectui/actions.mdx (via requiresFeature (literal, a string literal in lowerRequiresFeature))
  • content/docs/ui/actions.mdx (via requiresFeature (literal, a string literal in lowerRequiresFeature))

1 release-owned page(s) also name something this change touched. These are read-only:

  • content/docs/releases/v15.mdx (via requiresFeature (literal, a string literal in lowerRequiresFeature))

content/docs/releases/ is RELEASE-OWNED (AGENTS.md "Documentation Guardrails"): release
notes are written centrally at release time, and a code PR that edits them is the exact PR
that guardrail exists to stop. They are still audited — read-only. If one of them is actually
wrong, file an issue or open a dedicated docs-only PR; do not edit it here.

What this run could not see
  • the SDK route bridge reached 60 of 215 client-bound route-ledger rows — the other 155 have no registrar path: tail to select them, so pages documenting THEIR client methods cannot appear above, on this or any run. Of those 155: 0 are remediable by widening that discovery convention (an in-repo file declares the path; the convention did not scan it); 55 are structural — on a ledger where NOT ONE row is declared in-repo, so no discovery change reaches them at any price; 100 are undecided (no in-repo declaration, on a ledger that has other in-repo registrars — absence and an unreadable spelling are not distinguishable here). The rows themselves: node scripts/docs-audit/affected-docs.mjs --bridge-coverage
  • 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 — 136 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 298e9dde15e4e7c3b340323215e084fd60107cc3packageMentionDocs.

Which tree this was computed on

This run read content/docs from f5553b5de5a9e0debb0c0c3645b07f984da4a8d3 — the merge of head ec0eaed2d2053cd63da2db9c5efd0a17ba9e5e9c into base 298e9dde15e4e7c3b340323215e084fd60107cc3, which is what actions/checkout gives a pull_request run. Not the PR head.

A worktree cut from an older main holds a different content/docs, so re-deriving there can legitimately return a different list — that is a different tree, not a wrong row. To answer on the same tree:

# while this PR is open — GitHub drops the merge commit once it closes
git fetch origin f5553b5de5a9e0debb0c0c3645b07f984da4a8d3 && git checkout f5553b5de5a9e0debb0c0c3645b07f984da4a8d3
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin 298e9dde15e4e7c3b340323215e084fd60107cc3 ec0eaed2d2053cd63da2db9c5efd0a17ba9e5e9c && git checkout -B drift-repro 298e9dde15e4e7c3b340323215e084fd60107cc3 && git merge --no-ff ec0eaed2d2053cd63da2db9c5efd0a17ba9e5e9c

node scripts/docs-audit/affected-docs.mjs --json 298e9dde15e4e7c3b340323215e084fd60107cc3

⚠️ That checkout carried uncommitted changes, so the commit above does not fully identify what was read.

Advisory only, and a precision-first one (#9192): a page is listed because it names a
symbol, wire route or SDK method this diff touched — not because it mentions a changed
package. Each row says which anchor put it there, so a wrong row is reportable rather than
merely annoying. To re-verify, run the docs-accuracy-audit workflow scoped to these files:
node scripts/docs-audit/affected-docs.mjs 298e9dde15e4e7c3b340323215e084fd60107cc3 → pass the list as
args.docs, on the commit named under Which tree this was computed on.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation size/s tests tooling

Projects

None yet

2 participants