chore(build-cli): reshape bundleSize library to pure-function primitives

[ChumpChief]

Implements AB#73449 — followup to PR #27298 (AB#73299), continuing the bundle-size rework. Replaces the class-based ADOSizeComparator with pure-function primitives, redesigns the comparison types, and pulls generic helpers out of the bundleSize library.

Highlights

• bundleSize/ADO/ and ADOSizeComparator go away. New primitives (compareJsonReportsByPackage, extractAnalyzerJsonsFromArtifact, readAnalyzerJsonsFromFileSystem, sourcePackageFromAnalyzerPath) compose inside check bundleSize.
• Type redesign. BundleData / BundlesComparison / PackageComparison. Field presence on each bundle entry encodes added vs removed vs both-sides — previously silently dropped asymmetric assets.
• Generic helpers promoted out of bundleSize/. library/azureDevops/getArtifactForCommit.ts (find a build for a commit, download its artifact). library/git/pickFreshestRemote.ts (PR feat(build-cli): rework bundle size check for inner-dev-loop use #27298's canonical-remote auto-detect, now parameterized via a URL-matcher callback).
• Drop the "command never throws" contract. With the diff moving to a follow-up GH Actions workflow rather than a CI pipeline, the kind: "error" result variant was over-engineered. Use oclif's this.error() for fatals — matches the rest of the codebase (bump.ts, list.ts, etc.).
• downloadArtifact HTTP-status check. Missing artifacts now surface "Failed to download artifact X from build Y (HTTP 404)" instead of fflate's cryptic "invalid zip data."

Followup

Converge getArtifactForCommit / getBaselineBuildMetrics / getBuildArtifactForSpecificBuild around a shared "find build by selector + download artifact" pattern (AB#73595).

🤖 Generated with Claude Code

[github-actions]

Hi! Thank you for opening this PR. Want me to review it?

Based on the diff (1217 lines, 21 files), I've queued these reviewers:

• Correctness — logic errors, race conditions, lifecycle issues
• Security — vulnerabilities, secret exposure, injection
• API Compatibility — breaking changes, release tags, type design
• Performance — algorithmic regressions, memory leaks
• Testing — coverage gaps, hollow tests

How this works

• Adjust the reviewer set by ticking/unticking boxes above. Reviewer toggles alone don't trigger anything.

• Tick Start review below to dispatch the review fleet.

• After review finishes, tick Start review again to request another run — it auto-resets after each dispatch.

• This comment updates as new commits land; your reviewer selections are preserved.

• Start review

[github-actions]

🔭 PR Review Fleet Report

Note

This report is generated by an experimental AI review fleet and is provided as a beta feature. Findings are a starting point for discussion, not a gate. Use your own judgement.

Verdict: ⚠️ Approve with Suggestions

0 Exterminate, 2 Squash, 4 Investigate

Findings

Sev	#	Area	File	What	Fix
🦟 Squash	H1	Testing	build-tools/packages/build-cli/src/library/bundleSize/compareJsonReports.ts:57	compareJsonReports and compareJsonReportsByPackage have zero test coverage. The new field-presence encoding for added/removed bundles (...(baseBundle && { base: baseBundle })) is the entire basis of the new comparison API — a bug here (e.g. wrong set-union logic, inverted undefined-check) would silently produce an empty or wrong PackageComparison and the command would either suppress real regressions or falsely report clean results. No test file for this module exists in the repo.	Add unit tests: (1) base and compare both have the same bundle → both base and compare present; (2) bundle only in base (package removed) → only base field present; (3) bundle only in compare (package added) → only compare field present; (4) package only in base map → emits its bundles under the package key with no compare side; (5) package only in compare map → emits its bundles with no base side; (6) both maps empty → empty PackageComparison.
🦟 Squash	H2	Testing	build-tools/packages/build-cli/src/library/bundleSize/sourcePackageFromAnalyzerPath.ts:16	sourcePackageFromAnalyzerPath is the sole key-extraction function for both sides of the comparison — used in extractAnalyzerJsonsFromArtifact (baseline) and readAnalyzerJsonsFromFileSystem (local). It has no tests. If it returns wrong keys (e.g., a scoped package like @scope/pkg/analyzer.json is keyed differently on each side, or Windows backslash normalization is broken), the two AnalyzerJsonByPackage maps will never share keys — causing every package to appear as simultaneously added AND removed with no actual size delta visible to the user.	Add unit tests: (1) @scope/pkg/analyzer.json → '@scope/pkg'; (2) Windows path @scope\\pkg\\analyzer.json → '@scope/pkg'; (3) top-level analyzer.json (no directory prefix) → undefined; (4) path with no analyzer.json suffix → undefined; (5) deeply nested path a/b/c/analyzer.json → 'a/b/c' (verifying the full prefix is returned, not just the immediate parent).
🐜 Investigate	M1	Correctness	build-tools/packages/build-cli/src/library/azureDevops/getArtifactForCommit.ts:88	BuildStatus.Cancelling is not included in the isActivelyRunning guard (which only covers NotStarted, InProgress, and Postponed). A build in Cancelling state slips through to the next check: candidates.every((b) => b.result !== BuildResult.Succeeded). Because a cancelling build has not yet set its result, b.result is undefined, which satisfies !== BuildResult.Succeeded, so the check passes and the code throws "All builds for commit X have completed but none succeeded." The word "completed" is factually wrong — the build is still in the process of being cancelled — and the message implies a permanent failure when the user may just need to wait a moment for the cancellation to finish.	Add BuildStatus.Cancelling to the isActivelyRunning predicate (or rename it to something like isNotYetSettled to cover all pre-terminal states). The check would then correctly report "Found an in-progress build ... none have succeeded yet" for cancelling builds.
🐜 Investigate	M2	Correctness	build-tools/packages/build-cli/src/library/bundleSize/extractAnalyzerJsonsFromArtifact.ts:24	JSON.parse(text) is called without any error handling. If the downloaded ADO artifact contains a corrupt or truncated analyzer.json entry, a raw SyntaxError: Unexpected token … propagates up through getArtifactForCommit (outside its try/catch wrapper) and crashes the command with no context about which package or commit was problematic. The error message says nothing that helps the user diagnose whether the artifact was partially written, incompatible, or tampered with.	Wrap the parse in a try/catch and rethrow with relativePath (and ideally the commit SHA) in the message, e.g. throw new Error(\Failed to parse analyzer.json for "${relativePath}"`, { cause: e })`.
🐜 Investigate	M3	Correctness	build-tools/packages/build-cli/src/library/bundleSize/readAnalyzerJsonsFromFileSystem.ts:45	JSON.parse(text) is called without error handling inside a Promise.all callback. If any locally-generated analyzer.json is malformed (e.g. a partial write from an interrupted pnpm bundle-analysis:collect), the promise rejects with a raw SyntaxError that carries no indication of which file failed. The outer await Promise.all(...) re-throws this directly, so the command crashes with an unhelpful error.	Wrap the parse in a try/catch and rethrow with join(rootPath, relativePath) in the message so the user can find and delete or regenerate the offending file.
🐜 Investigate	M4	Testing	build-tools/packages/build-cli/src/library/azureDevops/getArtifactForCommit.ts:332	findBuildIdForCommit has a documented multi-build scenario (manual re-runs, partial-success retries) with no test coverage. The isActivelyRunning predicate gates which error message fires; if it were accidentally inverted or the some vs every call swapped, a user with an in-progress build would see 'all builds failed' instead of 'wait for the running build', causing wasted retries or incorrect manual intervention. The anomaly branch (succeeded build but id === undefined) is also entirely untested.	Add unit tests: (1) two builds for the same commit — one failed, one in-progress → throws 'in-progress' error, NOT 'all failed'; (2) two builds — one succeeded with id: undefined, one failed → throws 'no usable build id'; (3) two succeeded builds → returns the id of the first succeeded one; (4) no builds for the commit → throws 'No build found'.

---

View workflow run

[Copilot]

Pull request overview

Refactors the bundleSize library from class-based ADOSizeComparator into composable pure functions, introduces new comparison types that distinguish added/removed/changed bundles via field presence, and promotes generic helpers (ADO build/artifact lookup, freshest-remote selection) out of the bundleSize folder. The check bundleSize command is rewired to compose these primitives directly and uses this.error() for fatal cases instead of a kind: "error" result variant.

Changes:

• Removed bundleSize/ADO/, compareBundles.ts, and bundleSize/utilities/; added new primitives: compareJsonReportsByPackage, extractAnalyzerJsonsFromArtifact, readAnalyzerJsonsFromFileSystem, sourcePackageFromAnalyzerPath.
• Added library/azureDevops/getArtifactForCommit.ts and HTTP-status check in downloadArtifact.ts; generalized pickFreshestCanonicalRemote to pickFreshestRemote(branch, filter) in library/git/.
• Rewrote commands/check/bundleSize.ts to use the new primitives, drop the SizeComparison discriminated union, and emit fatal failures via this.error().

Reviewed changes

Copilot reviewed 21 out of 21 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
build-tools/packages/build-cli/src/library/git/pickFreshestRemote.ts	Generalized canonical-remote helper to accept a URL filter callback.
build-tools/packages/build-cli/src/library/bundleSize/utilities/index.ts	Removed (helpers relocated/inlined).
build-tools/packages/build-cli/src/library/bundleSize/utilities/getBuilds.ts	Removed; getBuilds wrapper inlined into getArtifactForCommit.
build-tools/packages/build-cli/src/library/bundleSize/utilities/getAllFilesInDirectory.ts	Removed; inlined into readAnalyzerJsonsFromFileSystem.
build-tools/packages/build-cli/src/library/bundleSize/types.ts	Replaced BundleSummaries/BundleMetric* with BundleData/BundlesComparison/PackageComparison.
build-tools/packages/build-cli/src/library/bundleSize/sourcePackageFromAnalyzerPath.ts	New helper to derive source-package name from analyzer.json path.
build-tools/packages/build-cli/src/library/bundleSize/readAnalyzerJsonsFromFileSystem.ts	New: reads analyzer.json files from a local directory tree.
build-tools/packages/build-cli/src/library/bundleSize/index.ts	Re-exports the new primitives and types; drops removed exports.
build-tools/packages/build-cli/src/library/bundleSize/extractAnalyzerJsonsFromArtifact.ts	New: extracts analyzer.json files from a downloaded artifact.
build-tools/packages/build-cli/src/library/bundleSize/compareJsonReports.ts	New: compares per-package JsonReports producing a PackageComparison.
build-tools/packages/build-cli/src/library/bundleSize/compareBundles.ts	Removed (replaced by compareJsonReports).
build-tools/packages/build-cli/src/library/bundleSize/ADO/*	Removed; functionality replaced by new primitives.
build-tools/packages/build-cli/src/library/azureDevops/getArtifactForCommit.ts	New: encapsulates "find ADO build by commit and download artifact".
build-tools/packages/build-cli/src/library/azureDevops/downloadArtifact.ts	Surfaces non-2xx HTTP status as a clear error before fflate parsing.
build-tools/packages/build-cli/src/commands/check/bundleSize.ts	Rewired to compose new primitives; uses this.error() for fatals; new formatComparison renderer.