Pin safe-output patch application to recorded base commit SHA#34876
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR aims to reduce git am --3way failures in concurrent pipelines by ensuring patch-transport flows apply patches against the same recorded base_commit that was captured during patch generation, rather than whatever the remote branch tip happens to be at apply time.
Changes:
create_pull_request: create the working branch frompullRequestItem.base_commitwhen available (with fallback to base branch behavior).push_to_pull_request_branch: for patch transport, attempt to reset the working tree tomessage.base_commitprior togit am.- Add targeted tests covering both handlers’ use of
base_commit.
Show a summary per file
| File | Description |
|---|---|
| actions/setup/js/push_to_pull_request_branch.cjs | Adds pre-git am re-anchoring to recorded base_commit (patch transport only). |
| actions/setup/js/push_to_pull_request_branch.test.cjs | Adds a regression test asserting reset/cat-file calls for base_commit. |
| actions/setup/js/create_pull_request.cjs | Anchors new PR branch creation to base_commit when present. |
| actions/setup/js/create_pull_request.test.cjs | Adds a regression test asserting branch creation from base_commit. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 4/4 changed files
- Comments generated: 3
| if (remoteHeadBeforePatch && remoteHeadBeforePatch !== recordedBaseCommit) { | ||
| core.warning(`Remote PR branch advanced since patch generation (remote HEAD ${remoteHeadBeforePatch}, patch base ${recordedBaseCommit}); applying patch from recorded base commit`); | ||
| } | ||
| await exec.exec("git", ["reset", "--hard", recordedBaseCommit], baseGitOpts); |
| if (!hasBundleFile && message.base_commit) { | ||
| const recordedBaseCommit = String(message.base_commit).trim(); | ||
| if (recordedBaseCommit) { | ||
| try { | ||
| try { | ||
| await exec.exec("git", ["fetch", "origin", recordedBaseCommit, "--depth=1"], { | ||
| env: { ...process.env, ...gitAuthEnv }, | ||
| ...baseGitOpts, | ||
| }); | ||
| } catch (fetchError) { | ||
| core.info(`Note: could not fetch base_commit ${recordedBaseCommit} explicitly (${getErrorMessage(fetchError)}); will verify local availability next`); | ||
| } | ||
| await exec.exec("git", ["cat-file", "-e", recordedBaseCommit], baseGitOpts); |
| const recordedBaseCommit = pullRequestItem.base_commit; | ||
| if (recordedBaseCommit) { | ||
| core.info(`Using base_commit from safe output entry for patch apply: ${recordedBaseCommit}`); | ||
| try { | ||
| try { | ||
| await exec.exec("git", ["fetch", "origin", recordedBaseCommit, "--depth=1"]); | ||
| } catch (fetchError) { | ||
| core.info(`Note: could not fetch base commit ${recordedBaseCommit} explicitly (${fetchError instanceof Error ? fetchError.message : String(fetchError)}); will verify local availability next`); | ||
| } | ||
| await exec.exec("git", ["cat-file", "-e", recordedBaseCommit]); | ||
| branchBaseRef = recordedBaseCommit; | ||
| } catch (baseCommitError) { |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #34876 does not have the 'implementation' label and has 0 new lines of code in default business logic directories (≤100 threshold). |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
🧪 Test Quality Sentinel completed test quality analysis. |
|
✅ PR Code Quality Reviewer completed the code quality review. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
🔎 Code quality review by PR Code Quality Reviewer · sonnet46 1.2M
| env: { ...process.env, ...gitAuthEnv }, | ||
| ...baseGitOpts, | ||
| }); | ||
| } catch (fetchError) { |
There was a problem hiding this comment.
Stale-object hazard: a failed git fetch is silently swallowed, then cat-file -e succeeds against a locally-cached object that may belong to a completely different history (e.g. after a force-push on origin), causing reset --hard to land on the wrong commit with no error surfaced.
💡 Suggested fix
After a fetch failure, do not fall through to cat-file. Either skip the reset path entirely or surface a warning and abort:
try {
await exec.exec("git", ["fetch", "origin", recordedBaseCommit, "--depth=1"], { ... });
} catch (fetchError) {
core.warning(`Cannot fetch base_commit ${recordedBaseCommit}; skipping base-pinning to avoid stale-object reset: ${getErrorMessage(fetchError)}`);
// do NOT fall through — exit the outer try here
throw fetchError; // or just: return / continue with HEAD
}
await exec.exec("git", ["cat-file", "-e", recordedBaseCommit], baseGitOpts);If the fetch fails and you still want to try a local-only path, at minimum log a prominent warning explaining that the object's provenance is unverified.
🧪 Test Quality Sentinel Report
📊 Metrics & Test Classification (2 tests analyzed)
Test Classification Details
Language SupportTests analyzed:
|
There was a problem hiding this comment.
Test Quality Sentinel: 70/100. Test quality is acceptable - 0% of new tests are implementation tests (threshold: 30%). Both tests verify the security-critical base_commit pinning behavior with observable success assertions. Consider adding error-path coverage for the commit-not-found case in a follow-up.
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnose and /tdd — commenting with suggestions; no blocking issues that should prevent merge.
📋 Key Themes & Highlights
Key Themes
reset --hardsemantics deserve documentation: The push flow silently discards any post-generation commits on the PR branch. This is likely intentional (agent work replaces the branch from its exact base), but it would benefit from an explicit comment or warning so operators know what to expect when the branch has drifted for non-trivial reasons.git fetch origin <SHA>will almost always fail: GitHub doesn't serve arbitrary SHA refspecs. The inner catch swallows this predictably, making it log noise on every invocation. Either remove the fetch or replace it with a branch-name fetch.- Missing fallback tests in both test files: Happy-path coverage is good; the fallback paths (commit unavailable → revert to current HEAD / base branch) are untested. This is where future regressions are most likely to hide.
Positive Highlights
- ✅ Root cause correctly identified and addressed at both apply sites
- ✅ Fallback semantics are safe — degradation to existing behaviour on unavailability
- ✅ Consistent approach across
create_pull_requestandpush_to_pull_request_branch - ✅ Good use of
cat-file -eas the availability gate before attempting destructive operations - ✅ Regression tests added alongside the fix
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 1.2M
| core.warning(`Unable to use recorded base_commit ${recordedBaseCommit}; applying patch on current branch HEAD: ${getErrorMessage(baseCommitError)}`); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
[/diagnose] git reset --hard silently discards any commits that were pushed to the PR branch after patch generation (e.g. manual edits, review-driven commits). A warning is logged when remoteHeadBeforePatch !== recordedBaseCommit, but only in that case — and the warning doesn't distinguish "unrelated merge drift" from "intentional work on the branch".
💡 Suggestion
Consider surfacing a more prominent warning (or even a core.error) when the reset would throw away commits that are ancestors of remoteHeadBeforePatch but not of recordedBaseCommit. This would alert operators before work is silently lost:
// before reset --hard, check how many commits would be discarded
const { stdout: commitsBetween } = await exec.getExecOutput(
"git", ["log", "--oneline", `${recordedBaseCommit}..HEAD`], baseGitOpts
);
if (commitsBetween.trim()) {
core.warning(`Discarding ${commitsBetween.trim().split('\n').length} commit(s) on PR branch to re-anchor to base_commit`);
}Alternatively, document the intentional semantics ("agent work always replaces the branch from base_commit") so reviewers understand the blast radius.
| }); | ||
| } catch (fetchError) { | ||
| core.info(`Note: could not fetch base_commit ${recordedBaseCommit} explicitly (${getErrorMessage(fetchError)}); will verify local availability next`); | ||
| } |
There was a problem hiding this comment.
[/diagnose] git fetch origin <SHA> --depth=1 is unreliable on GitHub and most Git hosts — servers don't advertise arbitrary commit SHAs as fetchable refs. This will almost always throw, landing in the inner catch. The outer cat-file then determines real availability, which is the right gate — but the fetch attempt adds noise in logs on every invocation.
💡 Suggestion
If the intent is to ensure the commit is present before cat-file, try fetching the branch ref instead of the raw SHA:
// Fetch the branch to ensure recent commits are available, then verify SHA
await exec.exec("git", ["fetch", "origin", baseBranch, "--depth=20"], fetchOpts);Or simply omit the fetch entirely and rely on cat-file as the gate, adding a comment explaining that shallow clones in Actions may not have the commit, and the fallback handles that case.
| const result = await handler({ patch_path: patchPath, base_commit: "base-sha-123" }, {}); | ||
|
|
||
| expect(result.success).toBe(true); | ||
| expect(mockExec.exec).toHaveBeenCalledWith("git", ["cat-file", "-e", "base-sha-123"], expect.any(Object)); |
There was a problem hiding this comment.
[/tdd] The new test covers the happy path but not the fallback: when cat-file fails (commit unavailable), the code should continue applying the patch on current HEAD without reset --hard. Without a test, a future refactor could accidentally break the fallback and silently always fail in cross-repo or missing-object scenarios.
💡 Suggested additional test
it("should fall back to current HEAD when base_commit is not available locally", async () => {
const patchPath = createPatchFile();
mockExec.getExecOutput.mockResolvedValue({ exitCode: 0, stdout: "abc123\n", stderr: "" });
mockExec.exec.mockImplementation(async (cmd, args) => {
if (args[0] === "cat-file") throw new Error("not in object store");
return 0;
});
const module = await loadModule();
const handler = await module.main({});
const result = await handler({ patch_path: patchPath, base_commit: "unknown-sha" }, {});
expect(result.success).toBe(true);
// reset --hard should NOT have been called
const resetCall = mockExec.exec.mock.calls.find(([c, a]) => c === "git" && a[0] === "reset");
expect(resetCall).toBeUndefined();
});|
|
||
| expect(result.success).toBe(true); | ||
| expect(global.exec.exec).toHaveBeenCalledWith("git", ["cat-file", "-e", MOCK_BASE_COMMIT_SHA]); | ||
| const checkoutWithBaseCommit = global.exec.exec.mock.calls.find(([cmd, args]) => cmd === "git" && Array.isArray(args) && args[0] === "checkout" && args[1] === "-b" && args[3] === MOCK_BASE_COMMIT_SHA); |
There was a problem hiding this comment.
[/tdd] The test only exercises the happy path (cat-file succeeds → checkout uses base_commit). The fallback path — where cat-file throws — should also be tested to confirm branchBaseRef reverts to baseBranch. Without it, a typo in the catch block (e.g., accidentally using branchBaseRef = recordedBaseCommit after the error) would go undetected.
💡 Suggested additional test
it("should fall back to base branch when base_commit is unavailable", async () => {
global.exec = {
exec: vi.fn().mockImplementation(async (cmd, args) => {
if (cmd === "git" && args[0] === "cat-file") throw new Error("not in object store");
return 0;
}),
getExecOutput: vi.fn().mockResolvedValue({ exitCode: 0, stdout: "", stderr: "" }),
};
const { main } = require("./create_pull_request.cjs");
const handler = await main({});
const result = await handler({ title: "Test", body: "B", patch_path: patchFilePath, branch: "test-branch", base_commit: MOCK_BASE_COMMIT_SHA }, {});
expect(result.success).toBe(true);
// checkout -b should use baseBranch (index 3), NOT the base_commit SHA
const checkoutCall = global.exec.exec.mock.calls.find(([c, a]) => c === "git" && a[0] === "checkout" && a[1] === "-b");
expect(checkoutCall[1][3]).not.toBe(MOCK_BASE_COMMIT_SHA);
});
|
@copilot review all comments. Make sure patch and bundle routes are supported. |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Addressed in c47e451.
|
|
@copilot review and integration tests |
Reviewed. I ran the relevant patch/bundle integration + targeted tests on
All passed (229/229). No additional code changes were needed. |
|
@copilot review all review comments from GitHub-action actor |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Done. I reviewed all
Validation and targeted integration tests pass:
Remaining automated notes are test-style suggestions only (non-blocking). |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
|
@copilot add enough logging to debug issues in logs |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Implemented in c560de5. Added debug logging in both create/push handlers for patch and bundle routes, including apply transport selection, explicit bundle-missing fallback to patch, resolved patch base_commit logging, remote/apply base ref context, and bundle ref update context to make workflow log triage easier. |
|
|
In concurrent pipelines, patches are generated from one commit graph but were applied against a newer remote branch tip, causing frequent
git am --3wayfailures after unrelated merges. This change aligns apply-time checkout/reset with the samebase_commitcaptured at patch-generation time.Create PR flow: apply from recorded generation base
create_pull_request.cjsnow preferspullRequestItem.base_commitwhen creating the working branch for patch transport.Push-to-PR-branch flow: re-anchor before
git ampush_to_pull_request_branch.cjsnow resets tomessage.base_commitbefore patch apply (patch transport only).Targeted regression coverage
base_commitwhen present, preventing apply-time drift from generation-time base.