Fix push_to_pull_request_branch generating bad patch on follow-up issue_comment runs#17206
Fix push_to_pull_request_branch generating bad patch on follow-up issue_comment runs#17206
Conversation
… follow-up runs Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
🎬 THE END — Smoke Claude MISSION: ACCOMPLISHED! The hero saves the day! ✨ |
|
🧪 Smoke Project is now testing project operations... |
|
🧪 Smoke Temporary ID is now testing temporary ID functionality... |
|
📰 BREAKING: Smoke Copilot is now investigating this pull request. Sources say the story is developing... |
|
✨ The prophecy is fulfilled... Smoke Codex has completed its mystical journey. The stars align. 🌟 |
|
✅ Smoke Project completed successfully. All project operations validated. |
|
Smoke test (Codex) summary:
|
|
Smoke test results for PR #17206:
Overall: PASS (10/11 - Serena MCP tools unavailable)
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Smoke test review: PR looks good. README docs are kept in sync with file removals, and the generate_git_patch.cjs refactoring is well-structured with clear strategy separation.
📰 BREAKING: Report filed by Smoke Copilot for issue #17206
|
📰 VERDICT: Smoke Copilot has concluded. All systems operational. This is a developing story. 🎤 |
|
Smoke Test Run §22230889913 — Overall: PARTIAL Core tests #1–10: ✅✅✅✅✅✅✅✅✅✅ PR review tests #11–17:
|
There was a problem hiding this comment.
💥 Automated smoke test review - all systems nominal!
💥 [THE END] — Illustrated by Smoke Claude for issue #17206
|
@copilot add a shim.cjs for the core logging function and load this shim in the safe output server and safe input server. update server.sh to copy shim.cjs in both folders |
|
🤖 Smoke test 22233329073 results for
Overall:
|
There was a problem hiding this comment.
Reviewed PR #17206 — the fix properly handles the missing origin/(branch) ref during push_to_pull_request_branch patch generation. The resolveBaseRef fallback chain is solid and the migration from shell to JS is clean.
📰 BREAKING: Report filed by Smoke Copilot for issue #17206
| const { getErrorMessage } = require("./error_helpers.cjs"); | ||
| const { execGitSync } = require("./git_helpers.cjs"); | ||
|
|
||
| const PATCH_PATH = "/tmp/gh-aw/aw.patch"; |
There was a problem hiding this comment.
Good to extract PATCH_PATH as a named constant — makes it easy to change the path in one place if needed.
| const baseRef = `origin/${branchName}`; | ||
| core.info(`[generate_git_patch] using remote tracking ref as baseRef="${baseRef}"`); | ||
| return baseRef; | ||
| } catch { |
There was a problem hiding this comment.
The three-level fallback strategy in resolveBaseRef (remote tracking ref → fresh fetch → merge-base) is well-structured and covers the gh pr checkout edge case nicely.
|
📰 VERDICT: Smoke Copilot has concluded. All systems operational. This is a developing story. 🎤 Smoke test complete: 10/11 tests passed. Serena MCP unavailable. Labels not added (not all tests passed). |
There was a problem hiding this comment.
Pull request overview
Fixes incorrect patch generation on follow-up issue_comment runs for push_to_pull_request_branch by ensuring the remote base ref is fetched (instead of immediately falling back to merge-base), and adds a Node shim so scripts relying on GitHub Actions’ core global can run in MCP server contexts.
Changes:
- Refactors
generate_git_patch.cjsinto helpers and fetchesorigin/<branch>when the tracking ref is missing to avoid including already-applied commits. - Introduces
shim.cjsand loads it early in MCP server entrypoints; updates tests to mockglobal.core. - Removes the legacy shell patch generator and a Go test that depended on it; updates setup docs and file-copy lists.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
actions/setup/js/generate_git_patch.cjs |
Fetches missing remote branch tracking ref before computing patch base; refactors patch generation into helpers with added logging. |
actions/setup/js/generate_git_patch.test.cjs |
Adds global.core mock and clears mocks between tests. |
actions/setup/js/shim.cjs |
Adds a minimal global.core shim for non-GitHub Actions Node execution. |
actions/setup/js/safe-outputs-mcp-server.cjs |
Requires the shim first so global.core exists before other imports. |
actions/setup/js/safe_inputs_mcp_server_http.cjs |
Requires the shim first so global.core exists before other imports. |
actions/setup/js/safe_outputs_handlers.test.cjs |
Adds global.core mock and clears mocks between tests. |
actions/setup/setup.sh |
Ensures shim.cjs is copied into both safe-inputs and safe-outputs deploy directories. |
actions/setup/README.md |
Updates shell script count/list after removing generate_git_patch.sh. |
actions/setup/sh/generate_git_patch.sh |
Deleted (superseded by JS implementation). |
pkg/workflow/git_patch_head_test.go |
Deleted (was testing the removed shell script path). |
.changeset/patch-fetch-push-patch-base.md |
Changeset entry for the patch-base fetch behavior change. |
.changeset/smoke-test-push-22232216256.md |
Adds a smoke-test artifact file (currently malformed content). |
Comments suppressed due to low confidence (1)
actions/setup/js/generate_git_patch.cjs:40
- The new “missing tracking ref → fetch origin/ → fallback” logic is the core fix for follow-up
issue_commentruns, but there’s no test exercising this path (e.g., simulating a repo whererefs/remotes/origin/<branch>is absent aftergh pr checkout). Adding a focused integration-style unit test aroundresolveBaseRef/tryBranchStrategywould help prevent regressions.
function resolveBaseRef(branchName, defaultBranch, cwd) {
try {
execGitSync(["show-ref", "--verify", "--quiet", `refs/remotes/origin/${branchName}`], { cwd });
const baseRef = `origin/${branchName}`;
core.info(`[generate_git_patch] using remote tracking ref as baseRef="${baseRef}"`);
return baseRef;
} catch {
// Remote tracking ref not found (e.g. after gh pr checkout which doesn't set tracking refs).
// Try fetching the branch from origin so we use only the NEW commits as the patch base.
core.info(`[generate_git_patch] refs/remotes/origin/${branchName} not found; fetching from origin`);
}
try {
execGitSync(["fetch", "origin", branchName], { cwd });
const baseRef = `origin/${branchName}`;
core.info(`[generate_git_patch] fetch succeeded, baseRef="${baseRef}"`);
return baseRef;
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Fixed in 5ff8086. The TypeScript typecheck was failing because |
|
Smoke Test Results — Run §22233329042 Overall: PARTIAL (17/17 run, 1 skipped) Core: ✅ ✅ ✅ ✅ ✅ ✅ ✅ ✅ ✅ ✅ (#1–10 all pass)
|
There was a problem hiding this comment.
💥 Automated smoke test review - all systems nominal!
💥 [THE END] — Illustrated by Smoke Claude for issue #17206
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
✨ The prophecy is fulfilled... Smoke Codex has completed its mystical journey. The stars align. 🌟 |
|
📰 BREAKING: Smoke Copilot is now investigating this pull request. Sources say the story is developing... |
|
🎬 THE END — Smoke Claude MISSION: ACCOMPLISHED! The hero saves the day! ✨ |
|
Smoke test §22246618529 results for
Overall: PASS (Serena MCP not available in this env)
|
There was a problem hiding this comment.
Quick review of PR #17206. The core fix — fetching origin/(branch) before generating the patch — is solid and well-structured with a clear fallback chain. Two minor observations: (1) duplicate changeset files, and (2) hardcoded PATCH_PATH. Overall the logic is correct and well-commented.
📰 BREAKING: Report filed by Smoke Copilot
|
📰 VERDICT: Smoke Copilot has concluded. All systems operational. This is a developing story. 🎤 |
|
PR titles:
|
generate_git_patch.cjs: addresolveBaseRef,writePatch,tryBranchStrategy,tryHeadStrategyhelpers; fetchorigin/before falling back to merge-base; addcore.info/core.warninglogginggenerate_git_patch.test.cjs: addglobal.coremock before tests andvi.clearAllMocks()in beforeEachsafe_outputs_handlers.test.cjs: addglobal.coremock andvi.clearAllMocks()actions/setup/sh/generate_git_patch.shpkg/workflow/git_patch_head_test.goactions/setup/README.md: change shell scripts count from 7 to 6, remove generate_git_patch.sh entryactions/setup/js/shim.cjs— minimalglobal.coreshim (no-op in GitHub Actions, console-backed in standalone Node.js)safe-outputs-mcp-server.cjssafe_inputs_mcp_server_http.cjsshim.cjstoSAFE_INPUTS_FILESandSAFE_OUTPUTS_FILESinsetup.shshim.cjs: add@ts-expect-erroronglobal.corereferencesOriginal prompt
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.
Changeset
origin/(branch)ref before generating the push_to_pull_request_branch patch so only new commits land on follow-up issue_comment runs.✨ PR Review Safe Output Test - Run 22230889913
Changeset
origin/(branch)ref before generating the push_to_pull_request_branch patch so follow-upissue_commentruns only include new commits.✨ PR Review Safe Output Test - Run 22232216256
Changeset
origin/(branch)ref before generating thepush_to_pull_request_branchpatch so follow-upissue_commentruns only include new commits.✨ PR Review Safe Output Test - Run 22233329042