From f697cd847ceeb3611e294af8ad87243372f65cd0 Mon Sep 17 00:00:00 2001 From: yaoweiprc <6896642+yaoweiprc@users.noreply.github.com> Date: Thu, 24 Sep 2026 10:58:59 +0800 Subject: [PATCH] chore(ci): make the cycle check runnable on Windows and report drift honestly (#10548) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two independent papercuts in the circular reference check, both hit while investigating a failure on another PR. `npm run check-cycle-references` could not run on Windows at all. It spawned the extensionless `node_modules/.bin/depcruise` shim through `execFileSync`, which Windows cannot launch; the `.cmd` sibling does not help either, since Node >=18 refuses to spawn `.cmd` without a shell (CVE-2024-27980). Resolve dependency-cruiser's own entry point and run it with `process.execPath` instead — no shell, works everywhere. The check also fails on baseline drift (cycles recorded in the baseline that no longer exist), which is correct: the baseline is a ratchet, and leaving a fixed cycle in it would silently keep permitting its reintroduction. But CI labelled every non-success outcome "New circular references detected", so a PR that *removed* two cycles was reported as having added some. That cost real debugging time. Give the script distinct exit codes — 1 for a new cycle, 2 for drift alone — and have the workflow capture and map them to separate messages. The failure behaviour is unchanged; only the wording is now accurate. Renamed the final step to match what it actually gates on. Co-authored-by: Claude Opus 5 (1M context) --- .github/workflows/test.yml | 29 +++++++++++++++---- .../check-cycle-references.mjs | 25 ++++++++++++++-- 2 files changed, 46 insertions(+), 8 deletions(-) diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index b926e0bad6..8692dc5968 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -107,7 +107,15 @@ jobs: id: check-cycles continue-on-error: true shell: bash - run: npm run check-cycle-references | tee cycle-report.txt + # Exit code 1 (a new cycle) and 2 (a stale baseline) are reported differently below, so + # capture it rather than letting `-e` collapse both into a generic step failure. + run: | + set +e + npm run check-cycle-references | tee cycle-report.txt + status=${PIPESTATUS[0]} + set -e + echo "status=$status" >> "$GITHUB_OUTPUT" + exit "$status" - name: Post circular references PR comment if: github.event_name == 'pull_request' && always() && steps.check-cycles.outcome != 'skipped' @@ -117,12 +125,21 @@ jobs: script: | const fs = require('fs'); - const passed = '${{ steps.check-cycles.outcome }}' === 'success'; + // 0 = matches baseline, 1 = a new cycle appeared, 2 = baseline is stale. Anything + // else means the check itself blew up, which is not a claim about cycles either way. + const status = '${{ steps.check-cycles.outputs.status }}'; + const passed = status === '0'; + const summaryByStatus = { + '0': { icon: '✅', text: 'No new circular references' }, + '1': { icon: '⚠️', text: 'New circular references detected' }, + '2': { icon: '♻️', text: 'No new circular references, but the baseline is stale — regenerate and commit it' }, + }; + const { icon, text } = summaryByStatus[status] ?? { icon: '💥', text: 'The circular reference check failed to run' }; const report = fs.readFileSync('cycle-report.txt', 'utf8').trim(); const markdownContent = `
- ${passed ? '✅' : '⚠️'} Circular References Report + ${icon} Circular References Report - **Status:** ${passed ? 'No new circular references' : 'New circular references detected'} + **Status:** ${text} \`\`\` ${report} @@ -170,6 +187,8 @@ jobs: console.log('Created new PR comment'); } - - name: Fail if new circular references found + # Fails on a stale baseline too, not just on new cycles: the baseline is a ratchet, so a + # cycle left in it after being fixed would silently keep permitting its reintroduction. + - name: Fail if the circular reference check did not pass if: steps.check-cycles.outcome == 'failure' run: exit 1 diff --git a/scripts/circular-references/check-cycle-references.mjs b/scripts/circular-references/check-cycle-references.mjs index 44f5cf72e4..8527ce4db9 100755 --- a/scripts/circular-references/check-cycle-references.mjs +++ b/scripts/circular-references/check-cycle-references.mjs @@ -3,16 +3,28 @@ // Runs dependency-cruiser per workspace package to find circular imports, and fails only on // cycles that are not already recorded in the baseline file. Run with --update-baseline to // regenerate the baseline from the current state (e.g. after fixing a cycle). +// +// Exits 0 when the tree matches the baseline, 1 when a new cycle appeared, and 2 when the +// baseline is merely stale. CI distinguishes the last two when reporting. import { execFileSync } from 'node:child_process'; import fs from 'node:fs'; import path from 'node:path'; import { fileURLToPath } from 'node:url'; const repoRoot = path.resolve(path.dirname(fileURLToPath(import.meta.url)), '..', '..'); -const depcruiseBin = path.join(repoRoot, 'node_modules', '.bin', 'depcruise'); +// Resolve dependency-cruiser's own entry point rather than the `.bin` shim, and run it with the +// current Node binary. On Windows the shim is split into an extensionless shell script (which +// `execFileSync` cannot launch) and a `.cmd` (which Node >=18 refuses to spawn without a shell, +// see CVE-2024-27980), so going through `.bin` makes this script POSIX-only for no benefit. +const depcruiseBin = path.join(repoRoot, 'node_modules', 'dependency-cruiser', 'bin', 'dependency-cruise.mjs'); const configPath = path.join(path.dirname(fileURLToPath(import.meta.url)), 'dependency-cruiser.json'); const baselinePath = path.join(path.dirname(fileURLToPath(import.meta.url)), 'known-violations.json'); +/** A cycle exists that the baseline does not record — a regression. */ +const EXIT_NEW_CYCLES = 1; +/** The baseline records cycles that no longer exist — it just needs regenerating. */ +const EXIT_BASELINE_DRIFT = 2; + // Same scope as `npm run lint`/`type-check`/`test` (--workspaces --if-present): the packages // actually declared as npm workspaces, not every directory under packages/ (e.g. // insomnia-component-docs is intentionally excluded and its deps aren't installed at the root). @@ -48,7 +60,11 @@ function cruisePackage({ name, dir }) { let stdout; try { - stdout = execFileSync(depcruiseBin, args, { cwd: dir, encoding: 'utf8', maxBuffer: 1024 * 1024 * 100 }); + stdout = execFileSync(process.execPath, [depcruiseBin, ...args], { + cwd: dir, + encoding: 'utf8', + maxBuffer: 1024 * 1024 * 100, + }); } catch (error) { if (error?.status !== 1 || !error.stdout) throw error; // dependency-cruiser exits with status 1 when forbidden violations are found. Parse its JSON @@ -143,7 +159,10 @@ function main() { if (hasNew) console.log('\nNew circular dependencies detected that are not in the baseline.'); if (hasDrift) console.log('\nBaseline contains circular dependencies no longer present in the current tree.'); console.log(`Run "npm run check-cycle-references:baseline" and commit the updated ${path.basename(baselinePath)}.`); - process.exit(1); + // Both cases need the baseline regenerated, but they mean opposite things — a new cycle is a + // regression, while drift alone means cycles were fixed and the baseline was left behind. + // Exit 1 vs 2 so CI can say which happened instead of reporting every failure as "new". + process.exit(hasNew ? EXIT_NEW_CYCLES : EXIT_BASELINE_DRIFT); } console.log('\nNo new circular dependencies.'); }