Skip to content

Commit 1c5b902

Browse files
committed
fix(spec): refuse requiresFeature composed with a blank-source CEL 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>
1 parent f962be9 commit 1c5b902

3 files changed

Lines changed: 79 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: 24 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,21 @@ 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+
message:
393+
'`requiresFeature` composes only with a CEL `visible` carrying a NON-BLANK `source`; this '
394+
+ '`source` is blank after trimming, so the composition would parenthesise nothing — '
395+
+ `\`( ) && ${gate}\` — which no CEL parse accepts on any scope. The gate would fault at `
396+
+ 'evaluation instead of gating: shown regardless of the flag where the surface is fail-soft, '
397+
+ 'hidden regardless of it where it is fail-closed. Either way the flag decides nothing, which '
398+
+ 'is the inert arrival ADR-0078 rejects. Drop the blank `visible` and `requiresFeature` emits '
399+
+ `\`${gate}\` alone, or put the predicate the gate should compose with in \`source\`.`,
400+
});
401+
return rest as Omit<T, 'requiresFeature'>;
402+
}
379403
return {
380404
...rest,
381405
visible: { ...existing, source: `(${existing.source}) && ${gate}` },

0 commit comments

Comments
 (0)