Conversation
There was a problem hiding this comment.
Pull request overview
This PR shifts expression complexity limiting from CEL parser limits toward a dedicated checker lint rule, then wires lint enforcement into SELRuntime so runtime and checker behavior align (including unknown-variable strictness).
Changes:
- Added a new
expressionComplexityrule (with tests) and exposed it via the checker rules facade/default exports. - Updated
SELRuntimeto run checker lint rules during evaluation, throwingSELLintErrorfor error-severity diagnostics and surfacing warning/info diagnostics onEvaluateResult. - Simplified runtime limits to execution-only (
maxRounds/maxCalls) and updated runtime/checker alignment tests for stricter unknown-variable handling.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/sel-runtime/test/integration/environment-alignment.spec.ts | Updates alignment expectations so unknown variables are rejected by both runtime and checker. |
| packages/sel-runtime/src/execution/types.ts | Extends EvaluateResult with optional advisory diagnostics. |
| packages/sel-runtime/src/errors/errors.ts | Adds SELLintError carrying originating diagnostics. |
| packages/sel-runtime/src/environment/types.ts | Narrows SELLimits to execution limits and lets runtime config accept checker rule options. |
| packages/sel-runtime/src/environment/environment.ts | Integrates lint checking into evaluation and threads advisory diagnostics into results. |
| packages/sel-runtime/src/environment/environment.spec.ts | Updates tests for typed context and new limits semantics. |
| packages/sel-checker/src/rules/facade.ts | Exposes expressionComplexity via the public rules facade. |
| packages/sel-checker/src/rules/defaults/index.ts | Re-exports the new default rule module. |
| packages/sel-checker/src/rules/defaults/expression-complexity.ts | Implements AST complexity metrics + diagnostics. |
| packages/sel-checker/src/rules/defaults/expression-complexity.spec.ts | Adds unit tests for all complexity thresholds and spans. |
| packages/sel-checker/src/environment/register-types.spec.ts | Updates checker env construction defaults for variable strictness. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -138,13 +130,16 @@ export class SELRuntime { | |||
| const { env, contractBindings, codecRegistry } = createRuntimeEnvironment( | |||
| this.schema, | |||
| handler, | |||
There was a problem hiding this comment.
createRuntimeEnvironment is now called without CEL limits, and the SELLimits type no longer exposes parser/AST limits. The new expressionComplexity lint rule runs after parsing, so it can’t prevent resource-heavy parses (potential DoS / runaway CPU/memory) the way parser limits did. Consider keeping CEL limits pass-through (or providing a separate config) so complexity is bounded at parse time as well as by lint diagnostics.
| handler, | |
| handler, | |
| // Pass through CEL parser/AST limits (if configured) so parsing is bounded | |
| limits && (limits as any).cel ? { limits: (limits as any).cel } : undefined, |
| this.checker = new SELChecker(config.schema, { | ||
| rules: config.rules, | ||
| }); |
There was a problem hiding this comment.
SELChecker is instantiated unconditionally. When config.rules is undefined/empty (the documented default), planExecution() still calls checker.check() which re-parses and re-typechecks the expression even though no rules will run. Consider lazily constructing the checker only when rules are provided, and skipping lint execution entirely when there are no rules to apply.
| this.checker = new SELChecker(config.schema, { | |
| rules: config.rules, | |
| }); | |
| if (Array.isArray(config.rules) && config.rules.length > 0) { | |
| this.checker = new SELChecker(config.schema, { | |
| rules: config.rules, | |
| }); | |
| } |
| // Run lint rules via checker | ||
| const lintResult = this.checker.check(expression); | ||
|
|
||
| // Enforcement: error-severity diagnostics cause throw | ||
| const lintErrors = lintResult.diagnostics.filter( | ||
| (d) => d.severity === "error", | ||
| ); | ||
| if (lintErrors.length > 0) { | ||
| throw new SELLintError(lintErrors); | ||
| } | ||
|
|
||
| // Advisory: warning/info diagnostics pass through to result | ||
| const diagnostics = lintResult.diagnostics.filter( | ||
| (d) => d.severity !== "error", | ||
| ); | ||
|
|
There was a problem hiding this comment.
planExecution() parses/type-checks via this.env.parse() + parseResult.check(), then calls this.checker.check(expression) which parses/type-checks again in a second CEL environment. This doubles work for every evaluation and can become a bottleneck. If possible, reuse the already-parsed AST (and existing type-check result) to run rules, or expose a checker API that accepts a pre-parsed AST.
| describe("config", () => { | ||
| it("accepts limits configuration", async () => { | ||
| const env = new SELRuntime({ | ||
| schema: buildSchema({}), | ||
| limits: { maxAstNodes: 500, maxDepth: 20 }, | ||
| limits: { maxRounds: 5, maxCalls: 50 }, | ||
| }); | ||
| expect((await env.evaluate<bigint>("1 + 2")).value).toBe(3n); | ||
| }); |
There was a problem hiding this comment.
Runtime lint integration adds new behaviors (throwing SELLintError on error-severity diagnostics and returning non-error diagnostics on EvaluateResult.diagnostics), but there are no corresponding assertions in this spec. Add tests that (1) enable a rule, (2) verify warnings propagate via diagnostics, and (3) verify error-severity diagnostics reject with SELLintError and include the originating diagnostics.
| if (metrics.calls > resolved.maxCalls) { | ||
| diagnostics.push( | ||
| context.reportAt( | ||
| 0, | ||
| span, | ||
| `Expression complexity: contract calls ${String(metrics.calls)} exceeds maximum of ${String(resolved.maxCalls)}.`, | ||
| ), | ||
| ); |
There was a problem hiding this comment.
The diagnostic messages use plural subjects with singular verbs (e.g. "contract calls … exceeds"). Adjust wording for correct agreement (e.g. "calls … exceed" or "call count … exceeds") to keep lint output polished and consistent.
No description provided.