[WIP] Fix aw.patch generation logic to handle local commits#4409
[WIP] Fix aw.patch generation logic to handle local commits#4409
Conversation
- Modified generate_git_patch.sh to detect commits made to current HEAD - Added Strategy 2: Falls back to GITHUB_SHA..HEAD when no branch name found - Enhanced logging throughout patch generation process - Shows environment information, git status, and commit details - Added comprehensive tests for HEAD-based patch generation - All existing tests continue to pass Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
@copilot continue working |
The implementation is complete and ready for review: ✅ All tests pass (including 3 new comprehensive tests) The fix handles the case where LLMs make commits directly to HEAD without creating a named branch. Commits since Commit: fc78876 |
|
✅ Agentic Changeset Generator completed successfully. |
There was a problem hiding this comment.
Pull Request Overview
This PR fixes a bug in patch generation where commits made directly to the checked-out HEAD (without creating a named branch) were not captured. The solution implements a two-strategy approach with fallback logic and comprehensive diagnostic logging.
Key changes:
- Added Strategy 2 (HEAD-based patch generation) as a fallback when no branch name is found in JSONL output
- Enhanced diagnostic logging throughout the patch generation process (environment info, git status, strategy selection, etc.)
- Added comprehensive test coverage for the new HEAD-based patch generation scenario
Reviewed Changes
Copilot reviewed 20 out of 20 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
pkg/workflow/sh/generate_git_patch.sh |
Core fix: Added Strategy 2 for HEAD-based patch generation with fallback logic, enhanced logging, and validation of git history (merge-base ancestor check) |
pkg/workflow/git_patch_head_test.go |
New test file with 3 comprehensive tests covering HEAD-based patches, strategy priority, and no-commits scenarios |
.changeset/patch-fix-patch-generation-local-commits.md |
Changeset documenting the patch-level fix |
.github/workflows/*.lock.yml (17 files) |
Recompiled workflow lock files incorporating the updated patch generation script |
Comments suppressed due to low confidence (1)
pkg/workflow/sh/generate_git_patch.sh:217
- Potential logic inconsistency: The script checks if
/tmp/gh-aw/aw.patchexists (line 184) to display patch info, but this could show information even whenPATCH_GENERATED=false. This can happen if:
- A previous patch file exists from a prior run
- The
git format-patchcommand fails and writes an error message to the file
The patch info display should respect the PATCH_GENERATED flag to avoid showing misleading information. Consider:
if [ "$PATCH_GENERATED" = true ] && [ -f /tmp/gh-aw/aw.patch ]; thenThis ensures patch info is only displayed when a patch was actually generated in this run.
if [ -f /tmp/gh-aw/aw.patch ]; then
echo ""
echo "=== Diagnostic: Patch file information ==="
ls -lh /tmp/gh-aw/aw.patch
# Get patch file size in KB
PATCH_SIZE="$(du -k /tmp/gh-aw/aw.patch | cut -f1)"
echo "Patch file size: ${PATCH_SIZE} KB"
# Count lines in patch
PATCH_LINES="$(wc -l < /tmp/gh-aw/aw.patch)"
echo "Patch file lines: $PATCH_LINES"
# Extract and count commits from patch file (each commit starts with "From <sha>")
PATCH_COMMITS="$(grep -c "^From [0-9a-f]\{40\}" /tmp/gh-aw/aw.patch 2>/dev/null || echo "0")"
echo "Commits included in patch: $PATCH_COMMITS"
# List commit SHAs in the patch
if [ "$PATCH_COMMITS" -gt 0 ]; then
echo "Commit SHAs in patch:"
grep "^From [0-9a-f]\{40\}" /tmp/gh-aw/aw.patch | sed 's/^From \([0-9a-f]\{40\}\).*/ \1/' || echo "Failed to extract commit SHAs"
fi
# Show the first 50 lines of the patch for review
{
echo '## Git Patch'
echo ''
echo '```diff'
head -500 /tmp/gh-aw/aw.patch || echo "Could not display patch contents"
echo '...'
echo '```'
echo ''
} >> "$GITHUB_STEP_SUMMARY"
fi
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| cmd.Run() | ||
|
|
||
| cmd = exec.Command("git", "commit", "-m", "Initial") | ||
| cmd.Dir = tmpDir | ||
| cmd.Run() | ||
|
|
||
| // Get SHA | ||
| cmd = exec.Command("git", "rev-parse", "HEAD") | ||
| cmd.Dir = tmpDir | ||
| output, _ := cmd.CombinedOutput() |
There was a problem hiding this comment.
Error handling is inconsistent in TestGitPatchNoCommits. The test ignores errors from git operations (lines 339, 343, 351, 355, 360), which could lead to false positives if git commands fail. Unlike the other two tests that properly check errors with t.Fatalf, this test silently continues even if setup operations fail.
Consider adding proper error checks:
if err := cmd.Run(); err != nil {
t.Fatalf("Failed to configure git: %v", err)
}| cmd.Run() | |
| cmd = exec.Command("git", "commit", "-m", "Initial") | |
| cmd.Dir = tmpDir | |
| cmd.Run() | |
| // Get SHA | |
| cmd = exec.Command("git", "rev-parse", "HEAD") | |
| cmd.Dir = tmpDir | |
| output, _ := cmd.CombinedOutput() | |
| if err := cmd.Run(); err != nil { | |
| t.Fatalf("Failed to add file to git: %v", err) | |
| } | |
| cmd = exec.Command("git", "commit", "-m", "Initial") | |
| cmd.Dir = tmpDir | |
| if err := cmd.Run(); err != nil { | |
| t.Fatalf("Failed to commit file to git: %v", err) | |
| } | |
| // Get SHA | |
| cmd = exec.Command("git", "rev-parse", "HEAD") | |
| cmd.Dir = tmpDir | |
| output, err := cmd.CombinedOutput() | |
| if err != nil { | |
| t.Fatalf("Failed to get git SHA: %v, output: %s", err, output) | |
| } |
|
|
||
| // Create empty safe-outputs | ||
| safeOutputsFile := filepath.Join(tmpDir, "safe-outputs.jsonl") | ||
| os.WriteFile(safeOutputsFile, []byte(""), 0644) |
There was a problem hiding this comment.
Missing error check for os.WriteFile operations. If these operations fail (e.g., due to permissions or disk space issues), the test will continue with an invalid test setup, potentially leading to confusing test failures or false positives.
Add error checks:
if err := os.WriteFile(testFile, []byte("content\n"), 0644); err != nil {
t.Fatalf("Failed to write test file: %v", err)
}| os.WriteFile(safeOutputsFile, []byte(""), 0644) | |
| if err := os.WriteFile(safeOutputsFile, []byte(""), 0644); err != nil { | |
| t.Fatalf("Failed to write safe-outputs.jsonl: %v", err) | |
| } |
| git format-patch "${GITHUB_SHA}..HEAD" --stdout > /tmp/gh-aw/aw.patch || echo "Failed to generate patch from HEAD" > /tmp/gh-aw/aw.patch | ||
| echo "Patch file created from commits on HEAD (base: $GITHUB_SHA)" | ||
| PATCH_GENERATED=true |
There was a problem hiding this comment.
The error message on line 160 may be misleading when the git format-patch command succeeds but writes the error message due to the || operator. The command git format-patch ... || echo "Failed to generate patch from HEAD" > ... will overwrite the patch file with the error message if format-patch returns a non-zero exit code, but the subsequent logging on line 161 will still report "Patch file created from commits on HEAD".
Consider checking the exit status before writing success messages:
if git format-patch "${GITHUB_SHA}..HEAD" --stdout > /tmp/gh-aw/aw.patch; then
echo "Patch file created from commits on HEAD (base: $GITHUB_SHA)"
PATCH_GENERATED=true
else
echo "Failed to generate patch from HEAD"
fi| git format-patch "${GITHUB_SHA}..HEAD" --stdout > /tmp/gh-aw/aw.patch || echo "Failed to generate patch from HEAD" > /tmp/gh-aw/aw.patch | |
| echo "Patch file created from commits on HEAD (base: $GITHUB_SHA)" | |
| PATCH_GENERATED=true | |
| if git format-patch "${GITHUB_SHA}..HEAD" --stdout > /tmp/gh-aw/aw.patch; then | |
| echo "Patch file created from commits on HEAD (base: $GITHUB_SHA)" | |
| PATCH_GENERATED=true | |
| else | |
| echo "Failed to generate patch from HEAD" | |
| fi |
| git format-patch "$BASE_REF".."$BRANCH_NAME" --stdout > /tmp/gh-aw/aw.patch || echo "Failed to generate patch from branch" > /tmp/gh-aw/aw.patch | ||
| echo "Patch file created from branch: $BRANCH_NAME (base: $BASE_REF)" | ||
| PATCH_GENERATED=true |
There was a problem hiding this comment.
Same issue as Strategy 2: The error handling pattern git format-patch ... || echo "Failed to generate patch from branch" > ... will overwrite a successful patch with an error message if format-patch returns non-zero, but line 109 will still log "Patch file created" and set PATCH_GENERATED=true regardless of whether the patch generation actually succeeded.
This is consistent with the existing behavior before this PR, but the same fix should be applied here for consistency:
if git format-patch "$BASE_REF".."$BRANCH_NAME" --stdout > /tmp/gh-aw/aw.patch; then
echo "Patch file created from branch: $BRANCH_NAME (base: $BASE_REF)"
PATCH_GENERATED=true
else
echo "Failed to generate patch from branch"
fi| git format-patch "$BASE_REF".."$BRANCH_NAME" --stdout > /tmp/gh-aw/aw.patch || echo "Failed to generate patch from branch" > /tmp/gh-aw/aw.patch | |
| echo "Patch file created from branch: $BRANCH_NAME (base: $BASE_REF)" | |
| PATCH_GENERATED=true | |
| if git format-patch "$BASE_REF".."$BRANCH_NAME" --stdout > /tmp/gh-aw/aw.patch; then | |
| echo "Patch file created from branch: $BRANCH_NAME (base: $BASE_REF)" | |
| PATCH_GENERATED=true | |
| else | |
| echo "Failed to generate patch from branch" | |
| fi |
Fix aw.patch generation for locally committed changes ✅
Problem Statement
The current patch generation logic only looks for explicitly named branches in the JSONL output. When an LLM makes commits directly to the currently checked out branch during action execution, these commits are not captured in the patch file, resulting in an empty patch.
Issue: https://github.com/githubnext/gh-aw/actions/runs/19544824714
Implementation ✅
generate_git_patch.shonly generates patches when it finds a branch name in JSONL outputgenerate_git_patch.shto detect commits made to current HEAD since checkout SHASolution Details
Two-Strategy Approach
Strategy 1 (Existing): Branch-based patch generation
create_pull_requestorpush_to_pull_request_branchStrategy 2 (NEW): HEAD-based patch generation
GITHUB_SHAis an ancestor of currentHEADGITHUB_SHA..HEADwhen commits are detectedExtensive Logging (as requested)
The script now logs:
Environment Information
GITHUB_SHA(checkout point)DEFAULT_BRANCHHEADSHAGit Status
JSONL Processing
Strategy Selection
Patch Generation Details
git format-patchcommand being executedPatch File Information
Tests Added ✅
TestGitPatchFromHEADCommits(200+ lines)GITHUB_SHA..HEADTestGitPatchPrefersBranchOverHEAD(120+ lines)TestGitPatchNoCommits(80+ lines)HEAD == GITHUB_SHA(no new commits)Test Results ✅
All existing tests also continue to pass.
Files Changed
Modified:
pkg/workflow/sh/generate_git_patch.sh(+97 lines)Added:
pkg/workflow/git_patch_head_test.go(397 lines)Updated: All
.github/workflows/*.lock.ymlfiles (17 files)Impact
This fix ensures that:
The solution directly addresses the problem statement and includes the requested extensive logging.
Original prompt
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.