Skip to content

Improve rulesIf evaluation #1876

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
ticapix wants to merge 5 commits into
base: master
Choose a base branch
from
+141 −178
Open

Improve rulesIf evaluation #1876

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
bde8d26
fix(rules:if): evaluate expressions via jsep AST, fix null !~ and rej…
ticapix Jun 5, 2026
960cd5c
fix(rules:if): reject non-regex RHS and wrap jsep parse errors
ticapix Jun 5, 2026
5b26451
test(rules:if): assert return value only, drop eval-string spy
ticapix Jun 5, 2026
aaef604
refactor(rules:if): remove dead _evaluateRuleIf and simplify _nodeToAtom
ticapix Jun 5, 2026
919bb83
refactor(rules:if): throw on unsupported node type in _nodeToAtom
ticapix Jun 5, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions bun.lock
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 2 additions & 0 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
"fetch-schema": "curl https://gitlab.com/gitlab-org/gitlab/-/raw/master/app/assets/javascripts/editor/schema/ci.json -sf > src/schema.json"
},
"dependencies": {
"@jsep-plugin/regex": "^1.0.4",
"ajv": "8.x.x",
"axios": "1.x.x",
"base64url": "3.x.x",
Expand All @@ -37,6 +38,7 @@
"fs-extra": "11.x.x",
"globby": "16.x.x",
"js-yaml": "4.x.x",
"jsep": "^1.4.0",
"jsonpointer": "5.x.x",
"micromatch": "4.x.x",
"object-traversal": "1.x.x",
Expand Down
186 changes: 112 additions & 74 deletions src/utils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
import "./global.js";
import {RE2JS} from "re2js";
import chalk from "chalk-template";
import jsep from "jsep";
import jsepRegex from "@jsep-plugin/regex";
import {Job, JobRule, Need, Service} from "./job.js";
import {needsComplex} from "./data-expander.js";
import fs from "fs-extra";
Expand All @@ -16,6 +18,10 @@ import {AxiosRequestConfig} from "axios";
import path from "node:path";
import {Argv} from "./argv.js";

jsep.plugins.register(jsepRegex); // /pattern/flags literals
jsep.addBinaryOp("=~", 10); // regex match operator
jsep.addBinaryOp("!~", 10); // regex non-match operator

type RuleResultOpt = {
argv: Argv;
cwd: string;
Expand Down Expand Up @@ -213,10 +219,39 @@ export class Utils {
return {when, allowFailure, variables: ruleVariable, needs: ruleNeeds};
}

// Reconstruct the source string for an atomic (non-logical) jsep node.
// Identifiers keep their name ($VAR), literals use their raw form so that
// strings retain quotes and regexes retain slashes and flags.
private static _nodeToAtom (node: jsep.Expression): string {
switch (node.type) {
case "Identifier": return (node as jsep.Identifier).name;
case "Literal": return (node as jsep.Literal).raw;
case "BinaryExpression": {
const n = node as jsep.BinaryExpression;
return `${Utils._nodeToAtom(n.left)} ${n.operator} ${Utils._nodeToAtom(n.right)}`;
}
default: throw new Error(`Unsupported expression node type: ${(node as any).type}`);
}
}

static stripQuotes (str: string) {
if (str.length < 2) return str;
const first = str[0];
const last = str[str.length - 1];
if ((first === "\"" && last === "\"") || (first === "'" && last === "'")) {
return str.slice(1, -1);
}
return str;
}

static evaluateRuleIf (ruleIf: string | undefined, envs: {[key: string]: string}): boolean {
if (ruleIf === undefined) return true;
assert(!/\$\{\w+\}/.test(ruleIf), chalk`rules:rule if invalid expression syntax: {blueBright ${ruleIf}}\nuse {green $VAR} not {red \${VAR\}} in rules:if`);
let evalStr = ruleIf;
evalStr = this.expandTextWith(evalStr, {
unescape: JSON.stringify("$"),
variable: (name) => JSON.stringify(envs[name] ?? null).replaceAll("\\\\", "\\"),
}); // replace all $VAR by their values

const flagsToBinary = (flags: string): number => {
let binary = 0;
Expand All @@ -231,97 +266,100 @@ export class Utils {
}
return binary;
};

// Expand all variables
evalStr = this.expandTextWith(evalStr, {
unescape: JSON.stringify("$"),
variable: (name) => JSON.stringify(envs[name] ?? null).replaceAll("\\\\", "\\"),
});
const expandedEvalStr = evalStr;

// Scenario when RHS is a <regex>
// https://regexr.com/85sjo
const pattern1 = /\s*(?<operator>(?:=~)|(?:!~))\s*\/(?<rhs>.*?[^\\])\/(?<flags>[igmsuy]*)(\s|$|\))/g;
evalStr = evalStr.replaceAll(pattern1, (_, operator, rhs, flags, remainingTokens) => {
let _operator;
switch (operator) {
case "=~":
_operator = "!=";
break;
case "!~":
_operator = "==";
break;
default:
throw operator;
// jsep parses ruleIf into an AST, handling &&, ||, () and operator precedence.
const walk = (node: jsep.Expression): boolean => {
if (node.type === "BinaryExpression") {
const n = node as jsep.BinaryExpression;
if (n.operator === "&&") return walk(n.left) && walk(n.right);
if (n.operator === "||") return walk(n.left) || walk(n.right);
if (n.operator === "=~" || n.operator === "!~") {
assert(n.left.type === "Literal", `Not a Literal: ${JSON.stringify(n.left)}`);
assert(n.right.type === "Literal", `Not a Literal: ${JSON.stringify(n.right)}`);
const leftStr = n.left as jsep.Literal;
const rightStr = n.right as jsep.Literal;
if (leftStr.value === null)
return n.operator === "!~"; // null =~ /p/ → false; null !~ /p/ → true
if (rightStr.value === null)
return false;
let regexStr: string = rightStr.raw;
regexStr = this.stripQuotes(regexStr);

const regex = /\/(?<pattern>.*)\/(?<flags>[igmsuy]*)/;
const _rhs = regexStr.replace(regex, (_: string, pattern: string, flags: string) => {
const flagsBinary = flagsToBinary(flags);
return `RE2JS.compile(${JSON.stringify(pattern)}, ${flagsBinary})`;
});

const assertMsg = [
"RHS (${rhs}) must be a regex pattern. Do not rely on this behavior!",
"Refer to https://docs.gitlab.com/ee/ci/jobs/job_rules.html#unexpected-behavior-from-regular-expression-matching-with- for more info...",
];
assert(_rhs !== regexStr, assertMsg.join("\n"));

const _operator = n.operator === "=~" ? "!=" : "=="; // =~ -> !=; !~ -> ==

const evalStr = `${leftStr.raw}.matchRE2JS(${_rhs}) ${_operator} null`;

let res;
try {
(globalThis as any).RE2JS = RE2JS;
res = (0, eval)(evalStr); // indirect eval
delete (globalThis as any).RE2JS;
} catch (error) {
console.error(error);
const assertMsg = [
"Error attempting to evaluate the following rules:",
" rules:",
` - if: '${Utils._nodeToAtom(node)}'`,
"as",
"```javascript",
`${evalStr}`,
"```",
];
assert(false, assertMsg.join("\n"));
}
return Boolean(res);
}
}
const _rhs = JSON.stringify(rhs); // JSON.stringify for escaping `"`
const containsNonEscapedSlash = /(?<!\\)\//.test(_rhs);
const assertMsg = [
"Error attempting to evaluate the following rules:",
" rules:",
` - if: '${expandedEvalStr}'`,
"as rhs contains unescaped quote",
];
assert(!containsNonEscapedSlash, assertMsg.join("\n"));
const flagsBinary = flagsToBinary(flags);
return `.matchRE2JS(RE2JS.compile(${_rhs}, ${flagsBinary})) ${_operator} null${remainingTokens}`;
});

// Scenario when RHS is surrounded by single/double-quotes
// https://regexr.com/85t0g
const pattern2 = /\s*(?<operator>=~|!~)\s*(["'])(?<rhs>(?:\\.|[^\\])*?)\2/g;
evalStr = evalStr.replaceAll(pattern2, (_, operator, __, rhs) => {
let _operator;
switch (operator) {
case "=~":
_operator = "!=";
break;
case "!~":
_operator = "==";
break;
default:
throw operator;
const atom = Utils._nodeToAtom(node);
let res;
try {
(globalThis as any).RE2JS = RE2JS;
res = (0, eval)(atom);
delete (globalThis as any).RE2JS;
} catch {
assert(false, [
"Error attempting to evaluate the following rules:",
" rules:",
` - if: '${ruleIf}'`,
"as",
"```javascript",
`${atom}`,
"```",
].join("\n"));
}
return Boolean(res);
};

const assertMsg = [
"RHS (${rhs}) must be a regex pattern. Do not rely on this behavior!",
"Refer to https://docs.gitlab.com/ee/ci/jobs/job_rules.html#unexpected-behavior-from-regular-expression-matching-with- for more info...",
];
assert((/\/(.*)\/(\w*)/.test(rhs)), assertMsg.join("\n"));

const regex = /\/(?<pattern>.*)\/(?<flags>[igmsuy]*)/;
const _rhs = rhs.replace(regex, (_: string, pattern: string, flags: string) => {
const flagsBinary = flagsToBinary(flags);
return `RE2JS.compile("${pattern}", ${flagsBinary})`;
});
return `.matchRE2JS(${_rhs}) ${_operator} null`;
});

evalStr = evalStr.replaceAll(/null.matchRE2JS\(.+?\)\s*!=\s*null/g, "false");
evalStr = evalStr.replaceAll(/null.matchRE2JS\(.+?\)\s*==\s*null/g, "true");

evalStr = evalStr.trim();

let res;
let ast;
try {
(globalThis as any).RE2JS = RE2JS;
res = (0, eval)(evalStr); // indirect eval
delete (globalThis as any).RE2JS;
ast = jsep(evalStr);
} catch {
const assertMsg = [
"Error attempting to evaluate the following rules:",
" rules:",
` - if: '${expandedEvalStr}'`,
` - if: '${ruleIf}'`,
"as",
"```javascript",
`${evalStr}`,
"```",
];
assert(false, assertMsg.join("\n"));
}
return Boolean(res);
return walk(ast!);
}


static evaluateRuleExist (cwd: string, ruleExists: string[] | {paths: string[]} | undefined): boolean {
if (ruleExists === undefined) return true;

Expand Down
Loading