Repository navigation
Conversation
There was a problem hiding this comment.
Pull request overview
Adds version-aware modeling and interprocedural analysis for downloaded external reusable workflows and composite actions.
Changes:
- Adds external workflow/action extension predicates and call resolution.
- Moves downloaded files under a collision-resistant UUID directory.
- Adds documentation, models, and regression tests.
Show a summary per file
Review details
- Files reviewed: 35/44 changed files
- Comments generated: 2
- Review effort level: Balanced
Resolve $/ paths in downloaded reusable workflows and composite actions against their modeled repository, and cover both forms while retaining ./ compatibility.
Preserve both Actions test data extensions and regenerate the conflicting CWE-094 and CWE-829 expected results.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
CodeQL found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.
Rename model-binding predicates that do not return values and replace omittable exists variables with don't-care expressions.
Add QLDoc for external workflow and composite-action model bindings, classifications, and callable-name resolution.
| include:.github/workflows/*.yaml | ||
| include:.github/reusable_workflows/**/*.yml | ||
| include:.github/reusable_workflows/**/*.yaml | ||
| include:9466014afba34ef28239871ceabf4132/**/*.yml |
There was a problem hiding this comment.
This magic number approach does not sound right to me. But I lack sufficient contextual knowledge here to suggest anything.
There was a problem hiding this comment.
It is just a GUID without dashes. On one hand, especially because it becomes visible in web ui alert, it could be something human readable. On the other hand something very short such as reusable_workflow may collide with a folder in the repository. So something like downloaded_external_workflows_and_actions may work. I went with a unique GUID, but I'm open for discussions.
| externalCompositeActionDataModel(owner, repo, action_path, requested_ref, resolved_commit_sha, | ||
| local_path) and | ||
| local_path.trim() = this.getLocation().getFile().getRelativePath() |
There was a problem hiding this comment.
Quick check: for tuples in externalCompositeActionDataModel is it true that there is at most one such tuple for each local_path? I.e. does the local_path uniquely determine the rest of the values? Because otherwise this looks like a massive performance pitfall, since this joins with any Locationwith a matchinggetFile`.
There was a problem hiding this comment.
local_path is not unique: the producer intentionally lets requested refs with identical content share one materialized file while retaining separate provenance rows.
However, I asked AI to check the generated evaluator plan. The current predicate scans the model rows first and then uses keyed joins through relative path, file, location, and the action AST; it does not start by enumerating every Location. I also tested a path-bound bindingset/inline_late helper, but that changed the plan to an AST-to-model Cartesian product, increased joined tuples, and showed no runtime improvement.
| externalReusableWorkflowDataModel(owner, repo, workflow_path, requested_ref, | ||
| resolved_commit_sha, local_path) and | ||
| local_path.trim() = this.getLocation().getFile().getRelativePath() |
There was a problem hiding this comment.
Same question applies here as the pattern is the same.
There was a problem hiding this comment.
I tested this on the Grafana Actions database. All eight reusable-workflow model rows resolved to extracted workflow files, with each local_path matching the AST file’s relative path. The current plan scans externalReusableWorkflowDataModel first and uses keyed joins through file, location, and ReusableWorkflowImpl.
Previously it wasn't using extension predicates, just looking for downloaded workflow files in
.github/workflows/external/. It is used only internally, so imho the accidental rename in #18563 is safe to be edited.I used a random unique name
9466014afba34ef28239871ceabf4132where the downloaded external workflows/actions are stored, but I'm open for discussion. The name is visible in alerts, but using something asreusable_workflowsmay potentially run into the naming conflict.