ci: run benchmark comparison in isolated worktree and harden security

- Run comparison benchmark in an isolated git worktree to prevent workspace pollution and local branch conflicts.
- Harden security by passing benchmark target and SHA as environment variables to prevent shell injection, and adding '--' to bazel query and git rev-parse.
- Optimize workflow by removing pnpm caching to mitigate cache poisoning risks.
- Improve robustness of benchmark log parsing, supporting both ZIP outputs and raw directories, and safely checking for JSON reports.
- Centralize git command execution on the dev-infra GitClient for consistency.
- Add tslib to benchpress dependencies to prevent module resolution failures.

(cherry picked from commit 547d85addf)

.github/workflows/benchmark-compare.yml

@@ -39,17 +39,19 @@ jobs:
       # We cannot use `angular/dev-infra/github-actions/npm/checkout-and-setup-node` here
       # because it does not support checking out from a fork (as it lacks a `repository` input).
       # Thus, we checkout and setup Node/pnpm manually.
-      - name: Install pnpm
-        uses: pnpm/action-setup@0e279bb959325dab635dd2c09392533439d90093 # v6.0.8
-
       - name: Setup Node.js
         uses: actions/setup-node@48b55a011bda9f5d6aeb4c2d9c7362e8dae4041e # v6.4.0
         with:
           node-version-file: '.nvmrc'
-          cache: 'pnpm'
+
+      - name: Install pnpm
+        uses: pnpm/action-setup@0e279bb959325dab635dd2c09392533439d90093 # v6.0.8
 
       - run: pnpm install --frozen-lockfile
 
+      - name: Setup Bazel
+        uses: angular/dev-infra/github-actions/bazel/setup@442c2fcbf06a321b5196b4c5fc70e78a49242958
+
       - uses: angular/dev-infra/github-actions/bazel/configure-remote@442c2fcbf06a321b5196b4c5fc70e78a49242958
         with:
           bazelrc: ./.bazelrc.user
@@ -61,10 +63,12 @@ jobs:
           COMMENT_BODY: ${{ github.event.comment.body }}
         run: pnpm benchmarks prepare-for-github-action "$COMMENT_BODY"
 
-      - run: pnpm benchmarks run-compare ${{steps.info.outputs.compareSha}} "${{steps.info.outputs.benchmarkTarget}}"
-
+      - run: pnpm benchmarks run-compare "$COMPARE_SHA" "$BENCHMARK_TARGET"
         id: benchmark
         name: Running benchmark
+        env:
+          BENCHMARK_TARGET: ${{steps.info.outputs.benchmarkTarget}}
+          COMPARE_SHA: ${{steps.info.outputs.compareSha}}
 
       - uses: peter-evans/create-or-update-comment@e8674b075228eee787fea43ef493e45ece1004c9 # v5
         with:

packages/benchpress/package.json

@@ -4,7 +4,8 @@
   "description": "Benchpress - a framework for e2e performance tests",
   "dependencies": {
     "@angular/core": "^22.0.0-next",
-    "reflect-metadata": "^0.2.0"
+    "reflect-metadata": "^0.2.0",
+    "tslib": "^2.3.0"
   },
   "repository": {
     "type": "git",

pnpm-lock.yaml

@@ -925,6 +925,9 @@ importers:
       reflect-metadata:
         specifier: ^0.2.0
         version: 0.2.2
+      tslib:
+        specifier: ^2.3.0
+        version: 2.8.1
 
   packages/common:
     dependencies:

scripts/benchmarks/index.mts

@@ -6,6 +6,8 @@
  * found in the LICENSE file at https://angular.dev/license
  */
 
+import fs from 'fs';
+import path from 'path';
 import {setOutput} from '@actions/core';
 import {GitClient, Log, bold, green, yellow} from '@angular/ng-dev';
 import {select} from '@inquirer/prompts';
@@ -17,7 +19,7 @@ import {
   getTestlogPath,
   resolveTarget,
 } from './targets.mts';
-import {exec} from './utils.mts';
+import {exec, projectDir} from './utils.mts';
 
 const benchmarkTestFlags = [
   '--cache_test_results=no',
@@ -27,6 +29,9 @@ const benchmarkTestFlags = [
   // reduce fluctuation. Output streamed ensures that deps can build with RBE, but
   // tests run locally while also providing useful output for debugging.
   '--test_output=streamed',
+  // In the comparison run, we create a hybrid workspace (main files + PR scripts/lockfiles).
+  // This causes a lockfile mismatch, so we must allow Bazel to update the lockfile in memory.
+  '--lockfile_mode=update',
 ];
 
 await yargs(process.argv.slice(2))
@@ -98,11 +103,11 @@ async function prepareForGitHubAction(commentBody: string): Promise<void> {
   // Attempt to find the compare SHA. The commit may be either part of the
   // pull request, or might be a commit unrelated to the PR- but part of the
   // upstream repository. We attempt to fetch/resolve the SHA in both remotes.
-  const compareRefResolve = git.runGraceful(['rev-parse', compareRefRaw]);
+  const compareRefResolve = git.runGraceful(['rev-parse', '--', compareRefRaw]);
   let compareRefSha = compareRefResolve.stdout.trim();
   if (compareRefSha === '' || compareRefResolve.status !== 0) {
     git.run(['fetch', '--depth=1', git.getRepoGitUrl(), compareRefRaw]);
-    compareRefSha = git.run(['rev-parse', 'FETCH_HEAD']).stdout.trim();
+    compareRefSha = git.run(['rev-parse', '--', 'FETCH_HEAD']).stdout.trim();
   }
 
   setOutput('compareSha', compareRefSha);
@@ -126,8 +131,8 @@ async function runBenchmarkCmd(bazelTargetRaw: string | undefined): Promise<void
 }
 
 /** Runs a benchmark Bazel target. */
-async function runBenchmarkTarget(bazelTarget: ResolvedTarget): Promise<void> {
-  await exec('bazel', ['test', bazelTarget, ...benchmarkTestFlags]);
+async function runBenchmarkTarget(bazelTarget: ResolvedTarget, cwd?: string): Promise<void> {
+  await exec('pnpm', ['bazel', 'test', bazelTarget, ...benchmarkTestFlags], cwd);
 }
 
 /**
@@ -138,13 +143,6 @@ async function runCompare(bazelTargetRaw: string | undefined, compareRef: string
   const git = await GitClient.get();
   const currentRef = git.getCurrentBranchOrRevision();
 
-  if (git.hasUncommittedChanges()) {
-    Log.warn(bold('You have uncommitted changes.'));
-    Log.warn('The script will stash your changes and re-apply them so that');
-    Log.warn('the comparison ref can be checked out.');
-    Log.warn('');
-  }
-
   if (bazelTargetRaw === undefined) {
     bazelTargetRaw = await promptForBenchmarkTarget();
   }
@@ -159,29 +157,92 @@ async function runCompare(bazelTargetRaw: string | undefined, compareRef: string
 
   const workingDirResults = await collectBenchmarkResults(testlogPath);
 
-  // Stash working directory as we might be in the middle of developing
-  // and we wouldn't want to discard changes when checking out the compare SHA.
-  git.run(['stash']);
+  // Define isolated temporary workspace inside `dist/` so it is ignored by git.
+  const tempDir = path.join(projectDir, 'dist/benchmark-compare-temp');
+  let comparisonResults: any = null;
 
   try {
-    Log.log(green('Fetching comparison revision.'));
-    // Note: Not using a shallow fetch here as that would convert the local
-    // user repository into an incomplete repository.
-    git.run(['fetch', git.getRepoGitUrl(), compareRef]);
-    Log.log(green('Checking out comparison revision.'));
-    git.run(['checkout', 'FETCH_HEAD']);
+    Log.log(green(`Creating isolated workspace in ${tempDir}`));
+    try {
+      git.run(['worktree', 'remove', '--force', tempDir]);
+    } catch (e) {
+      if (fs.existsSync(tempDir)) {
+        fs.rmSync(tempDir, {recursive: true, force: true});
+      }
+      try {
+        git.run(['worktree', 'prune']);
+      } catch (pruneError) {
+        // Ignore prune errors
+      }
+    }
 
-    await exec('pnpm', ['install', '--frozen-lockfile']);
-    await runBenchmarkTarget(bazelTarget);
+    // Ensure the comparison ref is fetched on the main repository if not already present.
+    const hasCommit = git.runGraceful(['cat-file', '-e', `${compareRef}^{commit}`]).status === 0;
+    if (!hasCommit) {
+      Log.log(green(`Fetching comparison revision ${compareRef}...`));
+      git.run(['fetch', git.getRepoGitUrl(), compareRef]);
+    } else {
+      Log.log(
+        green(`Comparison revision ${compareRef} is already available locally. Skipping fetch.`),
+      );
+    }
+
+    // Create isolated workspace instantly using native git worktree.
+    Log.log(green(`Creating isolated worktree for ${compareRef} in ${tempDir}`));
+    git.run(['worktree', 'add', '--detach', tempDir, compareRef]);
+
+    // Copy the current PR's benchmark scripts and packages into the isolated workspace.
+    // Explicitly exclude node_modules to avoid copying broken relative symlinks.
+    Log.log(green('Copying PR benchmark scripts and packages into isolated workspace...'));
+    const dirsToCopy = ['scripts/benchmarks', 'packages/benchpress'];
+    for (const relDir of dirsToCopy) {
+      const src = path.join(projectDir, relDir);
+      const dest = path.join(tempDir, relDir);
+      fs.rmSync(dest, {recursive: true, force: true});
+      fs.cpSync(src, dest, {
+        recursive: true,
+        filter: (srcPath) => !srcPath.split(path.sep).includes('node_modules'),
+      });
+    }
+
+    // Copy `.bazelrc.user` if it exists, otherwise create it.
+    const bazelrcUser = path.join(projectDir, '.bazelrc.user');
+    const tempBazelrcUser = path.join(tempDir, '.bazelrc.user');
+    if (fs.existsSync(bazelrcUser)) {
+      fs.copyFileSync(bazelrcUser, tempBazelrcUser);
+    } else {
+      fs.writeFileSync(tempBazelrcUser, '');
+    }
+
+    // Run pnpm install inside the isolated workspace.
+    Log.log(green('Installing dependencies in isolated workspace...'));
+    await exec('pnpm', ['install', '--no-frozen-lockfile', '--prefer-offline'], tempDir);
+
+    // Run the benchmark on the comparison workspace.
+    Log.log(green('Running benchmark in isolated workspace...'));
+    await runBenchmarkTarget(bazelTarget, tempDir);
+
+    // Resolve testlog path and collect results from the isolated workspace.
+    Log.log(green('Collecting comparison results...'));
+    const tempTestlogPath = await getTestlogPath(bazelTarget, tempDir);
+    comparisonResults = await collectBenchmarkResults(tempTestlogPath);
   } finally {
-    restoreWorkingStage(git, currentRef);
+    Log.log(green('Cleaning up isolated workspace...'));
+    try {
+      git.run(['worktree', 'remove', '--force', tempDir]);
+    } catch (e) {
+      Log.warn(`Failed to clean up isolated worktree: ${e}`);
+      if (fs.existsSync(tempDir)) {
+        fs.rmSync(tempDir, {recursive: true, force: true});
+      }
+      try {
+        git.run(['worktree', 'prune']);
+      } catch (pruneError) {
+        // Ignore prune errors
+      }
+    }
   }
 
-  // Re-install dependencies for `HEAD`.
-  await exec('pnpm', ['install', '--frozen-lockfile']);
-
-  const comparisonResults = await collectBenchmarkResults(testlogPath);
-
   // If we are running in a GitHub action, expose the benchmark text
   // results as outputs. Useful if those are exposed as a GitHub comment then.
   if (process.env.GITHUB_ACTION !== undefined) {
@@ -198,11 +259,3 @@ async function runCompare(bazelTargetRaw: string | undefined, compareRef: string
   Log.info(bold(yellow(`Working stage (${currentRef}) results:`)), '\n');
   Log.info(workingDirResults.summaryConsoleText);
 }
-
-function restoreWorkingStage(git: GitClient, initialRef: string) {
-  Log.log(green('Restoring working stage'));
-  git.run(['checkout', '-f', initialRef]);
-
-  // Stash apply could fail if there were not changes in the working stage.
-  git.runGraceful(['stash', 'apply']);
-}

scripts/benchmarks/results.mts

@@ -6,6 +6,7 @@
  * found in the LICENSE file at https://angular.dev/license
  */
 
+import fs from 'fs';
 import path from 'path';
 import Zip from 'adm-zip';
 
@@ -16,6 +17,7 @@ interface JsonReport {
   metricsText: string;
   statsText: string;
   validSampleTexts: string[];
+  completeSample?: any;
 }
 
 /** Results of an individual benchmark scenario. */
@@ -38,31 +40,80 @@ export interface OverallResult {
 
 /** Collects and parses the benchmark results of the given Bazel target testlog directory. */
 export function collectBenchmarkResults(testlogDir: string): OverallResult {
-  const z = new Zip(path.join(testlogDir, 'test.outputs/outputs.zip'));
   const scenarioResults: ScenarioResult[] = [];
+  const zipPath = path.join(testlogDir, 'test.outputs/outputs.zip');
 
-  for (const e of z.getEntries()) {
-    if (path.extname(e.entryName) !== '.json') {
-      continue;
+  if (fs.existsSync(zipPath)) {
+    const z = new Zip(zipPath);
+    for (const e of z.getEntries()) {
+      if (path.extname(e.entryName) !== '.json') {
+        continue;
+      }
+
+      try {
+        const data = JSON.parse(z.readAsText(e.entryName));
+        if (isJsonReport(data)) {
+          addScenarioResult(data, scenarioResults);
+        }
+      } catch (err) {
+        // Skip files that fail to parse
+      }
     }
+  } else {
+    const outputsDir = path.join(testlogDir, 'test.outputs');
+    if (fs.existsSync(outputsDir)) {
+      for (const file of fs.readdirSync(outputsDir)) {
+        if (path.extname(file) !== '.json') {
+          continue;
+        }
 
-    const data = JSON.parse(z.readAsText(e.entryName));
+        const filePath = path.join(outputsDir, file);
+        if (!fs.statSync(filePath).isFile()) {
+          continue;
+        }
 
-    // Skip files that do not look like benchpress reports.
-    if (!isJsonReport(data)) {
-      continue;
+        let data;
+        try {
+          data = JSON.parse(fs.readFileSync(filePath, 'utf-8'));
+        } catch (e) {
+          continue;
+        }
+
+        if (!isJsonReport(data)) {
+          continue;
+        }
+
+        addScenarioResult(data, scenarioResults);
+      }
     }
+  }
 
-    scenarioResults.push({
-      id: data.description.id,
-      data,
-      // Output used for console output when running locally/CI.
-      summaryConsoleText: `\
+  if (scenarioResults.length === 0) {
+    throw new Error(`No valid benchpress benchmark reports found in "${testlogDir}".`);
+  }
+
+  return {
+    scenarios: scenarioResults,
+    summaryConsoleText: scenarioResults
+      .map((s) => `${bold(s.id)}\n\n${s.summaryConsoleText}`)
+      .join('\n\n'),
+    summaryMarkdownText: scenarioResults
+      .map((s) => `### ${s.id}\n\n${s.summaryMarkdownText}`)
+      .join('\n\n'),
+  };
+}
+
+function addScenarioResult(data: JsonReport, scenarioResults: ScenarioResult[]) {
+  scenarioResults.push({
+    id: data.description.id,
+    data,
+    // Output used for console output when running locally/CI.
+    summaryConsoleText: `\
 ${data.metricsText}
 ${data.validSampleTexts.join('\n')}
 ${data.statsText}`,
-      // Output used for e.g. GitHub actions.
-      summaryMarkdownText: `\
+    // Output used for e.g. GitHub actions.
+    summaryMarkdownText: `\
 <details><summary>Full example results</summary>
 
 \`\`\`
@@ -77,21 +128,10 @@ ${data.statsText}
 ${data.metricsText}
 ${data.statsText}
 \`\`\``,
-    });
-  }
-
-  return {
-    scenarios: scenarioResults,
-    summaryConsoleText: scenarioResults
-      .map((s) => `${bold(s.id)}\n\n${s.summaryConsoleText}`)
-      .join('`\n'),
-    summaryMarkdownText: scenarioResults
-      .map((s) => `### ${s.id}\n\n${s.summaryMarkdownText}`)
-      .join('`\n'),
-  };
+  });
 }
 
 /** Whether the object corresponds to a benchpress JSON report. */
 function isJsonReport(data: any): data is JsonReport {
-  return data['completeSample'] !== undefined;
+  return data?.completeSample !== undefined;
 }

scripts/benchmarks/targets.mts

@@ -6,8 +6,9 @@
  * found in the LICENSE file at https://angular.dev/license
  */
 
+import fs from 'fs';
 import path from 'path';
-import {exec} from './utils.mts';
+import {exec, projectDir} from './utils.mts';
 
 /** Branded string representing a resolved Bazel benchmark target. */
 export type ResolvedTarget = string & {
@@ -17,10 +18,11 @@ export type ResolvedTarget = string & {
 /** Finds all benchmark Bazel targets in the project. */
 export async function findBenchmarkTargets(): Promise<string[]> {
   return (
-    await exec('bazel', [
+    await exec('pnpm', [
+      'bazel',
       'query',
       '--output=label',
-      `'kind("^web_test", //modules/...) intersect attr("name", "perf", //modules/...)'`,
+      `kind("^js_test|^web_test", //modules/...) intersect attr("name", "^perf$", //modules/...)`,
     ])
   )
     .split(/\r?\n/)
@@ -28,23 +30,27 @@ export async function findBenchmarkTargets(): Promise<string[]> {
 }
 
 /** Gets the testlog path of a given Bazel target. */
-export async function getTestlogPath(target: ResolvedTarget): Promise<string> {
-  return path.join(await bazelTestlogDir(), target.substring(2).replace(':', '/'));
+export async function getTestlogPath(
+  target: ResolvedTarget,
+  cwd: string = projectDir,
+): Promise<string> {
+  const symlinkPath = path.join(cwd, 'dist/testlogs', target.substring(2).replace(':', '/'));
+  if (fs.existsSync(path.join(cwd, 'dist/testlogs'))) {
+    return symlinkPath;
+  }
+  try {
+    const bazelTestlogs = (
+      await exec('pnpm', ['bazel', 'info', 'bazel-testlogs', '--lockfile_mode=update'], cwd)
+    ).trim();
+    return path.join(bazelTestlogs, target.substring(2).replace(':', '/'));
+  } catch (e) {
+    return symlinkPath;
+  }
 }
 
 /** Resolves a given benchmark Bazel target to the fully expanded label. */
 export async function resolveTarget(target: string): Promise<ResolvedTarget> {
-  // If the target does not specify an explicit browser test target, we attempt
-  // to automatically add the Chromium suffix. This is necessary for e.g.
-  // resolving testlogs which would reside under the actual test target.
-  if (!target.endsWith('_chromium')) {
-    target = `${target}_chromium`;
-  }
-
-  return (await exec('bazel', ['query', '--output=label', target])).trim() as ResolvedTarget;
-}
-
-let testlogDir: string | null = null;
-async function bazelTestlogDir(): Promise<string> {
-  return testlogDir ?? (testlogDir = (await exec('bazel', ['info', 'bazel-testlogs'])).trim());
+  return (
+    await exec('pnpm', ['bazel', 'query', '--output=label', '--', target])
+  ).trim() as ResolvedTarget;
 }

scripts/benchmarks/utils.mts

@@ -22,16 +22,16 @@ export const projectDir: string = path.join(scriptDir, '../..');
  * This ensures that special shell characters within arguments are treated as
  * literal values and cannot be used to inject additional commands.
  */
-export function exec(cmd: string, args: string[] = []): Promise<string> {
+export function exec(cmd: string, args: string[] = [], cwd: string = projectDir): Promise<string> {
   return new Promise((resolve, reject) => {
-    Log.info('Running command:', cmd, args.join(' '));
+    Log.info('Running command:', cmd, args.join(' '), `(in ${cwd})`);
 
     const proc = childProcess.spawn(cmd, args, {
       // Do not use a shell to spawn the process. This ensures that arguments
       // are passed directly to the executable without shell interpretation,
       // preventing injection via shell metacharacters.
       shell: false,
-      cwd: projectDir,
+      cwd,
       // Only capture `stdout`. Forward the rest to the parent TTY.
       stdio: ['inherit', 'pipe', 'inherit'],
     });