Skip to content

Commit 497655f

Browse files
os-billclaude
andauthored
fix(spec): refuse requiresFeature composed with a blank-source CEL visible (#17631) (#18575)
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 `source` — `typeof 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: - **The defective string is produced here.** `( ) && features.X != false` is built by the composition step in `lowerRequiresFeature`. Contract-first says refuse at the producer; this is the producer. - **The invariant being restored is this function's own**, stated in its docblock and cited to ADR-0078 one bullet above the leg that was missing. - **It covers every slot that composes the sugar at once** — `ActionSchema.visible` and `ActionParamSchema.visible` today, and whatever composes it next — because the rule lives in the shared lowering rather than in either slot's declaration. - **The wider repair would narrow something this card does not own.** Composing `visible` onto `EvaluatedExpressionInputSchema` also refuses a blank-`source` `visible` with **no** `requiresFeature` beside it — row 3 above, which parses clean today by design. #17631's own filing says that is one of the per-slot decisions #15811's census enumerates and "may want to travel with that family rather than alone". - **And the `expression.zod.ts` spelling of it is refused by that file's own declaration**, not merely by scope: `EvaluatedExpressionSchema`'s docblock states "`ExpressionSchema` itself is NOT narrowed: it is the persistence contract". The evaluated-slot rule already exists there as `EvaluatedExpressionInputSchema`; nothing in that file needed to move, and nothing in it did. It was declared read-only for this card (in flight as #17849 / PR #18557) and was read, never edited. ## 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.md` — `patch`. ## 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 59e27e1, 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 filed** — `check: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:generated` — **all 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 18575` — **exit 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 **#18353** and **#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](https://claude.ai/code)_ --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent 29a36f0 commit 497655f

3 files changed

Lines changed: 88 additions & 0 deletions

File tree

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,23 @@
1+
---
2+
'@objectstack/spec': patch
3+
---
4+
5+
fix(spec): `requiresFeature` refuses a blank-`source` CEL `visible` instead of composing a predicate that can never parse (#17631)
6+
7+
Clause-②: no
8+
9+
`lowerRequiresFeature` lowers the `requiresFeature: '<flag>'` 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 that enforced it tested the TYPE of `source` (`typeof existing.source !== 'string'`), so a whitespace-only `source` — legal on `ExpressionSchema`, which is the persistence contract and whose `min(1)` whitespace clears — passed it and the gate was composed AROUND a blank operand:
10+
11+
```
12+
visible: { dialect: 'cel', source: ' ' } + requiresFeature: 'organization'
13+
→ { dialect: 'cel', source: '( ) && features.organization != false' }
14+
```
15+
16+
That predicate parses on no scope at all (`celEngine.evaluate` answers `kind: parse`, `Unexpected token: RPAREN`), 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 parses-clean-changes-nothing arrival the guard exists to reject, produced by the guard's own composition step.
17+
18+
The lowering now refuses a `source` that is blank after trimming, on the same leg as the AST-only refusal one line above, with a refusal that names the composition it would have produced and both exits (drop the blank `visible` and the sugar emits the gate alone; or write the predicate the gate should compose with). The notion of blank is `source.trim()` — the one the engine's own helpers apply — so a `source` that is merely padded around real text still composes verbatim.
19+
20+
- **Refused at the producer, not tolerated at a consumer.** No renderer gains a fallback for the unparseable predicate; the lowering stops emitting it.
21+
- **Both slots that compose the sugar inherit it**`ActionSchema.visible` and `ActionParamSchema.visible` — because the rule lives in the shared lowering rather than in either slot's declaration.
22+
- **`ExpressionSchema` / `ExpressionInputSchema` are NOT narrowed.** They remain the persistence contract, and a blank-`source` `visible` with no `requiresFeature` beside it still parses exactly as before. What is refused is the COMPOSITION, which is the thing that could never work.
23+
- **Nothing that functioned stops functioning.** The only authoring this refuses is one whose output faulted at CEL parse on every scope, so the migration is the refusal's own prescription and there is no working shape to port.

packages/spec/src/kernel/public-auth-features.test.ts

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -147,4 +147,36 @@ describe('lowerRequiresFeature', () => {
147147
expect(issues[0]).toMatchObject({ code: 'custom', path: ['requiresFeature'] });
148148
}
149149
});
150+
151+
// The same ADR-0078 leg, one step further in: a blank `source` is a STRING,
152+
// so it passes the type test above and used to compose into
153+
// `( ) && <gate>` — a predicate no CEL parse accepts on any scope, so the
154+
// flag decided nothing at render. Same shape as the pin above: kind and
155+
// subject, never the wording.
156+
it('rejects a CEL visible whose source is blank after trimming (ADR-0078)', () => {
157+
for (const visible of [
158+
{ dialect: 'cel', source: ' ' },
159+
{ dialect: 'cel', source: '' },
160+
{ dialect: 'cel', source: '\n\t' },
161+
]) {
162+
const { ctx, issues } = noIssues();
163+
const out = lowerRequiresFeature({ requiresFeature: 'admin' as const, visible }, ctx);
164+
expect(issues).toHaveLength(1);
165+
expect(issues[0]).toMatchObject({ code: 'custom', path: ['requiresFeature'] });
166+
// Refused, not composed — no `( ) && …` envelope is produced.
167+
expect(out.visible).toEqual(visible);
168+
}
169+
});
170+
171+
// The boundary the refusal must NOT cross: a source that is merely PADDED is
172+
// authored, and still composes verbatim (the engine trims its own input).
173+
it('still composes a source that has leading/trailing whitespace around real text', () => {
174+
const { ctx, issues } = noIssues();
175+
const out = lowerRequiresFeature(
176+
{ requiresFeature: 'admin' as const, visible: { dialect: 'cel', source: ' a ' } },
177+
ctx,
178+
);
179+
expect(issues).toHaveLength(0);
180+
expect(out.visible).toEqual({ dialect: 'cel', source: '( a ) && features.admin == true' });
181+
});
150182
});

packages/spec/src/kernel/public-auth-features.ts

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -333,6 +333,15 @@ type WithRequiresFeature = {
333333
* Drop one of the two rather than shipping a gate that reads as load-bearing.
334334
* - Existing `visible` that is non-CEL or AST-only → loud parse error
335335
* (ADR-0078 no-silently-inert); write the combined predicate by hand.
336+
* - Existing CEL `visible` whose `source` is blank after trimming → loud parse
337+
* error, for the same reason one step further in. `source` is `min(1)` on the
338+
* persistence contract and whitespace clears it, so a blank one is a *string*
339+
* and would compose: the result parenthesises nothing (`( ) && <gate>`) and
340+
* faults at CEL parse on every scope, so the gate decides nothing wherever
341+
* the consuming surface is fail-soft and hides the element regardless of the
342+
* flag wherever it is fail-closed — the inert arrival again, reached through
343+
* the one spelling that passes the type test above. The notion of blank is
344+
* `source.trim()`, the one the engine's own helpers apply.
336345
*
337346
* Designed as a zod `.transform((v, ctx) => lowerRequiresFeature(v, ctx))`
338347
* appended after the schema's refinements.
@@ -376,6 +385,30 @@ export function lowerRequiresFeature<T extends WithRequiresFeature>(
376385
});
377386
return rest as Omit<T, 'requiresFeature'>;
378387
}
388+
if (existing.source.trim().length === 0) {
389+
ctx.addIssue({
390+
code: 'custom',
391+
path: ['requiresFeature'],
392+
// ⚠ Deliberately does NOT interpolate `gate`. Doing so puts
393+
// `featureGatePredicate` — and through it the whole
394+
// `PUBLIC_AUTH_FEATURES` registry — in a customer-facing message
395+
// position, and `check:doc-authoring`'s per-module fixed point then
396+
// sweeps that registry's INTERNAL `notes` / `exempt.reason` prose as
397+
// customer-facing text (measured: green at the base commit, three
398+
// pre-existing strings flagged with the interpolation in). The concrete
399+
// gate is one `featureGatePredicate` call away for anyone who wants it.
400+
message:
401+
'`requiresFeature` composes only with a CEL `visible` carrying a NON-BLANK `source`; this '
402+
+ '`source` is blank after trimming, so composing the feature gate onto it would parenthesise '
403+
+ 'nothing — the predicate would read `( ) && ` followed by the gate — which no CEL parse '
404+
+ 'accepts on any scope. The gate would fault at evaluation instead of gating: the element is '
405+
+ 'shown regardless of the flag where the consuming surface is fail-soft and hidden regardless '
406+
+ 'of it where it is fail-closed, so the flag decides nothing — the inert arrival ADR-0078 '
407+
+ 'rejects. Drop the blank `visible` and `requiresFeature` emits the gate alone, or put the '
408+
+ 'predicate the gate should compose with in `source`.',
409+
});
410+
return rest as Omit<T, 'requiresFeature'>;
411+
}
379412
return {
380413
...rest,
381414
visible: { ...existing, source: `(${existing.source}) && ${gate}` },

0 commit comments

Comments
 (0)