Align safe-outputs bundle pre-check with post-apply file detection#41457
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
| // bundle pre-check and post-apply verification aligned even when the | ||
| // patch artifact under-detects files (for example, merge-resolution | ||
| // content preserved only by the bundle transport). | ||
| { |
There was a problem hiding this comment.
@copilot refactor to own function, add git integration tests
There was a problem hiding this comment.
Addressed in 6cc5cdc: extracted the bundle pre-apply git diff into getBundlePreApplyFiles() and added git-backed integration coverage for fetched bundle refs, including merge-history bundles.
There was a problem hiding this comment.
Done in 6cc5cdc: getBundlePreApplyFiles() is extracted as its own function (line 114) and called at the bundle pre-check site (line 815). Integration tests covering fetched bundle refs, including merge-history bundles, are in push_to_pull_request_branch.integration.test.cjs.
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This change fixes a mismatch in safe-outputs bundle transport where pre-apply validation inferred changed files from the patch artifact, while post-apply verification used Git’s applied diff—leading to false-positive aborts. The bundle path now uses Git as the source of truth for pre-apply file detection, aligning both checks and adding regression coverage.
Changes:
- Add
getBundlePreApplyFiles()to compute the bundle’s authoritative file set viagit diff --name-only --no-renames <base>..<bundleRef>, and enforce the existing path-based protection logic pre-apply for bundle transport. - Add regression tests (unit + integration) ensuring multi-file bundles are detected correctly pre-apply and match post-apply verification behavior.
- Regenerate affected smoke workflow lockfiles (notably reflecting
strict: falsefrom the source.mdworkflows).
Show a summary per file
| File | Description |
|---|---|
| actions/setup/js/push_to_pull_request_branch.cjs | Adds Git-diff-based pre-apply file detection for bundle transport and applies protection checks against that authoritative file list. |
| actions/setup/js/push_to_pull_request_branch.test.cjs | Adds a focused unit regression test asserting bundle pre-apply detection sees the full multi-file set and matches post-apply diff ranges. |
| actions/setup/js/push_to_pull_request_branch.integration.test.cjs | Adds integration coverage exercising real git bundle fetch + getBundlePreApplyFiles to validate file listing in realistic repo states (including merge history). |
| .github/workflows/smoke-copilot.lock.yml | Regenerated lockfile; compiled strict value now reflects the source workflow’s strict: false. |
| .github/workflows/smoke-copilot-aoai-entra.lock.yml | Regenerated lockfile; compiled strict value now reflects the source workflow’s strict: false. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 5/5 changed files
- Comments generated: 0
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
❌ Test Quality Sentinel failed during test quality analysis. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR does not have the 'implementation' label and has 0 new lines of code in business logic directories (threshold: 100). |
🧪 Test Quality Sentinel Report🔶 Test Quality Score: 57/100 — Needs Improvement
📊 Metrics & Test Classification (3 tests analyzed)
Go: 0 ( Score breakdown:
|
There was a problem hiding this comment.
REQUEST_CHANGES — two issues in the new bundle pre-apply security check need fixing before merge.
### Blocking findings
1. Empty-diff silently bypasses policy (high, correctness): The if (bundleFiles.length > 0) guard at line 816 skips the entire authoritative check when git diff produces zero paths. This can occur if rangeBaseRef is stale, unresolvable, or — in a shallow clone — not yet locally available. The check was added specifically to catch files the patch under-detected; skipping it on an empty diff leaves the exact same gap.
2. No error handling for getExecOutput failure (medium, reliability): getBundlePreApplyFiles calls getExecOutput without ignoreReturnCode: true. A non-zero git exit — bad ref, missing object — throws into the outer catch (bundleError) which emits only "Failed to apply bundle", discarding the real cause and potentially failing valid bundle pushes that worked before this PR.
Both issues can be fixed together by adding ignoreReturnCode: true to the helper and making the empty/error path observable (a warning log) rather than a silent passthrough.
🔎 Code quality review by PR Code Quality Reviewer · 106.9 AIC · ⌖ 7.25 AIC · ⊞ 5.2K
| // content preserved only by the bundle transport). | ||
| { | ||
| const bundleFiles = await getBundlePreApplyFiles(exec, baseGitOpts, rangeBaseRef, bundleRef); | ||
| if (bundleFiles.length > 0) { |
There was a problem hiding this comment.
Silent policy bypass when bundle diff returns empty: if git diff resolves to zero paths — a stale rangeBaseRef (origin/${branchName} not tracked locally), objects missing in a shallow clone, or a bundle whose tip matches the base — the guard skips the entire authoritative check and the bundle applies without any file-scope validation.
💡 Impact and suggested fix
Before this PR, bundle transport never called git diff, so a new failure path now exists: in environments where rangeBaseRef is origin/${branchName} and the remote ref is not locally cached, the diff may return empty and silently bypass the check — while the first-pass patch check already passed (it only saw .changeset). This creates a hole precisely in the scenario this PR is trying to close.
The guard should fail closed, not open. Minimum fix: emit an observable warning and document the skip rather than treating an empty diff as safe:
const bundleFiles = await getBundlePreApplyFiles(exec, baseGitOpts, rangeBaseRef, bundleRef);
if (bundleFiles.length === 0) {
core.warning(`Pre-apply bundle verification: git diff returned no paths (rangeBaseRef=${rangeBaseRef}); check skipped`);
} else {
core.info(`Pre-apply bundle verification: ${bundleFiles.length} file(s) detected from bundle transport`);
const bundleProtection = checkFileProtectionPostApply(bundleFiles, config);
// deny/fallback handling unchanged
}Ideally also add a test asserting the empty-diff case logs a warning and does not silently succeed.
| // patch artifact under-detects files (for example, merge-resolution | ||
| // content preserved only by the bundle transport). | ||
| { | ||
| const bundleFiles = await getBundlePreApplyFiles(exec, baseGitOpts, rangeBaseRef, bundleRef); |
There was a problem hiding this comment.
Unhandled git error from getBundlePreApplyFiles collapses into generic bundle failure: getExecOutput throws on non-zero exit and there is no ignoreReturnCode: true or try/catch here — any git diff failure propagates into the outer catch (bundleError) which logs only "Failed to apply bundle", discarding the real cause.
💡 Why this matters / suggested fix
Before this PR, bundle transport never invoked git diff. Now it does, introducing a new failure mode: if rangeBaseRef is origin/${branchName} (the fallback when remoteHeadBeforePatch was not captured) and the remote ref is not locally available, git diff exits non-zero, the outer catch fires, and a valid bundle push fails with an unhelpful error.
Two options:
Option A — pass ignoreReturnCode: true and handle empty/error in the helper:
async function getBundlePreApplyFiles(exec, gitOptions, rangeBaseRef, bundleRef) {
const result = await exec.getExecOutput(
"git", ["diff", "--name-only", "--no-renames", `${rangeBaseRef}..${bundleRef}`],
{ ...gitOptions, ignoreReturnCode: true }
);
if (result.exitCode !== 0) {
core.warning(`Bundle pre-apply diff failed (rangeBaseRef=${rangeBaseRef}): ${result.stderr}`);
return [];
}
return result.stdout.split("\n").map(f => f.trim()).filter(Boolean);
}Option B — wrap the call in its own try/catch at the call site and log before falling through.
Either way, the error must be surfaced as a named warning, not swallowed by the bundle transport catch.
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnose and /tdd — requesting changes on two gaps: missing denial-path test coverage and missing error handling in getBundlePreApplyFiles.
📋 Key Themes & Highlights
Key Themes
- Missing security-path test: the pre-apply bundle check is a new security enforcement gate, but only the happy path (all files in scope → allow) is tested. A test that asserts
result.success === falsewhen the bundle contains out-of-scope files is needed to confirm the gate actually blocks. - Silent failure surface:
getBundlePreApplyFilespropagates an unhandled exception if git diff fails, and skips the entire check silently when git returns an empty file set — both paths should log or throw with context. - Naming friction: calling
checkFileProtectionPostApplyin a pre-apply context and then branching onsource === "post-apply"is correct but confusing; a short comment would help maintainers.
Positive Highlights
- ✅ Root cause well-targeted: using
git diff --name-onlyeliminates the patch-parser differential that caused false positives. - ✅ Bundle path isolated: patch transport is untouched, reducing blast radius.
- ✅ Integration tests with real git repos are a strong addition for the file-detection component.
- ✅
getBundlePreApplyFilesis correctly extracted and exported, keeping it independently testable. - ✅ Security comment in the implementation code clearly explains the why.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 81.6 AIC · ⌖ 10.7 AIC · ⊞ 6.5K
| * @returns {Promise<string[]>} | ||
| */ | ||
| async function getBundlePreApplyFiles(exec, gitOptions, rangeBaseRef, bundleRef) { | ||
| const bundleDiffResult = await exec.getExecOutput("git", ["diff", "--name-only", "--no-renames", `${rangeBaseRef}..${bundleRef}`], gitOptions); |
There was a problem hiding this comment.
[/diagnose] getBundlePreApplyFiles calls getExecOutput without ignoreReturnCode: true, so any git failure (e.g. a ref that unexpectedly does not exist) will throw an unhandled exception rather than produce a clear diagnostic message.
💡 Suggestion
Consider wrapping the call with ignoreReturnCode: true and surfacing git error output explicitly:
async function getBundlePreApplyFiles(exec, gitOptions, rangeBaseRef, bundleRef) {
const bundleDiffResult = await exec.getExecOutput(
"git",
["diff", "--name-only", "--no-renames", `${rangeBaseRef}..${bundleRef}`],
{ ...gitOptions, ignoreReturnCode: true }
);
if (bundleDiffResult.exitCode !== 0) {
throw new Error(`git diff for bundle pre-check failed (exit ${bundleDiffResult.exitCode}): ${bundleDiffResult.stderr}`);
}
return bundleDiffResult.stdout.split("\n").map(f => f.trim()).filter(Boolean);
}This is especially valuable because refs should always be present at this point in the flow, so a failure here indicates an unexpected state worth flagging loudly.
| // content preserved only by the bundle transport). | ||
| { | ||
| const bundleFiles = await getBundlePreApplyFiles(exec, baseGitOpts, rangeBaseRef, bundleRef); | ||
| if (bundleFiles.length > 0) { |
There was a problem hiding this comment.
[/diagnose] When getBundlePreApplyFiles returns an empty array, the pre-apply protection block is silently skipped with no log output. An empty bundle diff is almost certainly unexpected and warrants at least a core.debug message so operators can distinguish "protection ran and allowed" from "protection was not entered at all".
💡 Suggestion
const bundleFiles = await getBundlePreApplyFiles(exec, baseGitOpts, rangeBaseRef, bundleRef);
if (bundleFiles.length === 0) {
core.debug("Pre-apply bundle verification: git diff returned no files; skipping protection check");
} else {
core.info(`Pre-apply bundle verification: ${bundleFiles.length} file(s) detected from bundle transport`);
// ... existing protection logic
}This also makes it easier to diagnose future false-negatives where the diff range is wrong and git silently returns nothing.
| if (bundleProtection.action === "deny") { | ||
| const filesStr = bundleProtection.files.join(", "); | ||
| const msg = | ||
| bundleProtection.source === "post-apply" |
There was a problem hiding this comment.
[/diagnose] The bundleProtection.source === "post-apply" ternary is correct in behavior but the naming may confuse future maintainers: this is a pre-apply check that calls checkFileProtectionPostApply. The source value "post-apply" comes from checkFileProtectionPostApply's allowlist branch (line 286 of manifest_file_helpers.cjs), not from the timing of this call. A brief comment clarifying this would prevent confusion.
💡 Suggestion
// bundleProtection.source reflects which check fired:
// "post-apply" → file not in allowed_files list
// "protected" → file matched a protected-files rule
const msg =
bundleProtection.source === "post-apply"
? `Cannot push...`
: `Cannot push (protected files)...`;| const handler = await module.main({ allowed_files: [".changeset/**", "pkg/workflow/**"] }); | ||
| const result = await handler({ branch: "feature-branch", diff_size: 5 * 1024 }, {}); | ||
|
|
||
| expect(result.success).toBe(true); |
There was a problem hiding this comment.
[/tdd] This test only covers the happy path (all 4 files in scope → result.success === true). The pre-apply bundle check is a security-critical path that should also be covered by a test where the bundle contains files outside allowed_files and the push is denied.
💡 Missing test sketch
it("should deny push when bundle contains files outside allowed_files", async () => {
// same setup, but actualFiles includes a file not matching allowed_files globs
const actualFiles = [".changeset/patch-fix.md", "secrets/private.key"];
mockExec.getExecOutput.mockImplementation((cmd, args) => {
if (cmd === "git" && args[0] === "diff" && args[1] === "--name-only") {
return Promise.resolve({ exitCode: 0, stdout: actualFiles.join("\n") + "\n", stderr: "" });
}
// ... other mocks
});
const module = await loadModule();
const handler = await module.main({ allowed_files: [".changeset/**"] });
const result = await handler({ branch: "feature-branch", diff_size: 5 * 1024 }, {});
expect(result.success).toBe(false);
expect(result.error).toContain("bundle modifies files outside the allowed-files list");
});Without this, the security enforcement in the new bundle pre-check path has no regression test.
| if (cmd === "git" && args[0] === "rev-list") { | ||
| return Promise.resolve({ exitCode: 0, stdout: "2\n", stderr: "" }); | ||
| } | ||
| return Promise.resolve({ exitCode: 0, stdout: "", stderr: "" }); |
There was a problem hiding this comment.
[/tdd] The catch-all mock (return Promise.resolve({ exitCode: 0, ... })) silently swallows any unrecognised git command. If the implementation calls an unexpected git operation due to a bug, the test will still pass — exactly the failure mode we want tests to catch.
💡 Suggestion
Replace the catch-all with a rejecting fallback so unexpected calls surface immediately:
return Promise.reject(new Error(`Unexpected getExecOutput call: ${cmd} ${JSON.stringify(args)}`));This follows the principle that test doubles should be as strict as the real contract.
|
|
||
| const actualFiles = await getBundlePreApplyFiles(createExecApi(targetRepo), {}, baseSha, bundleRef); | ||
|
|
||
| expect(actualFiles.sort()).toEqual([".changeset/fix.md", "docs/guide.md"]); |
There was a problem hiding this comment.
[/tdd] The integration tests exercise getBundlePreApplyFiles in isolation, which is great for unit-level confidence. But the original bug was a false positive abort caused by mismatch between the pre-check and post-apply check. The integration suite would be more valuable if it also included a test exercising the full push_to_pull_request_branch handler with a bundle whose patch artifact under-counts files — verifying the push is not aborted when all bundle files are in scope.
💡 Suggestion
Add a third integration test that:
- Creates a bundle containing
.changeset/andpkg/workflow/files - Constructs a patch artifact that only mentions the
.changeset/file (the original bug scenario) - Runs the full handler with
allowed_files: [".changeset/**", "pkg/workflow/**"] - Asserts
result.success === true(not aborted) and that the pre-apply info log shows 4 files
This would be a direct regression test for the root-cause scenario described in the PR.
|
@copilot run pr-finisher skill |
|
@copilot run pr-finisher skill, merge main and recompile |
…ix-patch-parser # Conflicts: # .github/workflows/smoke-copilot-aoai-entra.lock.yml # .github/workflows/smoke-copilot.lock.yml Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Done — merged main (resolved lock file conflicts in the two smoke workflow lockfiles), recompiled all 251 workflows (no additional changes), and local validation passes. |
|
`` Please work through the open review threads and call out any remaining blockers.
|
|
`` Keep the response focused on the smallest remaining unblocker.
|
Safe-outputs was rejecting legitimate multi-file changeset bundles because the pre-apply path inferred modified files from the patch artifact while post-apply verification used Git’s actual applied diff. In bundle transport, that mismatch produced false-positive “patch-parser bypass” aborts.
Use Git as the source of truth for bundle pre-checks
push_to_pull_request_branch, bundle transport now computes the pre-apply file set from:Preserve existing patch behavior
Add regression coverage for multi-file bundles
.changesetfile while the bundle/post-apply diff contains additional source files.Example of the new bundle pre-check:
pr-sous-chef: update branch requested for https://github.com/github/gh-aw/actions/runs/28194350585