root/fusion
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

fix(merger): quote pnpm filter args + git refs, widen out-of-scope detection

Browse Source 
Code-review fixes for 036387610 / d02cd38d7:

1. `deriveScopedPnpmTestCommand` now passes each `--filter` argument through
   `quoteArg`. Package names come from workspace `package.json` files which
   are not strictly trusted input — a metacharacter in a name would have
   leaked into the shell command.

2. `getBranchChangedFiles` now quotes both git refs in the `<base>...<head>`
   range. Branch names can legally contain `/` and other characters; this
   is defense-in-depth consistent with the rest of merger.ts.

3. Out-of-scope detection is now package-aware via a new
   `packageNamesForFiles` helper. A failure in `__tests__/foo.test.ts` is
   correctly treated as in-scope when the branch touched `src/foo.ts` in
   the same package, whereas the previous filename-prefix heuristic missed
   that case entirely. The dead `bf.startsWith(ff/)` clause is removed.
   Falls back to the directory-prefix heuristic when pnpm-workspace.yaml
   is unavailable.

195 tests pass across merger-verification.test.ts and the reused
merge-reuse-task-worktree.test.ts.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
gsxdsm
2026-05-22 22:06:18 -07:00
parent 848a226cab
commit ed4575cc6f
1 changed files with 46 additions and 9 deletions
Download Patch File Download Diff File Expand all files Collapse all files

55
packages/engine/src/merger.ts
View File

@@ -870,6 +870,28 @@ export function mapChangedFilesToPackageNames(
return Array.from(nameSet);
}
/**
* Best-effort: map a list of repo-relative file paths to the pnpm package
* names they belong to. Returns an empty array if pnpm-workspace.yaml is
* missing or unparseable — callers fall back to a directory-based heuristic.
*
* @internal Exported for testing only.
*/
export function packageNamesForFiles(rootDir: string, files: string[]): string[] {
if (files.length === 0) return [];
let workspaceContent: string;
try {
workspaceContent = readFileSync(join(rootDir, "pnpm-workspace.yaml"), "utf-8");
} catch {
return [];
}
const globs = parsePnpmWorkspaceGlobs(workspaceContent);
if (globs.length === 0) return [];
const packageRoots = resolveWorkspacePackageRoots(rootDir, globs);
if (packageRoots.length === 0) return [];
return mapChangedFilesToPackageNames(files, packageRoots, rootDir);
}
/**
* Attempt to derive the set of pnpm package names touched by the branch.
* Returns null when scoping cannot be determined (missing git context, no
@@ -921,8 +943,11 @@ export function deriveScopedPnpmTestCommand(
}
// 5. Compose the scoped pnpm command
// `...^` includes dependents (packages that import the changed packages)
const filters = packageNames.map((name) => `--filter "${name}...^"`).join(" ");
// `...^` includes dependents (packages that import the changed packages).
// Package names come from workspace package.json files (potentially
// untrusted) so we quote each filter argument via `quoteArg` to prevent
// shell interpolation if a name contains metacharacters.
const filters = packageNames.map((name) => `--filter ${quoteArg(`${name}...^`)}`).join(" ");
return `pnpm ${filters} test`;
}
@@ -1113,12 +1138,13 @@ export function parseFailingFilesFromOutput(output: string): string[] {
*/
export function getBranchChangedFiles(rootDir: string, baseBranch: string, branch: string): string[] {
try {
// Use the branch ref directly when it's not HEAD
const range = branch === "HEAD"
? `${baseBranch}...HEAD`
: `${baseBranch}...${branch}`;
// Quote both refs — branch names can legally contain `/` and other
// characters that, while harmless to git, would expose us to shell
// injection if a caller ever passed an unsanitized branch string.
const baseRef = quoteArg(baseBranch);
const headRef = branch === "HEAD" ? "HEAD" : quoteArg(branch);
const output = execSync(
`git diff --name-only ${range}`,
`git diff --name-only ${baseRef}...${headRef}`,
{ cwd: rootDir, stdio: "pipe", encoding: "utf-8" },
).toString();
return output.split("\n").map((f) => f.trim()).filter(Boolean);
@@ -1744,13 +1770,24 @@ ${failureContext.output.slice(0, VERIFICATION_LOG_MAX_CHARS)}
// file paths, check whether ALL failing files are outside the branch
// diff. If so, throw OutOfScopeVerificationError so the caller can mark
// the task failed immediately rather than retrying into limbo.
//
// Heuristic: a failing file is "in-scope" if any branch-changed file
// shares the same workspace package (preferred), or — when pnpm
// workspace info is unavailable — if the paths share a common
// directory prefix. The exact-match clause catches the trivial case
// (test failure in the same file the branch touched).
if (baseBranch && branch) {
const failingFiles = parseFailingFilesFromOutput(failureContext.output);
if (failingFiles.length > 0) {
const branchFiles = getBranchChangedFiles(rootDir, baseBranch, branch);
if (branchFiles.length > 0) {
const allOutOfScope = failingFiles.every((ff) =>
!branchFiles.some((bf) => bf === ff || ff.startsWith(`${bf}/`) || bf.startsWith(`${ff}/`)),
const branchPackages = new Set(packageNamesForFiles(rootDir, branchFiles));
const failingPackages = packageNamesForFiles(rootDir, failingFiles);
const hasPackageOverlap =
branchPackages.size > 0 &&
failingPackages.some((p) => branchPackages.has(p));
const allOutOfScope = !hasPackageOverlap && failingFiles.every((ff) =>
!branchFiles.some((bf) => bf === ff || ff.startsWith(`${bf}/`)),
);
if (allOutOfScope) {
const msg =