feat(build-cli): add `flub check trustPolicy` - audit lockfile against pnpm's no-downgrade trust policy by TommyBrosman · Pull Request #27191 · microsoft/FluidFramework

TommyBrosman · 2026-04-28T19:21:38Z

pnpm 10's trustPolicy: no-downgrade rejects installs where a package's publisher or signing key has changed since it was first trusted. Running it against our existing workspace only surfaces one violation at a time and isn't usable as a CI signal because we can't pre-populate the trusted-publishers store from a single run. This new command audits every name@version and reports the full violation set in one shot by generating a temporary workspace with a single large package.json containing every single { package, version } entry from the repo.

Tracking bug in the pnpm repo: pnpm/pnpm#10622

Description

New oclif command flub check trustPolicy in the build-cli package, with --json, --keep, --tempDir, and --verbose flags.
Lockfile enumeration parses the pnpm lockfile and collects every unique name@version from both the packages and snapshots sections, skipping URL, tarball, and git refs (which trust policy doesn't apply to).
Install loop runs pnpm install --trust-policy no-downgrade --reporter ndjson, parses pnpm's NDJSON stdout for ERR_PNPM_TRUST_DOWNGRADE events, adds each offender to --trust-policy-exclude, and re-runs until pnpm exits cleanly or stops surfacing new violations.
Strict NDJSON parsing throws with the offending line on JSON-parse failure, unrecognized event codes, or missing package.{name,version} so any pnpm contract change surfaces loudly instead of silently skipping violations.
Structured output prints a human-readable report by default or a stable JSON document with tempDir, exitCode, iterations, elapsedSec, totalUniqueDependencies, and the sorted violations list under --json.

Sample run on this branch: 8 iterations / 2606 unique dependencies / ~82s, surfacing 7 violations (@langchain/core@0.3.80, @octokit/endpoint@9.0.6, axios@0.30.3, chokidar@4.0.3, detect-port@1.6.1, semver@5.7.2, semver@6.3.1).

the pnpm trust policy. Details: - Treats trust as binary (has-provenance vs no-provenance). pnpm has finer tiers (trusted publisher > provenance > none) but the registry's public abbreviated metadata only reliably exposes provenance. So this script may miss trusted-publisher -> provenance regressions, but won't produce false positives. - Uses publish time from the npm registry to determine "earlier" versions, matching pnpm's documented "based solely on publish date" wording. - Ignores prereleases when evaluating stable versions (matches pnpm v10.24+ behavior). - Respects trustPolicyExclude from pnpm-workspace.yaml. - Doesn't model trustPolicyIgnoreAfter (yet) This script is a little verbose as it manually parses the lockfile (see parseLockfilePackages) instead of calling a pnpm API. It also parses it as text instead of YAML.

github-actions · 2026-04-28T19:22:37Z

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

Based on the diff (460 lines, 3 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

…ions). - Added brackets around if statement bodies.

Co-authored-by: Copilot <copilot@github.com>

Copilot

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

Adds a new flub check trustPolicy command to audit the repo's pnpm-lock.yaml against pnpm 10's trustPolicy: no-downgrade behavior and report the full set of violations.

Changes:

Adds a new oclif command that enumerates lockfile entries, builds a temporary audit workspace, and iteratively reruns pnpm install while excluding discovered trust-policy offenders.
Adds command documentation for flub check trustPolicy and its flags/output modes.
Ignores the default scratch directory used by the new audit command.

Reviewed changes

Copilot reviewed 2 out of 3 changed files in this pull request and generated 3 comments.

File	Description
build-tools/packages/build-cli/src/commands/check/trustPolicy.ts	Implements the new trust-policy audit command, lockfile parsing, temp workspace generation, pnpm execution, and NDJSON parsing.
build-tools/packages/build-cli/docs/check.md	Documents the new `flub check trustPolicy` command and its CLI flags.
.gitignore	Ignores the default `.trust-audit-temp/` scratch directory created by the command.

github-actions · 2026-05-04T20:32:28Z

On it! Starting review with: Correctness, Security, API Compatibility, Testing

github-actions · 2026-05-04T20:35:27Z

🔭 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: ❌ Request Changes

0 Spicy, 3 Pungent, 2 Smelly

Findings

Sev	#	Area	File	What	Fix
🧄 Pungent	H1	Correctness	`build-tools/packages/build-cli/src/commands/check/trustPolicy.ts:586`	`extractTrustViolations` throws an unhandled exception when any NDJSON line contains the string "ERR_PNPM_TRUST_DOWNGRADE" but `event.code` is not exactly that value. pnpm's `--reporter ndjson` output may include secondary events (hint events, summary lines, or debug logs) that reference this error code in a `message`, `hint`, or `err.message` field. When such a line passes the `rawLine.includes(TRUST_DOWNGRADE_CODE)` pre-filter and parses as valid JSON, the strict `event.code !== TRUST_DOWNGRADE_CODE` branch throws, propagating out of the `while(true)` loop. The `finally` block cleans up the temp dir and the entire command exits as an unhandled error — all violations found so far are silently discarded and never reported.	Replace the hard throw with a `continue` (skip the event) when `event.code` does not match. Reserve throwing only for the case where the line is not parseable as JSON at all. Optionally log a verbose warning so contract changes in pnpm are still surfaced: `this.verbose(\`Skipping NDJSON line with unexpected code: ${rawLine}`)`.
🧄 Pungent	H2	Testing	`build-tools/packages/build-cli/src/commands/check/trustPolicy.ts:400`	`collectPinnedVersions()` has no tests at all. This function is the sole source of truth for which `(name, version)` pairs get audited. Without tests, the following bugs would ship silently: (1) an entry whose `resolved` field is a non-http URL (e.g. `file:`, `git+https:`) could be incorrectly included if the regex pre-filter is ever widened; (2) the cross-project deduplication via `seen` Set is untested — a duplicate `(name, version)` surfaced by two projects could accidentally be dropped or double-counted if the Set logic regresses; (3) `npm:`-aliased packages rely on `entry.from` (the registry name) being distinct from the alias key — this critical aliasing behaviour is completely unexercised.	Add unit tests in `src/test/commands/check/trustPolicy.test.ts` (or `src/test/library/trustPolicy.test.ts`) that call `collectPinnedVersions` directly with fixture JSON strings: (a) a multi-project array where the same `(name, version)` appears in two projects — assert it appears exactly once in the output; (b) entries with `resolved` starting with `file:`, `git+https:`, and `workspace:` — assert they are excluded; (c) an `npm:`-aliased entry where `from` differs from the map key — assert the output uses `from` (the registry name), not the alias key; (d) an empty `[]` input — assert the output is also empty.
🧄 Pungent	H3	Testing	`build-tools/packages/build-cli/src/commands/check/trustPolicy.ts:565`	`extractTrustViolations()` has no tests. This function controls which packages end up in the violation list that CI acts on. Three throw paths and one happy path are completely unexercised: (1) a line that contains `ERR_PNPM_TRUST_DOWNGRADE` as a substring but is invalid JSON should throw — if that throw is accidentally removed the loop silently skips real violations; (2) a parseable JSON line where `event.code` contains the string but does not equal the constant should throw — no test catches a regression here; (3) a valid event with `event.package.name` or `event.package.version` missing should throw; (4) happy path: a well-formed NDJSON stream with one violation line among many non-violation lines should return exactly that one `PinnedVersion`.	Add unit tests calling `extractTrustViolations` directly: (a) empty string → empty array; (b) NDJSON with unrelated lines only → empty array; (c) one well-formed trust-downgrade event line mixed with non-event lines → `[{ name, version }]`; (d) multiple trust-downgrade events in one stream → correct array in emission order; (e) a line containing `ERR_PNPM_TRUST_DOWNGRADE` that is not valid JSON → should throw with a message including the raw line; (f) a valid JSON line whose `event.code` is a string containing but not equal to the constant → should throw; (g) a valid event where `package.name` is missing → should throw.
🧅 Smelly	M1	Correctness	`build-tools/packages/build-cli/src/commands/check/trustPolicy.ts:223`	The `--trust-policy-exclude` flag is built by joining multiple versions with `\|\|` (no spaces): e.g. `semver@5.7.2\|\|6.3.1`. pnpm's own documentation example uses spaces around the operator: `webpack@4.47.0 \|\| 5.102.1`. If pnpm's version-union parser splits on `\|\|` (with surrounding spaces) rather than bare `\|\|`, the multi-version exclusion is silently mis-parsed as a single invalid version string. Consequence: on the next iteration, a previously-found violation is re-reported; `newViolations.length === 0`; `auditIncomplete = true` is set and the loop breaks. The violations array remains accurate, but CI receives a spurious "audit incomplete" signal for any package that has two or more violating versions, preventing automation from distinguishing a real failure from a parsing artefact.	Use `\|\|` (with surrounding spaces) as the join separator to match the format shown in pnpm's documentation: `versions.join(' \|\| ')`. Verify the resulting exclude flag value against a live pnpm 10 install if possible.
🧅 Smelly	M2	Testing	`build-tools/packages/build-cli/src/commands/check/trustPolicy.ts:237`	`countViolations()` is called in every audit iteration to report progress but has no test. More importantly, the multi-violation-per-name accumulation logic in `run()` (lines 268-276, building `violationsByName`) is untested: if two different versions of the same package (e.g. `semver@5.7.2` and `semver@6.3.1`) are each returned as violations in separate iterations, both must end up in the same Set under the same name key so they are emitted as a single `--trust-policy-exclude semver@5.7.2\|\|6.3.1` flag. A regression where the second version starts a new Set would produce two separate `--trust-policy-exclude semver@...` flags; pnpm silently drops the second one, so the second version would never be excluded and the audit would loop forever.	Add a test (can be a pure function test or a command integration test with mocked `runPnpm`) that simulates two pnpm runs: first run returns violation `semver@5.7.2`, second run returns violation `semver@6.3.1` for the same name. Assert that after both iterations the exclusion flags passed to pnpm contain `semver@5.7.2\|\|6.3.1` in a single `--trust-policy-exclude` argument, not two separate flags.

View workflow run

- --lockfile-only when installing (better performance). - Linked bug in pnpm repo.

Co-authored-by: Copilot <copilot@github.com>

…rkspace root now comes from this.getBuildProject(this.flags.path).root instead of getContext(). - Added path flag — Flags.directory({ exists: true }) so the command can target a build project outside the current working directory. - Adopted oclif's built-in JSON mode — static readonly enableJsonFlag = true;, removed the hand-rolled --json flag, and run() now returns a typed TrustPolicyAuditResult. Exit-on-failure gated on !this.jsonEnabled() so JSON consumers still get the structured payload. - Restructured violation tracking as Map<name, Set<version>> — replaced the flat Set<string> of name@version tokens. The exclude-flag builder now reads versions directly from the map (no lastIndexOf("@") parsing); the || union construction itself was already in place from the previous commit. - Violations returned as objects, not strings — TrustPolicyAuditResult.violations and extractTrustViolations both now return PinnedVersion[] ({name, version}). Text output formats ${name}@${version} inline. - Removed the gross sort helper — extractTrustViolations returns events in pnpm's emission order (caller dedupes via the map). The single sort that matters (final output stability) is inlined where the result array is built. - Misc cleanup — removed backticks from verbose log strings; added countViolations helper.

- .log -> .info

…red under any workspace, not just the repo root (since the audit now picks the most specific workspace and writes its temp dir there). Per-workspace audit. --path now selects the most specific workspace (longest-prefix match) inside the build project containing the path, rather than always auditing the build-project root. This lets the command target a single release group (e.g. build-tools, routerlicious) instead of the whole repo. Updated --path and --tempDir flag descriptions accordingly. pnpm list -r and the scratch dir are now anchored to the chosen workspace, not the repo root. Removed the lockfile existence check. getBuildProject() already validates the workspace exists; the manual pnpm-lock.yaml check was redundant. info() instead of log() for the human-readable summary block, so --quiet suppresses it. --json mode already suppresses both, so the INFO: prefix doesn't leak into structured output. output-determinism doc comment added at the violations-flatten block, explaining that the sort exists so printed/JSON output is stable across runs (Map/Set preserve insertion order, which depends on pnpm's emission order). Best-effort post-run cleanup. The finally block's rmSync(tempDir, ...) is now wrapped in try/catch — failures (Windows file locks, AV holds, etc.) become a this.warning instead of masking the audit's exit code or violations list. Doc comment explains the rationale. Pre-run cleanup hardened. Removed the existsSync TOCTOU check in writeAuditWorkspace (since rmSync with force: true already no-ops on missing paths). Wrapped the rmSync itself in try/catch with a clear actionable error: "Cannot prepare scratch workspace: ... Delete it manually or pass a different --tempDir." Dropped the now-unused existsSync import.

- Replace the hand-rolled `node:child_process` spawn helper with `execa`, matching the rest of build-cli. Because execa resolves `pnpm.cmd` natively without a shell, we no longer need `shell: true` and can drop the workaround that double-quoted `--trust-policy-exclude name@v1||v2` values to keep cmd.exe from interpreting `||`. - Replace the local `slugify(name, version)` helper plus `usedSlugs` collision map with a single `GithubSlugger` instance, which handles filesystem-safe slugging and duplicate suffixing internally. Both deps were already in the build-cli package. - No behavior change: `--path build-tools --json` produces the same 5 violations in 6 iterations as before.

tylerbutler · 2026-05-05T18:54:36Z

+	} as const;
+
+	public async run(): Promise<TrustPolicyAuditResult> {
+		const searchPath = path.resolve(this.flags.path ?? process.cwd());


nit: you can give the flag a default value of process.cwd() in the flag declaration above - then this defensive code can go away.

tylerbutler · 2026-05-05T18:57:50Z

+		let workspaceDir: string | undefined;
+		for (const ws of buildProject.workspaces.values()) {
+			const dir = path.resolve(ws.directory);
+			if (
+				(searchPath === dir || searchPath.startsWith(`${dir}${path.sep}`)) &&
+				(workspaceDir === undefined || dir.length > workspaceDir.length)
+			) {
+				workspaceDir = dir;
+			}
+		}
+		if (workspaceDir === undefined) {
+			this.error(`No workspace in build project ${buildProject.root} contains ${searchPath}.`);
+		}


Hmmm, I see what you mean. You could consider taking a workspace name or release group name as a flag, but I think this is fine. Easy enough to refactor later if needed, and the comment is clear.

tylerbutler · 2026-05-06T17:15:33Z

Do a full build-tools local build and commit the results - that should address the build failures.

TommyBrosman and others added 5 commits May 1, 2026 12:27

Trust policy as a flub command.

b1008e7

Removed old script.

8bf8b0c

- Typing for pinned versions (no more string parsing in calling funct…

1743790

…ions). - Added brackets around if statement bodies.

Cleanup.

d19888d

Co-authored-by: Copilot <copilot@github.com>

Lint.

f8e1847

Co-authored-by: Copilot <copilot@github.com>

TommyBrosman changed the title ~~[DRAFT] Script: check pnpm trust policy~~ Add flub check trustPolicy - audit lockfile against pnpm's no-downgrade trust policy May 4, 2026

TommyBrosman marked this pull request as ready for review May 4, 2026 18:58

Copilot AI review requested due to automatic review settings May 4, 2026 18:58

Copilot AI reviewed May 4, 2026

View reviewed changes

Comment thread build-tools/packages/build-cli/src/commands/check/trustPolicy.ts Outdated

Comment thread build-tools/packages/build-cli/src/commands/check/trustPolicy.ts Outdated

Comment thread build-tools/packages/build-cli/src/commands/check/trustPolicy.ts

Copilot started reviewing on behalf of TommyBrosman May 4, 2026 19:25 View session

CraigMacomber reviewed May 4, 2026

View reviewed changes