Fix push_to_pull_request_branch generating bad patch on issue_comment follow-up runs#16927
Fix push_to_pull_request_branch generating bad patch on issue_comment follow-up runs#16927
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
… commits When issue_comment triggers a workflow run, gh pr checkout is used to check out the PR branch. Unlike git fetch + checkout, gh pr checkout does NOT create a refs/remotes/origin/<branch> remote tracking ref. Without that ref, generate_git_patch fell back to merge-base with the default branch, which includes ALL commits on the PR branch (including ones already pushed). On a follow-up run this produced a [PATCH 1/2] for an already-applied commit, causing git am to fail. Fix: before falling back to merge-base, try fetching origin/<branch>. If the fetch succeeds (branch already on remote), use it as the patch base so only genuinely new commits are included. Only fall back to merge-base for truly brand-new branches. Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
@copilot merge main, recompile |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Done — merged main and recompiled all lock files in bdce136. Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
Adds logInfo/logWarning helpers (stderr + core.info/core.warning when in GitHub Actions context) and instruments every decision point: - Entry: branchName, GITHUB_SHA, defaultBranch, cwd, patchPath - Strategy 1: local branch check, tracking-ref lookup, fetch attempt, merge-base fallback, baseRef chosen, commitCount, commit list, patch write result - Strategy 2: currentHead vs GITHUB_SHA, ancestry check, commitCount, commit list, patch write result - Final: success summary or error reason Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
@copilot too much logging, just use core.info. Merge main and recompile |
- Replace logInfo/logWarning helpers + stderr writes with direct core.info and core.warning calls throughout generate_git_patch.cjs - Merge origin/main (adds macOS arm64 smoke-test fix) - Recompile all workflow lock files (154/154) Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Done in 5d412ab — replaced the custom helpers/stderr writes with direct Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
| @@ -73,14 +73,22 @@ if [ -n "$BRANCH_NAME" ]; then | |||
| echo "Using origin/${BRANCH_NAME@Q} as base for patch generation" | |||
There was a problem hiding this comment.
@copilot determine if this file is still used in the generated lock.yml file. If not remove it.
There was a problem hiding this comment.
Confirmed — generate_git_patch.sh is not referenced in any compiled .lock.yml file. Patch generation at runtime is handled entirely by generate_git_patch.cjs. Removed the script, its test file (git_patch_head_test.go), and the README entry in 174e6f7.
![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 of PR #16927. The refactoring of generate_git_patch.cjs looks solid — resolveBaseRef correctly prioritizes remote tracking refs, then fetches, then falls back to merge-base, which directly addresses the bug. The decomposition into resolveBaseRef, writePatch, tryBranchStrategy, and tryHeadStrategy improves readability. Two minor inline comments added.
📰 BREAKING: Report filed by Smoke Copilot for issue #16927
| return false; | ||
| } | ||
|
|
||
| const baseRef = resolveBaseRef(branchName, defaultBranch, cwd); |
There was a problem hiding this comment.
Nice refactor! The resolveBaseRef + writePatch separation makes tryBranchStrategy clean and easy to follow. The priority order (remote tracking → fetch → merge-base) correctly handles the issue_comment follow-up case.
| 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.
Hardcoding PATCH_PATH as a module-level constant is a good improvement over having it scattered as a string literal. Consider exporting it or making it configurable for testing purposes.
|
📰 VERDICT: Smoke Copilot has concluded. All systems operational. This is a developing story. 🎤 |
|
PR titles: Prevent MCP server version bump test failures by replacing hardcoded versions with constants; Stabilize frontmatter hash across LF/CRLF newline conventions
|
Smoke Test Results — Run §22226202391Overall: PARTIAL (1 failure, 1 skipped) 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 #16927
|
@copilot merge main and recompile |
|
🧪 Smoke Temporary ID is now testing temporary ID functionality... |
|
💫 TO BE CONTINUED... Smoke Claude failed! Our hero faces unexpected challenges... |
|
✨ The prophecy is fulfilled... Smoke Codex has completed its mystical journey. The stars align. 🌟 |
|
🧪 Smoke Project is now testing project operations... |
|
📰 BREAKING: Smoke Copilot is now investigating this pull request. Sources say the story is developing... |
|
✅ Smoke Project completed successfully. All project operations validated. |
|
Smoke test results
|
|
Smoke Test Results for
Overall: PASS (1 tool unavailable)
|
There was a problem hiding this comment.
Smoke test review pass. The generate_git_patch.cjs refactor looks solid - good fallback strategy for base ref resolution and clean patch writing logic. generate_git_patch.sh removal is clean.
📰 BREAKING: Report filed by Smoke Copilot for issue #16927
|
📰 VERDICT: Smoke Copilot has concluded. All systems operational. This is a developing story. 🎤 |
generate_git_patch.cjs: fetchorigin/<branch>before falling back to merge-basegenerate_git_patch.cjsinto smaller functions:resolveBaseRef,writePatch,tryBranchStrategy,tryHeadStrategycore.info/core.warninglogginggenerate_git_patch.test.cjs: set upglobal.coremock before testssafe_outputs_handlers.test.cjs: set upglobal.coremock before testsgenerate_git_patch.shOriginal prompt
push_to_pull_request_branchgenerates a bad patch when triggered via anissue_commentevent on a PR #16925🔒 GitHub Advanced Security automatically protects Copilot coding agent pull requests. You can protect all pull requests by enabling Advanced Security for your repositories. Learn more about Advanced Security.
Changeset
push_to_pull_request_branchfetches the target branch so generated patches only include new commits✨ PR Review Safe Output Test - Run 22210660047
✨ PR Review Safe Output Test - Run 22226202391
Changeset
push_to_pull_request_branchfetches the target branch before generating patches so follow-up commits only include new work.