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, 4 Smelly

Findings

Sev	#	Area	File	What	Fix
🧄 Pungent	H1	Correctness	`build-tools/packages/build-cli/src/commands/check/trustPolicy.ts:409-413`	`spawn` is called with `shell: true` and the `args` array includes entries from `violationSet`, which are `name@version` strings extracted from pnpm's NDJSON output. On Unix, Node.js implements `shell: true` by joining all args and running them via `/bin/sh -c`, so any shell metacharacter in a package name or version (`;`, `&&`, `\|`, `$()`, backticks) is interpreted by the shell rather than passed literally. A lockfile referencing a package from a private or mirror registry whose name is not subject to npm's public validation — for example `foo; rm -rf ${HOME}` — would be extracted from pnpm's NDJSON event at line 488 and then injected verbatim into the shell command at each subsequent `--trust-policy-exclude` flag. Because this command is a security-auditing tool, shell injection through it is especially damaging.	Remove `shell: true` from the `spawn` options. Node.js passes each element of the `args` array as a distinct argv entry when `shell` is false, which prevents shell interpretation of metacharacters. If `shell: true` is required for path resolution on Windows, sanitize each violation string against a strict allowlist (`/^[\w@/.-]+$/`) before including it as a CLI flag, and throw if it does not match.
🧄 Pungent	H2	Testing	`build-tools/packages/build-cli/src/commands/check/trustPolicy.ts:285`	`collectPinnedVersions()` has zero test coverage despite complex parsing logic with multiple branches. It strips peer suffixes via `indexOf('(')`, locates the version boundary with `lastIndexOf('@')`, and filters URL/tarball refs with `/[/:]/.test(version)`. A regression in any branch silently produces a wrong package list — causing the audit to miss violations or include garbage entries. For example, a scoped package key like `@scope/pkg@1.0.0(@peer/dep@2.0.0)` must parse to `{name: '@scope/pkg', version: '1.0.0'}` and duplicates across `packages`+`snapshots` must be deduplicated — neither is exercised by any test.	Add unit tests for `collectPinnedVersions` covering: (1) scoped package with peer suffix — `@scope/pkg@1.0.0(@peer/dep@2.0.0)` → `{name:'@scope/pkg', version:'1.0.0'}`; (2) unscoped package without suffix — `lodash@4.17.21` → `{name:'lodash', version:'4.17.21'}`; (3) URL/tarball ref — `pkg@https://example.com/pkg.tgz` → filtered out; (4) same token in both `packages` and `snapshots` → appears exactly once in result; (5) key with no `@` at index > 0 (e.g. bare string) → skipped.
🧄 Pungent	H3	Testing	`build-tools/packages/build-cli/src/commands/check/trustPolicy.ts:455`	`extractTrustViolations()` has zero test coverage. The function pre-filters lines with `rawLine.includes(TRUST_DOWNGRADE_CODE)` and then throws if `event.code !== TRUST_DOWNGRADE_CODE`. If pnpm ever emits a line where `ERR_PNPM_TRUST_DOWNGRADE` appears inside a `message` string or stack trace (not in `code`), the pre-filter matches but the code check throws — crashing the command entirely instead of reporting violations. Similarly, the throw path for missing `package.name`/`package.version` is untested. Without tests, these fragile contract assumptions will silently break when pnpm's output format changes.	Add unit tests for `extractTrustViolations` covering: (1) valid NDJSON line with `code: 'ERR_PNPM_TRUST_DOWNGRADE'` and `package: {name, version}` → returns `['name@version']`; (2) multiple violations including duplicates → returns sorted, deduplicated array; (3) line containing the code string only in a `message` field (not `code`) → should throw with a clear error (verifying the existing behavior is intentional); (4) valid NDJSON line missing `package.version` → throws; (5) blank lines and unrelated NDJSON lines → silently skipped.
🧅 Smelly	M1	Correctness	`build-tools/packages/build-cli/src/commands/check/trustPolicy.ts:198,258-263`	The `rmSync(tempDir, ...)` cleanup at lines 258-260 is not inside a `try/finally` block. `extractTrustViolations` at line 198 is explicitly designed to `throw` when pnpm's NDJSON output format changes (e.g., a missing `package.name`, an unrecognized `code` field, or a malformed JSON line). When it throws, the exception propagates through `run()` without executing the cleanup code, leaving the scratch workspace on disk indefinitely — even though `--keep` was never requested. On subsequent runs, `writeAuditWorkspace` deletes and recreates the directory, so the stale tree is eventually removed, but any CI run that fails between those two points leaves `.trust-audit-temp/` behind, and a concurrent run writing to the same `tempDir` path will destroy the first run's workspace mid-iteration.	Wrap the `writeAuditWorkspace` call, the iteration loop, and the reporting block in a `try/finally`, and move the `rmSync` cleanup into the `finally` clause so it executes regardless of how `run()` exits.
🧅 Smelly	M2	Correctness	`build-tools/packages/build-cli/src/commands/check/trustPolicy.ts:372-397`	The collision-resolution scheme tracks usage counts by the base slug only. When two packages share the same base slug, the first gets `slug` and the second gets `slug-1`. But `usedSlugs` is never updated with the entry for `slug-1`. If a third package independently slugifies to `slug-1` (for example, a package named differently but whose non-alphanumeric characters collapse to the same string after `replace(/[^\dA-Za-z]+/g, '-')`), both that package and the collision-resolved second package will compute a final directory of `projects/slug-1/`. `mkdirSync` with `recursive: true` silently succeeds on the existing directory, and `writeFileSync` silently overwrites the `package.json`. The earlier package's dependency is dropped from the audit workspace without any error or warning, meaning one dependency is never checked by the trust policy scan.	After resolving a collision, insert the derived slug into `usedSlugs` so subsequent packages with the same natural slug detect the conflict: after `slug = \`${slug}-${collision}``, add` usedSlugs.set(slug, 1)`. Alternatively, track all final slugs in a single` Set` and keep incrementing the suffix until a free name is found.
🧅 Smelly	M3	Security	`build-tools/packages/build-cli/src/commands/check/trustPolicy.ts:409-414`	spawn() is called with shell: true while args include --trust-policy-exclude values derived from pnpm's NDJSON stdout (package names/versions from the audited lockfile). On Windows, Node.js serialises the entire args array into a single cmd.exe command string, so a package whose name contains cmd.exe metacharacters (&, \|, >, etc.) would cause arbitrary command execution. A malicious or compromised pnpm-lock.yaml entry whose name contains such characters would trigger this when the tool runs on a Windows CI agent: (1) lockfile key with a crafted name is parsed, (2) written into a scratch package.json dependency, (3) pnpm reports it as a trust violation, (4) extractTrustViolations returns name@version containing metacharacters, (5) that string is appended to the cmd.exe command line via the joined args array, executing attacker-chosen commands. The tool is explicitly designed to be run against PRs, making a supply-chain-poisoned lockfile a credible delivery mechanism.	Remove shell: true from the spawn options. pnpm is a known binary that needs no shell interpretation; passing an args array to spawn without a shell is both correct and safe. All metacharacter injection risk disappears because Node.js will exec the binary directly with the argv array unchanged.
🧅 Smelly	M4	Testing	`build-tools/packages/build-cli/src/commands/check/trustPolicy.ts:329`	`slugify()` is untested. Its output feeds directly into `writeAuditWorkspace()` as a filesystem directory name. If both the name and version are entirely non-alphanumeric (e.g. a hypothetical `---@---` lockfile key), both regex replacements yield empty strings, and after trimming leading/trailing hyphens `slugify` returns an empty string `"-"` or `""`. This would cause `mkdirSync` to receive a degenerate path and either silently overwrite unintended directories or fail with an opaque OS error. The collision-suffix logic in `writeAuditWorkspace` (lines 376-379) is also untested — two different `name@version` pairs producing the same slug must produce distinct directory names.	Add unit tests for `slugify`: (1) scoped package `@scope/pkg` + `1.0.0` → `scope-pkg-1-0-0`; (2) pre-release `pkg` + `1.0.0-beta.1` → `pkg-1-0-0-beta-1`; (3) all-special-char inputs → result is not empty/degenerate. Also add a test for `writeAuditWorkspace` where two entries produce the same base slug — verify they get distinct directory names (`slug` and `slug-1`).

View workflow run

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

tylerbutler · 2026-05-04T21:30:53Z

+ */
+
+import { spawn } from "node:child_process";
+import { existsSync, mkdirSync, rmSync, writeFileSync } from "node:fs";


Prefer the /promises imports and async for file i/o.

tylerbutler · 2026-05-04T21:39:47Z

+		const child = spawn("pnpm", args, {
+			cwd,
+			shell: true,
+			stdio: ["ignore", "pipe", "pipe"],
+			env: { ...process.env, CI: "1" },
+		});


We use a wrapper called execa that I would recommend using here. If you're especially forward thinking you could adopt tiny-exec now, which is what I would like to replace execa with eventually. This code works, but it's really low level and a higher level API would be easier to read and maintain (IMO).

tylerbutler · 2026-05-04T21:50:38Z

+			default: false,
+		}),
+		tempDir: Flags.directory({
+			description: "Scratch workspace directory (default: <repo-root>/.trust-audit-temp).",


Is there value to putting this in the repo boundary vs. a system-wide temp directory? E.g. /tmp on linux?

I've been going back and forth on the OS temp dir approach. On one hand, it doesn't clutter up the repo. On the other hand, it's harder to find. I think using the OS temp dir makes sense if you never have to inspect the output. The script exposes a "keep" flag for the case where you might want to examine the output. I think I want to keep it in the repo unless it is significantly simpler not to.

tylerbutler · 2026-05-04T21:56:18Z

+function slugify(name: string, version: string): string {
+	const safeName = name.replace(/[^\dA-Za-z]+/g, "-").replace(/^-+|-+$/g, "");
+	const safeVersion = version.replace(/[^\dA-Za-z]+/g, "-").replace(/^-+|-+$/g, "");
+	return `${safeName}-${safeVersion}`.toLowerCase();
+}


Honestly might be worth a tiny dep, otherwise just make sure it's got unit tests.

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.

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