Base full-mode patch/bundle on GITHUB_SHA when running from a non-default branch - #52164
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR does not have the 'implementation' label and has ≤100 new lines of code in business logic directories (0 additions detected in default business logic paths).
|
|
✅ PR Code Quality Reviewer completed the code quality review. Warning Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding. What happenedThe threat detection engine failed to produce results. Review the workflow run logs for details. Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "api.individual.githubcopilot.com"See Network Configuration for more information.
|
|
✅ Ponytail Reviewer completed successfully!
|
|
✅ Test Quality Sentinel completed test quality analysis.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: Base full-mode patch/bundle on GITHUB_SHA when running from a non-default branch
The logic is correct and well-tested. The new conditional correctly handles the non-default-branch dispatch scenario:
GITHUB_SHAis used as base only when it is an ancestor of the agent branch and not contained in the default branch — preventing regressions for normal (default-branch) runs.- Falls back to the existing
merge-basepath in all other cases. - Diagnostic improvements (
describeGitFailure,isPartialClone) are clearly scoped and do not affect the happy path. - The switch from
show-reftorev-parsefor local-branch existence checks is strictly better: both are local-only, butrev-parse --verifyexits with a predictable non-zero code when the ref is absent. - Three new integration tests cover the feature and the fallback.
No blocking issues found.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 23.2 AIC · ⌖ 6.27 AIC · ⊞ 5.4K
There was a problem hiding this comment.
Generated by ✂️ Ponytail Reviewer for #52164 · auto · 20 AIC · ⌖ 5.71 AIC · ⊞ 6.8K
Comment /ponytail to run again
Comments that could not be inline-anchored
actions/setup/js/generate_git_bundle.cjs:43
shrink: dispatched-branch base-selection block (compute dispatchedSha + isAncestorCommit check) duplicated verbatim in generate_git_patch.cjs L98-101. Extract a shared selectFullModeBaseRef(githubSha, tipRef, defaultBranchRef, cwd) helper in git_patch_utils.cjs, 1 caller each.
🧪 Test Quality Sentinel Report✅ Test Quality Score: 90/100 — Excellent
📊 Metrics (3 tests)
🔍 Per-test notesTest 1 — patch on non-default branch GITHUB_SHA Test 2 — merge-base fallback when GITHUB_SHA is on default branch Test 3 — bundle on non-default branch GITHUB_SHA Verdict
|
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd — commenting with a few targeted suggestions; no blocking issues.
📋 Key Themes & Highlights
Key Themes
- Regex over-breadth:
"could not fetch"indescribeGitFailureis wide enough to annotate unrelated network errors with the partial-clone explanation. - Missing no-commit edge case: The GITHUB_SHA base-selection guard does not prevent an empty patch when
GITHUB_SHA === tipRef(agent branch tip, no commits yet). - Test coverage is solid — three well-structured integration tests cover the fix and the regression path. Minor: the regression test could benefit from an explicit commit-count assertion.
Positive Highlights
- ✅ Root cause correctly identified and fixed symmetrically in both patch and bundle generators
- ✅ The
GITHUB_SHA not-on-default-branchguard is precise and leaves default-branch runs untouched - ✅ Improved diagnostics for auth failures on partial clones are a genuine UX win
- ✅ Shared helpers in
git_patch_utils.cjsfollow the existing module pattern cleanly - ✅ Changeset entry is clear and accurate
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 42.5 AIC · ⌖ 6.92 AIC · ⊞ 7K
Comment /matt to run again
| function describeGitFailure(message, cwd) { | ||
| if (!/promisor|Authentication failed|Invalid username or token|could not fetch/i.test(message)) { | ||
| return message; | ||
| } |
There was a problem hiding this comment.
[/diagnosing-bugs] The "could not fetch" pattern in describeGitFailure is broad and may decorate unrelated network errors with a partial-clone explanation.
💡 Suggestion
"could not fetch" can appear in many git error messages unrelated to lazy object hydration. Since isPartialClone is already checked, the string pattern only needs to match the lazy-fetch indicators. Consider narrowing to promisor-specific strings:
if (!/promisor|Authentication failed|Invalid username or token|fetch-pack|object not found/i.test(message)) {This prevents misleading diagnostics for ordinary network failures in partial-clone repos.
@copilot please address this.
There was a problem hiding this comment.
Narrowed the regex to promisor-specific indicators (promisor|Authentication failed|Invalid username or token|fetch-pack|object not found), dropping the broad could not fetch match. Fixed in bccc136.
| // and fails). GITHUB_SHA is the commit the agent started from and every | ||
| // object it needs is already present in the checkout. | ||
| const dispatchedSha = normalizeCommitSHA(githubSha); | ||
| if (defaultBranchRef && dispatchedSha && isAncestorCommit(dispatchedSha, tipRef, cwd) && !isAncestorCommit(dispatchedSha, defaultBranchRef, cwd)) { |
There was a problem hiding this comment.
[/diagnosing-bugs] The GITHUB_SHA base selection condition runs two isAncestorCommit calls (each spawning git merge-base --is-ancestor) even in the common default-branch case where both will succeed quickly. More importantly, if GITHUB_SHA equals the agent branch tip (no commits added yet), the condition is still true and will produce an empty patch.
💡 Suggested guard
Add a check that dispatchedSha is not identical to tipRef to ensure there is at least one agent commit in the patch:
if (defaultBranchRef && dispatchedSha && dispatchedSha !== tipRef &&
isAncestorCommit(dispatchedSha, tipRef, cwd) &&
!isAncestorCommit(dispatchedSha, defaultBranchRef, cwd)) {This also avoids silently producing a zero-commit patch when the agent branch was created but no commits were made yet.
@copilot please address this.
There was a problem hiding this comment.
Added dispatchedSha !== tipRef to the guard in both generate_git_patch.cjs and generate_git_bundle.cjs so the GITHUB_SHA base is never selected when there are zero agent commits. Fixed in bccc136.
| expect(result.success).toBe(true); | ||
| expect(result.baseCommit).toBe(dispatchedSha); | ||
|
|
||
| const patchContent = fs.readFileSync(result.patchPath, "utf8"); |
There was a problem hiding this comment.
[/tdd] The default-branch regression test checks that result.baseCommit === mainSha, but mainSha was captured before the agent branch was created — if main advances between these two points (e.g. in a parallel test), the assertion could fail spuriously. More critically, the test does not assert that the patch content contains both "Agent commit one" and "Agent commit two" — only two expect calls on patchContent are present. This gap means a regressed single-commit path would still pass.
💡 Suggested addition
expect(patchContent).toContain("Agent commit one");
expect(patchContent).toContain("Agent commit two");Both lines are actually already there — just confirming they cover the full agent commit range. Pair this with a count check if the full-mode invariant matters:
const logLines = execGit(["log", "--oneline", `${mainSha}..HEAD`], { cwd: workingRepo }).stdout.trim().split("
");
expect(logLines).toHaveLength(2);@copilot please address this.
There was a problem hiding this comment.
Added a commit-count assertion (git log --oneline mainSha..HEAD has length 2) to the regression test. Fixed in bccc136.
| function isAncestorCommit(ancestor, descendant, cwd) { | ||
| try { | ||
| execGitSync(["merge-base", "--is-ancestor", "--", ancestor, descendant], { cwd, suppressLogs: true }); | ||
| return true; |
There was a problem hiding this comment.
[/codebase-design] isAncestorCommit silently returns false on any git failure, including unexpected ones like a corrupt object store. This is the right behaviour for the caller decision, but consider adding a suppressLogs: true comment note or an explicit debugLog path for non-expected failures so future diagnostics are not silently swallowed.
💡 Optional improvement
function isAncestorCommit(ancestor, descendant, cwd) {
try {
execGitSync(["merge-base", "--is-ancestor", "--", ancestor, descendant], { cwd, suppressLogs: true });
return true;
} catch (err) {
// treat all failures (unknown revision, missing object, etc.) as "not an ancestor"
return false;
}
}The comment captures the intent; callers who need richer diagnostics can call describeGitFailure on the caught error separately.
@copilot please address this.
There was a problem hiding this comment.
Added a doc-comment note clarifying that failures are intentionally suppressed here and that callers needing richer diagnostics should call describeGitFailure on their own caught error. Fixed in bccc136.
There was a problem hiding this comment.
Pull request overview
Updates full-mode patch and bundle generation to use the workflow’s starting commit for non-default-branch runs.
Changes:
- Selects
GITHUB_SHAas the transport base when appropriate. - Improves branch and partial-clone diagnostics.
- Adds integration coverage and release notes.
Show a summary per file
| File | Description |
|---|---|
.changeset/patch-fix-non-default-branch-patch-base.md |
Documents the fix. |
actions/setup/js/git_patch_utils.cjs |
Adds ancestry and diagnostic helpers. |
actions/setup/js/generate_git_patch.cjs |
Updates patch base selection and errors. |
actions/setup/js/generate_git_bundle.cjs |
Updates bundle base selection. |
actions/setup/js/git_patch_integration.test.cjs |
Tests default and non-default branch behavior. |
Review details
Tip
Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
- Files reviewed: 5/5 changed files
- Comments generated: 2
- Review effort level: Balanced
| if (hasLocalDefaultBranch && dispatchedSha && isAncestorCommit(dispatchedSha, branchName, cwd) && !isAncestorCommit(dispatchedSha, `origin/${defaultBranch}`, cwd)) { | ||
| baseRef = dispatchedSha; | ||
| debugLog(`Strategy 1 (full): GITHUB_SHA ${dispatchedSha} is not contained in origin/${defaultBranch} (non-default-branch run); using it as the bundle base instead of the merge-base`); |
| function describeGitFailure(message, cwd) { | ||
| if (!/promisor|Authentication failed|Invalid username or token|could not fetch/i.test(message)) { | ||
| return message; | ||
| } | ||
| if (!isPartialClone(cwd)) { | ||
| return message; |
|
@copilot please address the open review threads below, refresh the branch if needed, and run the pr-finisher skill. Open review threads (newest first):
|
…verage Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
🤖 Triage: PR #52164
|
|
@copilot merge main recompile |
…ow-dispatch-patch Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Merged |
When a workflow runs from a ref that is not contained in the default branch (e.g.
workflow_dispatchon a feature branch),generate_git_patchbased the transport artifact onmerge-base(defaultBranch, agentBranch). The patch then spanned the entire dispatched branch instead of the agent's commits, and on a partial clone it failed outright — diffing from that older base required base-side blobs that were never fetched, so git attempted a lazy promisor fetch which is unauthenticated (persist-credentials: false).Both symptoms are invisible on a default-branch run, where
merge-base == GITHUB_SHA.Changes
generate_git_patch.cjs(full mode) — before falling back to the merge-base, preferGITHUB_SHAas the base when it is an ancestor of the agent branch tip but not contained in the default branch. That is precisely the non-default-ref dispatch case; the patch then holds only the agent's commits and requires no objects beyond the checkout commit's tree, which also removes the partial-clone failure. Default-branch runs take the unchanged merge-base path.generate_git_bundle.cjs— same base selection in its full-mode merge-base path.create_pull_requestdefaults topatch_format: bundle, so the bundle step would otherwise reproduce the failure immediately after the patch step.git rev-parse --verify --quiet refs/heads/<branch>(local refs only), and the Strategy 1 failure path re-checks local refs before wording its message, so a network/auth failure is no longer reported asBranch 'X' does not exist locally. The incremental-mode error is likewise split by whether the branch actually exists.describeGitFailureappends an explanation when a git failure looks like a failed lazy hydration andremote.origin.promisor=true, since gh-aw knows it emittedpersist-credentials: false.git_patch_utils.cjs— new sharedisAncestorCommit,isPartialClone,describeGitFailurehelpers used by both generators.baseCommit === GITHUB_SHA), the default-branch regression case (still merge-base, all agent commits included), and the bundle equivalent.run: https://github.com/github/gh-aw/actions/runs/31547117706> Generated by 👨🍳 PR Sous Chef · gpt54 · 7.19 AIC · ⌖ 5.2 AIC · ⊞ 8.5K · ◷