fix: expand variables in the rules:exists:paths form - #1923
Open
luantaraschi wants to merge 1 commit into
Open
Conversation
rules:exists accepts both a list of globs and an object with a paths key,
and Utils.evaluateRuleExist normalises the two. The variable expansion in
the Job constructor only recognised the list, so a job written with the
paths form kept its ${VAR} literals and the pattern handed to globbySync
matched nothing, silently turning the rule false.
The line now mirrors the rules:changes expansion right above it, which has
handled both forms since the paths form was added.
Contributor
There was a problem hiding this comment.
1 issue found across 3 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/job.ts">
<violation number="1" location="src/job.ts:200">
P1: When a job uses `parallel:matrix` with a variable in `rules:exists:paths`, later permutations reuse the first permutation's expanded path because this expansion mutates shared `jobData.rules`. Clone the rules and nested path arrays per job, or build an expanded rule copy without mutating the parsed job data.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| // Expand variables in rules:exists | ||
| this.rules.forEach((rule, ruleIdx, rules) => { | ||
| const exists = Array.isArray(rule.exists) ? rule.exists : null; | ||
| const exists = Array.isArray(rule.exists) ? rule.exists : rule.exists?.paths; |
Contributor
There was a problem hiding this comment.
P1: When a job uses parallel:matrix with a variable in rules:exists:paths, later permutations reuse the first permutation's expanded path because this expansion mutates shared jobData.rules. Clone the rules and nested path arrays per job, or build an expanded rule copy without mutating the parsed job data.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/job.ts, line 200:
<comment>When a job uses `parallel:matrix` with a variable in `rules:exists:paths`, later permutations reuse the first permutation's expanded path because this expansion mutates shared `jobData.rules`. Clone the rules and nested path arrays per job, or build an expanded rule copy without mutating the parsed job data.</comment>
<file context>
@@ -197,7 +197,7 @@ export class Job {
// Expand variables in rules:exists
this.rules.forEach((rule, ruleIdx, rules) => {
- const exists = Array.isArray(rule.exists) ? rule.exists : null;
+ const exists = Array.isArray(rule.exists) ? rule.exists : rule.exists?.paths;
if (!exists) {
return;
</file context>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
rules:existsaccepts two shapes, a list of globs and an object with apathskey.Utils.evaluateRuleExistknows both and normalises them:The variable expansion in the
Jobconstructor only knows one:So a job written with the
pathsform keeps its${VAR}literals,globbySyncgets a pattern with a dollar sign in it, nothing matches, and the rule quietly evaluates to false. The job is skipped and nothing is printed to explain why, which is the part that makes it hard to spot from the outside.The declared type shows the same asymmetry,
changes?: string[] | {paths: string[]}next toexists?: string[], which is what let thenullbranch look complete.The fix is the
existsline rewritten to match thechangesline above it, plus the type. Thepathsform arrived in #1530 forexistsand the expansion was not extended along with it.Testing
tests/test-cases/rules-exists/.gitlab-ci.ymlalready hasvar-expand-test, which uses${DIR}in the list form, andexecuted-job-paths, which uses thepathsform without a variable. The gap was exactly the combination of the two, so I addedvar-expand-paths-testfor it and one assertion inintegration.test.ts.On
masterthe new job never starts, so the assertion fails on missing output whilevar-expand-testright beside it runs and expands correctly. With the change both run.I also ran the neighbouring rule suites,
rules-blank,rules-curly-bracket-if,rules-needs,include-rulesandworkflow-rules-variables, all passing.rules-changesfails for me withspawn docker ENOENT, and it fails the same way on a cleanmastercheckout in the same environment, so that one is my sandbox and not this change.tsc --noEmitandeslintare clean.A note on the environment, since it affects what I can claim: my machine is Windows and the suite needs
rsync, whichUtils.rsyncTrackedFilescalls throughUtils.bash, so everything above was run inside a Debian container on Bun 1.3.14 with--pool=forks. The default threads pool crashes there onthis._thread.stdout.pipe, which looks like a Bun and vitest interaction rather than anything about this repository, so I did not chase it. I did not run the full suite or thedind-*tests.Disclosure: I used an AI coding assistant while working on this. Every command and result reported above I ran and checked myself.
Summary by cubic
Expands variable placeholders in
rules:existswhen using thepathsform so jobs no longer skip silently. Previously, only the list form expanded${VAR}; now both the list and{paths: []}forms expand, matchingrules:changes.Jobconstructor to expandrule.exists?.paths; widen the type toexists?: string[] | {paths: string[]}.var-expand-paths-testand an integration assertion to cover${VAR}expansion in thepathsform.rules:exists:pathswith variables may now trigger where they previously did not.Written for commit 820e7dc. Summary will update on new commits.