feat(build-cli): support pnpm catalog references in flub release report by alexvy86 · Pull Request #26614 · microsoft/FluidFramework

alexvy86 · 2026-03-02T20:36:29Z

Description

flub release report (and the release state machine) would throw TypeError: Invalid comparator: catalog:X when any package in the repo used pnpm catalog protocol references in its dependencies.

This PR adds a catalog.ts utility that reads pnpm-workspace.yaml and resolves catalog: / catalog:default / catalog:X references to their actual version ranges before any semver operations. Both getFluidDependencies and getPreReleaseDependencies in package.ts now resolve catalog references via a per-workspace-root cache, making getFluidDependencies async. The release report command is updated to await the now-async call.

Includes a testRepoCatalog fixture and unit/integration tests that cover the catalog resolution logic end-to-end.

This will enable us to use pnpm catalogs to simplify our version ranges across the repo.

Reviewer Guidance

The review process is outlined on this wiki page.

`flub release report` (and the release state machine) would throw `TypeError: Invalid comparator: catalog:X` when any package in the repo used pnpm catalog protocol references in its dependencies. Adds a `catalog.ts` utility that reads `pnpm-workspace.yaml` and resolves `catalog:` / `catalog:default` / `catalog:X` references to their actual version ranges before any semver operations. Both `getFluidDependencies` and `getPreReleaseDependencies` in `package.ts` now resolve catalog references via a per-workspace-root cache, making `getFluidDependencies` async. The `release report` command is updated to await the now-async call. Includes a `testRepoCatalog` fixture and unit/integration tests that cover the catalog resolution logic end-to-end. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

build-tools/packages/build-cli/src/test/library/package.test.ts

build-tools/packages/build-cli/src/library/catalog.ts

build-tools/packages/build-cli/src/library/pnpmCatalog.ts

build-tools/packages/build-cli/src/test/library/package.test.ts

- Rename catalog.ts → pnpmCatalog.ts for clarity - Remove redundant `|| catalogRef === "default"` in resolveCatalogVersion - Only swallow ENOENT in readPnpmCatalogs; propagate other read errors - Use distinct catalog vs. input versions in the "unchanged" test to confirm the catalog is not consulted for non-catalog strings - Tighten error regex in both `throws` tests to match the full message Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

Copilot

Pull request overview

This PR fixes a TypeError: Invalid comparator: catalog:X crash in flub release report (and the release state machine) when any package in the repo uses pnpm catalog protocol references (catalog: / catalog:default / catalog:X) in its dependencies. It adds a new pnpmCatalog.ts utility that reads pnpm-workspace.yaml and resolves catalog references to their actual version ranges before any semver operations.

Changes:

Adds pnpmCatalog.ts with readPnpmCatalogs() and resolveCatalogVersion() utilities plus the yaml npm dependency
Integrates catalog resolution in getPreReleaseDependencies() and getFluidDependencies() (now async) in package.ts, with per-workspace-root caching; updates report.ts to await the now-async call
Adds a testRepoCatalog fixture and unit/integration tests covering catalog resolution logic

Reviewed changes

Copilot reviewed 14 out of 15 changed files in this pull request and generated 8 comments.

Show a summary per file

File	Description
`build-tools/pnpm-lock.yaml`	Adds the `yaml@2.8.1` dependency for parsing `pnpm-workspace.yaml`
`build-tools/packages/build-cli/package.json`	Declares the new `yaml` runtime dependency
`build-tools/packages/build-cli/src/library/pnpmCatalog.ts`	New utility: reads pnpm catalog from `pnpm-workspace.yaml` and resolves `catalog:` references
`build-tools/packages/build-cli/src/library/package.ts`	Adds catalog resolution in both `getPreReleaseDependencies` and `getFluidDependencies` (now async), with per-workspace-root caching
`build-tools/packages/build-cli/src/commands/release/report.ts`	Adds `await` for now-async `getFluidDependencies` call
`build-tools/packages/build-cli/src/test/init.ts`	Exports path to new `testRepoCatalog` fixture
`build-tools/packages/build-cli/src/test/library/package.test.ts`	Unit tests for `resolveCatalogVersion` and integration test for `getFluidDependencies` with catalog refs
`build-tools/packages/build-infrastructure/src/test/data/testRepoCatalog/*`	New test fixture files: `pnpm-workspace.yaml`, `fluidBuild.config.cjs`, and package structures for `pkg-a`, `pkg-b`, and `pkg-c`

Files not reviewed (1)

build-tools/pnpm-lock.yaml: Language not supported

build-tools/packages/build-cli/src/library/package.ts

build-tools/packages/build-cli/src/library/pnpmCatalog.ts

build-tools/packages/build-cli/src/test/library/package.test.ts

build-tools/packages/build-cli/src/library/package.ts

build-tools/packages/build-cli/src/test/library/package.test.ts

tylerbutler · 2026-03-02T21:42:33Z

Thanks for the PR — I reviewed this with focus on correctness, reliability, and security/perf. I found two actionable items:

Misleading parse error after catalog resolution (build-tools/packages/build-cli/src/library/package.ts)
- In getFluidDependencies, parsing is done on resolvedVersion, but the error still reports dep.version:
  - current: throw new Error(\Failed to parse depVersion: ${dep.version}`)`
- This can hide the real bad value when a catalog: entry resolves to an invalid semver/range.
- Suggested fix: include resolvedVersion (and optionally original dep.version for context).
Test coverage gaps for new catalog behavior
- Missing direct unit tests for readPnpmCatalogs in pnpmCatalog.ts (especially malformed YAML + non-ENOENT file errors).
- Missing resolveCatalogVersion test for "catalog exists but package key is missing" (Package "..." not found in pnpm catalog ...).
- Missing coverage for catalog-resolution path in getPreReleaseDependencies (PR added behavior there but no dedicated test).

Security/performance-wise, I didn’t find any major concerns; the caching + async propagation look good.

build-tools/packages/build-cli/src/library/package.ts

build-tools/packages/build-cli/src/library/pnpmCatalog.ts

tylerbutler

Seems good though I don't like the async change. I'm surprised this is the only place that needs to understand the catalogs, though. Won't other tools continue to be confused, including vs code?

- Make readPnpmCatalogs synchronous; revert getFluidDependencies to sync - Use typed variable declaration for YAML.parse result instead of as-cast; guard against null return (empty/whitespace-only pnpm-workspace.yaml) - Import PnpmCatalogMap directly; use Map<string, PnpmCatalogMap> for cache - Fix import order in package.ts (pnpmCatalog after context, per alpha order) - Fix error message in getFluidDependencies to report resolvedVersion - Add missing test: package not found in an existing catalog - Remove redundant resolveCatalogVersion test from getFluidDependencies block - Run pnpm lint:fix and pnpm format Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

… error messages Store the resolved semver range (not the raw catalog: reference) in prereleasePackages/prereleaseGroups so that the version displayed to the user in the release prompts shows the actual version constraint rather than an opaque "catalog:X" string. Also update error messages to include both the original value from package.json and the resolved version when they differ, making it easier to diagnose catalog resolution issues. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

…dency The ignore-filter callback in getTscCommandDependencies already handled workspace: references, but catalog: references in the same build group would reach semver.satisfies() and throw "Invalid comparator: catalog:X". Extract the callback into a tested, exported shouldIgnoreBuildDependency function and add a guard: once a dep is confirmed to be in the same group, a catalog: version is treated as a real local dependency (return false / don't ignore), analogous to how workspace: is handled. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

… fixture The testRepoCatalog fixture package.json files were missing the homepage, repository (with directory), and private fields required by the npm-package-metadata-and-sorting, npm-package-license, and npm-private-packages policy checks, causing npm run policy-check to fail. Modeled after the existing testRepo fixture which already satisfies these policies. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

…Dependency catalog: references pin versions for external or cross-workspace packages; same-group packages always use the workspace: protocol. So encountering catalog: in the same-group path is unexpected — the right response is to ignore the dep (return true) rather than treating it as a confirmed local build dependency. Also remove @internal tag (not a convention used in build-tools) and restore the historical comment that was lost when the logic was extracted. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

alexvy86 · 2026-03-02T23:48:32Z

Won't other tools continue to be confused, including vs code?

Do you have an idea of what exactly would confuse VSCode? The only error that surfaced (and we noticed) when we tried pnpm catalogs last time was this issue with flub release report so that's what I focused on for now. I had Claude look for other potential problem codepaths and it called out getTscCommandDependencies which should be handled now, and I'm working through flub bump --updateAllDeps which seems like ideally it would want to update the entry in the catalog, but I'm wondering if we can just make it skip catalog: dependencies and log warnings about them for now. Thoughts? @tylerbutler

alexvy86 · 2026-03-02T23:52:38Z

flub bump --updateAllDeps

Seems like that flag is an escape hatch to restore old behavior which was arguably incorrect, and probably isn't being used? Under that assumption, I'm skipping handling catalog: dependencies in that code path.

alexvy86 · 2026-03-02T23:52:46Z

/azp run Build - build-tools

azure-pipelines · 2026-03-02T23:52:57Z

Azure Pipelines successfully started running 1 pipeline(s).

tylerbutler · 2026-03-03T00:37:30Z

flub bump --updateAllDeps

Seems like that flag is an escape hatch to restore old behavior which was arguably incorrect, and probably isn't being used? Under that assumption, I'm skipping handling catalog: dependencies in that code path.

I think this is the right call.

tylerbutler · 2026-03-03T00:41:13Z

Do you have an idea of what exactly would confuse VSCode?

I was thinking it would get confused about where, say, the biome bin is. But I think I'm comcpletely wrong about that. vscode cares about the actual files on disk in node_modules and that should be the same with or without catalogs.

I'm mostly worried about places where we bump versions or calculate versions. Even though we should skip those, it might be nice if, say, version parsing in version-tools threw a useful error or something when parsing them. But maybe that's not needed in practice. I would trust a Claude review on that front.

tylerbutler

I'm good with these as-is.

## Description - Introduces a `buildTools` catalog in `pnpm-workspace.yaml` for the client release group to centralize version management of `@fluid-tools/build-cli`, `@fluid-tools/version-tools`, `@fluidframework/build-tools`, and `@fluidframework/bundle-size-tools` across the release group, and updates all package.json files in the release group to leverage the catalog instead of explicit version specifiers Second attempt at #23935, made possible by having merged #26614. --------- Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>

alexvy86 commented Mar 2, 2026

View reviewed changes

alexvy86 marked this pull request as ready for review March 2, 2026 21:00

Copilot AI review requested due to automatic review settings March 2, 2026 21:00

Copilot started reviewing on behalf of alexvy86 March 2, 2026 21:00 View session

Copilot AI reviewed Mar 2, 2026

View reviewed changes

tylerbutler reviewed Mar 2, 2026

View reviewed changes

build-tools/packages/build-cli/src/library/package.ts Outdated Show resolved Hide resolved

tylerbutler reviewed Mar 2, 2026

View reviewed changes

build-tools/packages/build-cli/src/library/pnpmCatalog.ts Outdated Show resolved Hide resolved

tylerbutler reviewed Mar 2, 2026

View reviewed changes

alexvy86 and others added 6 commits March 2, 2026 22:30

Fix comments

571ad98

tylerbutler approved these changes Mar 3, 2026

View reviewed changes

alexvy86 merged commit be07c88 into microsoft:main Mar 3, 2026
25 checks passed

alexvy86 deleted the support-pnpm-catalogs branch March 3, 2026 16:18

alexvy86 mentioned this pull request Mar 3, 2026

build: Use pnpm catalogs #26634

Merged

Conversation

alexvy86 commented Mar 2, 2026

Description

Reviewer Guidance

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tylerbutler commented Mar 2, 2026

Uh oh!

Uh oh!

Uh oh!

tylerbutler left a comment

Choose a reason for hiding this comment

Uh oh!

alexvy86 commented Mar 2, 2026

Uh oh!

alexvy86 commented Mar 2, 2026

Uh oh!

alexvy86 commented Mar 2, 2026

Uh oh!

azure-pipelines bot commented Mar 2, 2026

Uh oh!

tylerbutler commented Mar 3, 2026

Uh oh!

tylerbutler commented Mar 3, 2026

Uh oh!

tylerbutler left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants