ci: build, test, lint only changed packages and dependants - #9373
Merged
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Mrtenz
changed the title
Mrtenz
changed the title
Mrtenz
marked this pull request as ready for review
July 15, 2026 10:21
Instead of rebuilding all 89 packages on every CI run, use `yarn workspaces foreach --since` to build only the packages that changed relative to the target branch, plus their transitive dependants. Falls back to a full `yarn build` on push-to-main events where no base branch ref is available.
`--depth=1` on the base branch fetch doesn't give git enough history to find the common ancestor with the PR branch, causing yarn's `--since` to fail. Replace with `--no-tags` (full history, no tags).
The `action-checkout-and-setup` does a `fetch-depth: 1` shallow clone, so the PR branch history only goes back one commit. Git cannot find the merge base against `origin/$BASE_REF` from such a shallow history, causing yarn's `--since` to fail with "No ancestor could be found". Unshallow the checkout first, so the full history is available for the merge-base computation.
Instead of heuristic depth fetches, call the GitHub Compare API to get the exact merge base SHA, then fetch only that single commit. This avoids fetching large chunks of history while being precise regardless of how old the branch is.
The previous approach fetched the merge base commit but git still couldn't confirm it was an ancestor of HEAD because the PR branch checkout is depth=1. Fix by unshallowing the checkout using `--filter=blob:none`, which fetches full commit and tree history without downloading file content — sufficient for `git diff --name-only`.
Without an explicit refspec, `git fetch --unshallow` deepens all refs that were fetched during checkout. Passing `origin HEAD` limits it to the currently checked-out ref.
Instead of running `yarn workspaces foreach --since` sequentially, generate a temporary tsconfig that lists only changed packages and their transitive dependants as project references. This lets ts-bridge use TypeScript's project-references to parallelise the build.
mktemp creates the file in /tmp by default, causing ts-bridge to resolve relative package paths against /tmp instead of the repo root. Using --tmpdir="$GITHUB_WORKSPACE" and cleaning up with rm -f after the build keeps relative paths correct without leaving a dirty tree.
…dencies Three bugs: - actions/checkout checks out the merge commit (PR + base), so `git diff $MERGE_BASE...HEAD` included all of main's changes. Fix: pass the explicit PR head SHA and diff against that instead. - Packages included as dependants of changed packages couldn't build because their own dependencies had no dist files. Fix: expand the build set to include transitive dependencies as well. - ts-bridge rejects a tsconfig with `files: []` and no references. Fix: skip ts-bridge entirely when no packages need building.
Extract shared workspace logic into scripts/lib/workspaces.mts and add scripts/get-changed-workspaces.mts. A new get-changed-packages CI job fetches the merge base once and computes which packages changed; test-18/20/22, validate-changelog, and build all consume its outputs rather than computing independently. The build job no longer calls the GitHub Compare API itself — it reads the merge base from get-changed-packages and only needs to unshallow its own checkout.
CI-only PRs (e.g. changes only to workflow files) would previously fall
back to running tests and changelog validation for all packages. This was
meant to ensure required status checks were not skipped, but the only
required check is at the workflow level ("all jobs pass"), not at the
individual job level. An empty matrix safely skips those jobs without
blocking merges.
GitHub Actions errors with "Matrix vector does not contain any values" when a matrix input resolves to an empty array. Add an `if` condition to `validate-changelog`, `test-18`, `test-20`, and `test-22` so they are skipped entirely when `package-names` is `[]`.
This job builds the full wallet-cli dependency subtree, so it can't use the package matrix. Instead, skip it entirely when a merge base is available and `@metamask/wallet-cli` is not in the changed packages list.
If any changed file lives outside all package directories, rebuild and test everything. Root-level configs, workflow files, and scripts can all affect every package, so a full run is the safe default. A set of known-safe root files (yarn.lock, README.md, .gitignore, etc.) are excluded from this check since they don't affect package builds or tests.
getAllWorkspaces was using `yarn workspaces list` without `--no-private`, so private packages (e.g. wallet-framework-docs) were included in the test matrix. The prepare job always used `--no-private`, so these were never tested before and have no working test scripts.
Extract a `checkRootChange` helper and an optional `changedFiles` parameter in `computeChangedWorkspaces` to avoid a double `git diff` call. Rewrite `get-changed-workspaces.mts` with Yargs and change its output to a single JSON object (`names`, `locations`, `hasRootChange`) so one script invocation covers all three fields. In CI, move `lint:eslint` out of the matrix into a dedicated `lint-eslint` job. When a PR only touches packages, pass their paths directly to `yarn lint:eslint`; when root files change, fall back to a full run.
Avoids the unquoted subshell (shellcheck SC2046) and keeps the invocation to a single eslint process. With ~93 packages averaging ~30 chars each, the argument list is well under ARG_MAX.
`jq` pretty-prints arrays across multiple lines by default, which causes "Invalid format" errors when writing to GITHUB_OUTPUT. Adding `-c` keeps the JSON on a single line.
- Fix dead `== ''` guard in `test-wallet-cli-e2e` (should be `== '[]'` since `package-names` is always a JSON array string, never empty) - Remove `eslint-suppressions.json` from `IGNORED_ROOT_FILES` so a PR that only adds suppressions still runs ESLint - Pass `getAllWorkspaces()` to `computeChangedWorkspaces` in `generate-partial-build-tsconfig.mts` so that JS-only package changes are correctly attributed and don't trigger a spurious full TS rebuild - Parallelise `package.json` reads in `getWorkspaceDependencies` with `Promise.all`
Removes the duplicate `analyse-code` job from `main.yml` and moves it into `lint-build-test.yml` where it can consume the `scan-paths` output from `get-changed-packages` directly. When only specific packages changed, the scanner receives their locations; when root files changed or there is no merge base, it scans everything. Uses a temporary commit SHA for the scanner action until `paths` support is released as a stable version.
Aligns `scanner-ref` with the action SHA so the internal scanner also picks up the `paths` input implementation.
Paths scoping in CodeQL doesn't reduce runtime — most time is spent compiling .qls query files regardless of the analysis scope. Revert the analyse-code job back to main.yml at @v2.
Saves one sequential job hop (~30s) by running the merge base fetch and changed package detection directly in the 24.x matrix run of prepare, guarded with `if: matrix.node-version == '24.x'`.
Makes it clear at a glance whether CI is running a full or partial check, and which packages are included.
Print each package on its own line with a "- " prefix, and add a blank line after the list to visually separate it from the command output.
Renames eslint-paths to changed-paths and uses it in the build step too. Previously, a root change would set MERGE_BASE and trigger a partial build even though everything should be rebuilt. Now both steps check CHANGED_PATHS == "full" as the authoritative signal.
The partial-run branches already print a blank line after the package list. Add the same blank line to the full-run branches.
Declares the minimum permissions required by the workflow explicitly.
Mrtenz
force-pushed
the
mrtenz/ci-incremental-build
branch
from
July 23, 2026 14:43
664902c to
a508a37
Compare
Gudahtt
reviewed
Jul 23, 2026
The previous condition ran the job when package-names was empty, which would only happen when nothing changed — the wrong time to run e2e tests. On a full run, package-names already contains all packages including wallet-cli, so contains() is sufficient on its own.
Gudahtt
reviewed
Jul 23, 2026
| mergeBase: string, | ||
| headRef: string, | ||
| includeDependencies: boolean, | ||
| changedFiles?: string[], |
Member
There was a problem hiding this comment.
Nit: It's a little odd for this parameter to exist, because the required mergeBase and headRef parameters aren't actually used when this is passed in.
It seems clearer to choose one other the other: either require changed packages to be passed in, or require mergeBase and headRef.
Given how this is used in get-changed-workspaces, probably requiring changedPackages would be better
Member
There was a problem hiding this comment.
Another nit: I prefer to use option objects for cases like this so that each parameter can be identified at the callsite. I had to scroll back and forth to ensure they were passed in the correct order, and to identify what the true 4th param represented.
I'm tempted to propose this as a lint rule, but it feels a bit too opinionated 🤔 Not sure.
Member
Author
There was a problem hiding this comment.
Good point. Removed mergeBase and headRef from this function. I would change it to an option object, but it probably makes less sense now that it only has three parameters?
| 'CLAUDE.md', | ||
| 'README.md', | ||
| 'teams.json', | ||
| 'yarn.lock', |
Member
There was a problem hiding this comment.
Not sure about this one. A lockfile change could definitely break something
Member
Author
There was a problem hiding this comment.
I'm still on the fence about this too.
If we do consider changes to yarn.lock as needing to run everything, I feel like we limit the number of PRs where these changes are useful a lot.
I was thinking we could try to parse the changes to yarn.lock to make it more granular, but that's probably not worth the effort.
I'm open to removing this from IGNORED_ROOT_FILES for now.
Member
There was a problem hiding this comment.
Maybe as a compromise we could ignore yarn.lock changes that are just workspace package bumps (e.g. only run the workspaces that were bumped, rather than all). But if any external dependency changes, re-run everything.
That would be far more achievable than trying to calculate exactly which packages were impacted by a given lockfile change.
We already have some code for parsing lockfile diffs here: https://github.com/MetaMask/metamask-extension/blob/446c5a25b56d6521ad963e7503ce337ad975a8fb/.github/scripts/yarn-audit-diff.mts#L233
(though I didn't review it carefully)
Member
There was a problem hiding this comment.
For now I'm leaning towards removing it from the ignore list. We can always add it later.
Member
Author
There was a problem hiding this comment.
Sounds good. Will remove it for now, and create a ticket for us to consider this in the future.
| .help() | ||
| .parseAsync(); | ||
|
|
||
| const mergeBase = argv._[0] as string; |
Member
There was a problem hiding this comment.
Nit: I found that we can get rid of these type assertions by getting the named parameters from argv, e.g. argv.mergeBase, argv.headSha, etc.).
I had to switch demandCommand to demandOption though; apparently demandCommand only works for sub-commands not positional parameters (which their docs seem to contradict, but 🤷 I tested it).
Member
There was a problem hiding this comment.
Here's the full snippet I tested:
const argv = await yargs(hideBin(process.argv))
.usage('$0 <merge-base> [head-sha] [options]')
.positional('merge-base', {
type: 'string',
describe: 'Merge base SHA',
})
.positional('head-sha', {
type: 'string',
describe: 'PR branch tip SHA (defaults to HEAD)',
default: 'HEAD',
})
.option('include-dependencies', {
type: 'boolean',
default: false,
describe:
'Also expand to transitive dependencies (needed for TypeScript builds)',
})
.demandOption('merge-base')
.help()
.parseAsync();
const { mergeBase, headSha: headRef, includeDependencies } = argv;
const workspaces = await getAllWorkspaces();
const changedFiles = await getChangedFiles(mergeBase, headRef);
const hasRootChange = checkRootChange(workspaces, changedFiles);
const changed = await computeChangedWorkspaces(
workspaces,
mergeBase,
headRef,
includeDependencies,
changedFiles,
);
Member
Author
There was a problem hiding this comment.
Ah, nice. That's definitely a lot better.
Done in c5ad03c.
Member
There was a problem hiding this comment.
Whoops, I guess I didn't test it properly. I see the CI failure.
Member
Author
There was a problem hiding this comment.
Hmm, I tried the demandOption parameter in the positional options too, but then it's not added to the argv object as parameter, only to argv._. I'll just make them regular parameters.
Member
There was a problem hiding this comment.
I think I found the problem. Our .usage call has no description. If I pass a description, then .positional works as expected, and demandOption gives it the proper type.
Why the usage description matters here, I have no idea
Member
There was a problem hiding this comment.
Oh, making it an option works too
Member
Author
There was a problem hiding this comment.
Interesting. 😅 I guess it doesn't really matter if we use an option compared to a positional, so will just keep it as-is.
Gudahtt
previously approved these changes
Jul 23, 2026
Previously the function accepted optional `changedFiles` alongside `mergeBase`/`headRef`, which were redundant when the file list was already provided. Now callers always fetch changed files first and pass them in, removing the implicit fallback fetch from inside the function.
Replaces positional arguments with `--merge-base` and `--head-ref` named options, using yargs `demandOption` for the required one. Updates the workflow call site to match.
Changes to `yarn.lock` alone should trigger a full run, since they may indicate dependency upgrades that affect package behaviour.
4 tasks
pull Bot
pushed a commit
to dmrazzy/core
that referenced
this pull request
Jul 24, 2026
…ces in incremental CI (MetaMask#9643) ## Explanation Previously, any PR that touched `yarn.lock` would fall through to a full rebuild and test run of all packages, because the script had no way to know which packages were actually affected by the lockfile change. Similarly, any change to the root `package.json` (including version-only bumps from release PRs) triggered a full run. This PR adds smart diffing for both files: **`yarn.lock` diffing:** When `yarn.lock` appears in the changed files, it is parsed using `@yarnpkg/parsers` and compared against the merge-base version. Entry checksums are compared (rather than resolutions) so that any change to installed package content is detected, including the rare case of a package being re-released under the same version. The changed package names are then cross-referenced against each workspace's full transitive dependency closure (built by walking the resolved lockfile graph via `@yarnpkg/core`) to produce the minimal set of workspaces that need to be rebuilt and tested. **Root `package.json` diffing:** When `package.json` changes, both sides are parsed and compared with the `version` field stripped. A version-only diff (e.g. a release PR bump) is ignored; any other change (scripts, dependencies, etc.) still triggers a full run. **Refactored `computeChangedWorkspaces`:** The function now fetches workspaces and changed files internally, and returns `{ workspaces: Workspace[], hasRootChange: boolean }` instead of `Set<string>`. This removes boilerplate from both callers and allows `get-changed-workspaces` to short-circuit before doing any workspace computation when `hasRootChange` is true. ## References - Part of the incremental CI build work from MetaMask#9373 ## Checklist - [ ] I've updated the test suite for new or updated code as appropriate - [ ] I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate - [ ] I've communicated my changes to consumers by [updating changelogs for packages I've changed](https://github.com/MetaMask/core/tree/main/docs/processes/updating-changelogs.md) - [ ] I've introduced [breaking changes](https://github.com/MetaMask/core/tree/main/docs/processes/breaking-changes.md) in this PR and have prepared draft pull requests for clients and consumer packages to resolve them <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Medium Risk** > Changes how CI decides what to build/test; incorrect lockfile or package.json diff logic could skip needed workspaces or over-build. New Yarn Berry APIs in scripts add maintenance surface but no runtime product impact. > > **Overview** > **Incremental CI** no longer treats every `yarn.lock` touch as a full monorepo run. When the lockfile changes, the script diffs merge-base vs head checksums, maps changed packages to workspaces whose **transitive** closure includes them (via `@yarnpkg/core`), then applies the usual dependant/dependency expansion. > > **Root `package.json`** is ignored for “root change” unless something besides `version` changed (release-only bumps stay incremental). `yarn.lock` is also excluded from the generic root-change path so lockfile logic handles it. > > **`computeChangedWorkspaces`** now takes `{ mergeBase, headRef, includeDependencies }`, loads workspaces and git diffs internally, and returns `{ workspaces, hasRootChange }` with dependency graph edges as `Workspace` objects. Callers `get-changed-workspaces.mts` and `generate-partial-build-tsconfig.mts` were simplified to match. > > Adds `@yarnpkg/cli`, `@yarnpkg/core`, `@yarnpkg/fslib`, and `@yarnpkg/parsers` dev deps (large `yarn.lock` churn). **`foundryup`** gets a `@ts-expect-error` on `tar` extract `transform` for `tar@7.5.12` typing. > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit 4bf933e. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).</sup> <!-- /CURSOR_SUMMARY -->
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
3 participants
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.
Explanation
Currently, every CI run rebuilds all packages from scratch and runs tests, changelog validation, and ESLint across the entire monorepo, regardless of how many packages actually changed.
This PR scopes TypeScript builds, tests, changelog validation, and ESLint to only the packages that changed plus their transitive dependants, by:
git diff --name-onlyagainst that merge base to find changed files.ts-bridgehas all referenceddist/outputs available).main).A new
scripts/get-changed-workspaces.mtsscript computes the changed package set and is called from thepreparejob (24.x matrix run only). Downstream jobs read the outputs frompreparedirectly — the separateget-changed-packagesjob has been removed, saving one sequential job hop.The
changed-pathsoutput gates both ESLint scope and TypeScript build scope: when it is"full", both run against all packages; when it is a JSON array of paths, each runs only against that subset.Benchmark PRs
Four benchmark PRs target this branch to verify the expected behaviour:
IGNORED_ROOT_FILES@metamask/logging-controller)logging-controller+ 2 transitive dependants)accounts-,gas-fee-,network-controller)README.md(ignored) +logging-controllerREADME.mddoes not trigger a full runChecklist
Note
Medium Risk
Changes how every PR is validated; incorrect workspace expansion or root-change detection could skip builds or tests, though impact is limited to CI, not shipped product code.
Overview
CI runs only what the PR touches instead of rebuilding and testing the whole monorepo on every run.
The
preparejob (Node 24.x) resolves the merge base via the GitHub Compare API, runsget-changed-workspaces.mts, and exportspackage-names,changed-paths, andmerge-base. Downstream jobs use those outputs: changelog validation and per-Node test matrices iterate only affected workspace names;wallet-clie2e runs only when that package is in the set.ESLint moves to a dedicated
lint-eslintjob that runsyarn lint:eslinton all packages whenchanged-pathsisfull, otherwise only on the listed workspace paths.Build uses
generate-partial-build-tsconfig.mtsplusts-bridgefor a partial TypeScript reference graph (changed packages plus transitive dependants and dependencies) when a merge base exists and there is no root-level change; otherwise it keepsyarn build. Root or workflow changes outside ignored files (e.g. notREADME.md) still force a full run; pushes without a merge base fall back to all packages.New shared logic lives in
scripts/lib/workspaces.mts(git diff, dependency graph, root-change detection);tsconfig.scripts.jsonnow includes*.mtsfor those scripts.Reviewed by Cursor Bugbot for commit b910c31. Bugbot is set up for automated code reviews on this repo. Configure here.