Repository navigation
Dependency cleanup for 3.11 - #9122
Conversation
Refresh npm lockfiles and Studio ClientLib reviewed locks, add an osv-scanner workflow for release branches, remove unused root NuGet pins, and drop the unused remotion source directory. Co-authored-by: Sipke Schoorstra <sipkeschoorstra@outlook.com>
|
@elsa-workflows-bot review |
[Medium impact] No blocking issues were identified; the changes are suitable to merge based on the available evidence. Builds and tests were not rerun. SummaryRefreshes selected development dependencies, removes unused root NuGet pins and the unused video source project, and adds release-branch dependency scanning.
Reviews (2) · Last reviewed commit: "chore: make the release-branch osv-scann..." · Reviewed by Greptile |
There was a problem hiding this comment.
Elsa 3 Code Review: REQUEST_CHANGES + HIGH @ 0052479
Code Review, Round 1/6
The dependency bumps and housekeeping are clean. One thing in the new scan workflow needs a change before this lands, plus a small comment fix.
Blocking
1. The new scan check would be red on release branches from its first run, independent of the PR (.github/workflows/osv-scanner.yml:6-13, :21-31)
I ran osv-scanner 2.7.0 (the version this pin resolves to) locally on a clean export of this head with exactly these scan-args. It exits 1: the recursive pass picks up four reference packages.lock.json files under scripts/integration-program/secrets-sqlserver-bridge/ and scripts/integration-program/secrets-sqlserver-upgrade/, which pin old versions on purpose. The reusable workflow defaults fail-on-vuln to true, so:
- every PR into
release/*would carry a failingosv-scancheck that has nothing to do with that PR, which stalls release picks that are otherwise green; - every push to
release/*would put a failing run on the branch head; - on a fork PR into
release/*the SARIF upload step also fails, because the token is read-only there.
Please make the run report-only, for example by passing fail-on-vuln: false in with: (the SARIF upload and artifact still happen). Also consider dropping the pull_request trigger so it runs on push to release/* plus workflow_dispatch, which matches the PR description. If a hard gate is wanted later, exclude the reference fixtures first, for example with an osv-scanner.toml next to them.
2. Header comment overstates NuGet coverage (.github/workflows/osv-scanner.yml:1)
The comment says the workflow scans Directory.Packages.props. In the same local run, every Directory.Packages.props in the repo (root, apps/, extensions/src, extensions/test, extensions/samples, studio/src, studio/test, studio/samples, studio/react/dotnet, templates/) reports found 0 packages. Core uses versionless PackageReference items, so the central pins are not read at all. Only packages.lock.json files and csproj files with explicit versions contribute. Please reword the comment so it doesn't promise coverage it doesn't give. Adding real coverage for the central pins can be a follow-up.
Non-blocking
osv-scanner.yml:15-18repeats the job-levelpermissionsat workflow level. The job block alone is enough; the top level could bepermissions: {}. Optional.
Verified
- Lockfiles, resolved trees. I diffed
packagesin each changed lockfile against617a31d5. Every changed entry isdev, and no runtime entry was added, removed or re-versioned:designer: vitest 4.1.11 to 4.1.12 and its family, vite 8.3.1 to 8.3.5, rolldown 1.2.11 to 1.2.15, postcss, nanoid, chai, tinyrainbow, undici 8.12.3, source-map-js 1.2.2, serialize-javascript 7.1.4, fast-uri 3.1.9. The@types/react*/csstypeentries move fromdevOptionaltodev, which is the same install set.bpmn-shared: the vitest family and@types/estree.dom-interopand the JS Libraries ClientLib: fast-uri only.bpmn-jsandbpmn-x6: unchanged.
- Lock roots. The root
packages[""]of all five reviewed Studio lockfiles matches the matchingpackage.json, including the vitest 4.1.12 bumps in the Bpmn and Designer ClientLibs, sonpm ciinbuild_studio_clientlibs.shstays consistent. - JS Libraries dist.
npm ci && npm run buildinElsa.Expressions.JavaScript.Libraries/ClientLibreproducesdist/lodash.js,dist/lodashFp.jsanddist/moment.jsbyte for byte. fast-uri is only reached throughschema-utils > ajv. No committed bundle changed in the PR, and the Studio browser assets are untracked build outputs. - Wrapper overrides. The nested override syntax is scoped correctly. The lock has exactly one top-level
undici(7.30.1, used only by nx 23.3.0). The nested copies undernode-gyp(8.x) andpacote/node-gyp(6.x) are untouched. The workspace's ownuuid14.0.3 is unaffected. In a scratch install with the same overrides, nx 23.3.0 runs, and@storybook/addon-actions8.6.18 only callsv4fromuuid(import { v4 }/uuid.v4in its dist), which uuid 11.1.1 exports under both CJS and ESM. - Workflow pin.
b1d267aea5211fa648d9d442fa64b90dbdcbdd36is the commit behind the annotated tagv2.7.0. The reusable workflow pins all of its own actions by SHA. There is nopull_request_target, nosecrets:and noinherit, and checkout usespersist-credentials: false. The five--lockfile=package-lock.json:...arguments parse and each file is scanned (33 to 316 packages). - Deleted directory. Nothing in the README, docs, workflows, csproj or scripts refers to
design/video/elsa-readme-video. The only hits are indocs/integration-program/inventory/inventory.json, a dated snapshot pinned to an older commit, which is fine to leave.design/video/exports/stays. - Removed central pins.
rgover every csproj, props and targets file shows that onlyextensions/src/email/Elsa.Email(MailKit) andextensions/src/dropins/Elsa.DropIns(NuGet.Protocol) reference these packages, and nothing references AutoMapper. Both projects sit underextensions/src/Directory.Packages.props, which is their nearest central file and does not import the root one.dotnet restoreofElsa.Core.slnfandElsa.Extensions.slnfsucceeds,project.assets.jsonresolves MailKit 4.16.0 and NuGet.Protocol 7.0.3, and both projects build with 0 errors. The consolidatedsource-integration.patchcarries its own pins for a different layout, so it doesn't depend on the root lines.
Bot reviews
- CodeRabbit and Greptile left no inline threads on this head. Greptile's summary reports no blocking issue, and it notes that no scan was run. The local run above is what surfaces item 1.
- CodeRabbit's note that centrally pinned NuGet versions aren't covered matches item 2. Its suggestion to split the PR is non-blocking: each part is small and independently verified above.
— Code Review (Elsa 3), via Elsa Review
Keep the scan on push to release/* and workflow_dispatch, do not fail the job on findings, and describe only the lockfiles that are scanned. Co-authored-by: Sipke Schoorstra <sipkeschoorstra@outlook.com>
|
@elsa-workflows-bot review |
There was a problem hiding this comment.
Elsa 3 Code Review: APPROVE + HIGH @ b7c6266
Code Review, Round 2/6
Both Round 1 items are resolved. The delta since 00524792 is one commit (b7c6266d) touching only .github/workflows/osv-scanner.yml (+6/-10). Nothing else changed.
Round 1 items
- Report-only run, fixed (
osv-scanner.yml:7-11,:19). Thepull_requesttrigger is gone, so the workflow runs on push torelease/*andworkflow_dispatch, and the job passesfail-on-vuln: false. I re-ran osv-scanner 2.7.0 locally on a clean export of this head with the samescan-args. The scan still exits 1 and writes a non-emptyresults.json, so the reusable workflow's "Check that the scan completed" step passes. I then ranosv-reporterfrom the pinnedghcr.io/google/osv-scanner-action:v2.7.0image on that file with the exact arguments the reusable workflow builds. With--fail-on-vuln=falseit exits 0 and writes the SARIF, and withtrueit exits 1. The job now reports without going red. - Header comment, fixed (
osv-scanner.yml:1-4). It no longer claimsDirectory.Packages.propscoverage.
Permissions
The workflow level is now permissions: {}. The calling job (:28-31) keeps the same three grants as in Round 1 (actions and contents read, plus code-scanning write). That is exactly what the reusable workflow's osv-scan job requests, and the job-level grant is what the called workflow receives. So the checkout, artifact upload and SARIF upload to code scanning keep the access they need, and nothing else in the workflow gets a token.
Non-blocking
osv-scanner.yml:1now says "npm lockfiles". The recursive pass also reads NuGetpackages.lock.jsonfiles and csproj files with explicit versions; in fact, all current findings come from four NuGet lockfiles underscripts/integration-program/secrets-sqlserver-*. "lockfiles" alone would be more accurate. Wording only, no change needed for this PR.
Unchanged from Round 1
The dependency side is identical to 00524792: no lockfile, package.json, props or deleted-path file changed in the delta. The Round 1 checks still hold: dev-only lockfile changes with no runtime tree change, a byte-identical JS Libraries dist/, correctly scoped overrides (nx 23.3.0 with undici 7.30.1, addon-actions with uuid 11.1.1 using only v4), no remaining references to the deleted directory, and core plus extensions restoring and building without the three root pins.
Bot reviews
The four inline notes on osv-scanner.yml (lines 4, 11, 13 and 19) each describe a change that is in this head; they are answered and resolved. CodeRabbit's incremental pass over 00524792..b7c6266d produced no actionable comments, and Greptile's summary for this head reports no blocking issue.
CI
Every check on b7c6266d61e06b49acf5b0e4b55e2d21d4b6f530 is complete and green, including prove. run-affected-tests is skipped by design.
— Code Review (Elsa 3), via Elsa Review
Purpose
Dependency bumps and cleanup for the 3.11 milestone.
Scope
Description
Problem
A 3.11 cleanup batch for lockfiles, unused root NuGet pins, an unused remotion source tree, and release-branch scanning.
Solution
core/.../Elsa.Expressions.JavaScript.Libraries/ClientLib:fast-urinow resolves to 3.1.9. Webpack rebuild leftdist/lodash.js,dist/lodashFp.js, anddist/moment.jsbyte-identical.studio/src/wrappers: the installed nx is 23.3.0, which pinsundici7.29.0 exactly, so a lerna/nx bump is not enough. npmoverridessetnx.undicito^7.30.1and@storybook/addon-actions.uuidto^11.1.1.vite buildfor@elsa-workflows/elsa-studio-wasm-reactsucceeded.designer,dom-interop,bpmn-shared): dev-tree resolutions now include vite >= 8.3.3,@vitest/mocker>= 4.1.12, undici >= 8.12.2,source-map-js>= 1.2.2,serialize-javascript>= 7.1.3, andfast-uri>= 3.1.9. Designer and BPMN shared ClientLibpackage.jsonfiles bumpvitestfrom 4.1.11 to 4.1.12 so@vitest/mockercan match. Runtime dependency versions are unchanged.bpmn-jsandbpmn-x6locks do not contain those packages.scripts/integration-program/build_studio_clientlibs.shstill copies these reviewed locks into gitignoredClientLib/package-lock.jsonpaths; the consolidated ClientLib build passed (designer tests, generated BPMN types, all 10 required assets).package-lock.jsonwould either collide five files in that folder or move them next to each ClientLib (updating.gitignore,build_studio_clientlibs.sh, two workflows, and the DesignerHover README). The osv-scanner job lists those files with--lockfile=package-lock.json:....design/video/elsa-readme-videoonly.design/video/exportsis unchanged. The integration-program inventory snapshot still records the old path as historical evidence and was not rewritten..github/workflows/osv-scanner.yml: runs osv-scanner on pushes torelease/*and onworkflow_dispatch. It recursively scans lockfiles and explicitly includes the studio-clientlib lockfiles. The job is report-only (fail-on-vuln: false). The action is pinned by SHA. Directory.Packages.props coverage is out of scope.Directory.Packages.props:AutoMapper,MailKit, andNuGet.Protocolhad no root-graphPackageReference. Elsa.Email and Elsa.DropIns resolve throughextensions/src/Directory.Packages.props. Removed the unused root pins.dotnet restore Elsa.Core.slnfandElsa.Extensions.slnfsucceeded; extensions still resolve MailKit 4.16.0 and NuGet.Protocol 7.0.3.Verification
npm update fast-urithennpm run build; dist SHA-256 unchanged.npm ci --ignore-scripts(local wasm tarball substitution only for install), thennpm run build --workspace=@elsa-workflows/elsa-studio-wasm-react.scripts/integration-program/build_studio_clientlibs.sh .— Studio ClientLib proof passed.dotnet restore Elsa.Core.slnfanddotnet restore Elsa.Extensions.slnfafter removing the unused root pins.Expected outcome: lockfile and pin cleanup only; no runtime package version changes in the Studio ClientLibs; wrappers and ClientLib builds still succeed.
Summary by CodeRabbit