Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .changeset/patch-create-pr-max-files-config.md

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

9 changes: 9 additions & 0 deletions .changeset/patch-fix-stale-origin-branch-merge-base.md

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion .changeset/patch-push-pr-incremental-size-check.md

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion .github/aw/github-agentic-workflows.md

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion actions/setup/js/create_pull_request.test.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -384,7 +384,7 @@ describe("create_pull_request - max limit enforcement", () => {
expect(() => enforcePullRequestLimits(patchContent)).not.toThrow();
});

it("should count unique files across multi-commit patches (regression: github/gh-aw issue tsessebe autoloop)", () => {
it("should count unique files across multi-commit patches (regression: long-running branch with multi-commit patches)", () => {
// Simulate `git format-patch` output where the same files are modified across
// multiple commits. Previously the limit check counted every `diff --git`
// header (3 commits * 2 files = 6) instead of the 2 unique files actually
Expand Down
82 changes: 41 additions & 41 deletions actions/setup/js/generate_git_patch.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -164,53 +164,53 @@ async function generateGitPatch(branchName, baseBranch, options = {}) {
// FULL MODE (for create_pull_request):
// Include all commits since merge-base with default branch.
// This is appropriate for creating new PRs where we want all changes.

debugLog(`Strategy 1 (full): Checking if origin/${branchName} exists`);
//
// IMPORTANT: We deliberately do NOT short-circuit to `origin/${branchName}` even
// when that remote-tracking ref exists locally. That ref is fetched at workflow
// startup and represents the *remote* branch state at that moment, not the
// branch state before the agent made changes. If the local branch was
// fast-forwarded to the default branch during the agent run (a common pattern),
// using the stale `origin/${branchName}` would cause the patch to include all
// commits from the default branch since the old branch tip — commits the agent
// never made. Always compute the merge-base with the default branch so the patch
// contains exactly the agent's changes.
debugLog(`Strategy 1 (full): Computing merge-base with ${defaultBranch} (ignoring any stale origin/${branchName})`);
// Check if origin/<defaultBranch> already exists locally (e.g., from checkout with fetch-depth: 0)
// This is important for cross-repo checkouts where persist-credentials: false prevents fetching
let hasLocalDefaultBranch = false;
try {
// Check if origin/branchName exists
execGitSync(["show-ref", "--verify", "--quiet", `refs/remotes/origin/${branchName}`], { cwd });
baseRef = `origin/${branchName}`;
debugLog(`Strategy 1 (full): Using existing origin/${branchName} as baseRef`);
execGitSync(["show-ref", "--verify", "--quiet", `refs/remotes/origin/${defaultBranch}`], { cwd });
hasLocalDefaultBranch = true;
debugLog(`Strategy 1 (full): origin/${defaultBranch} exists locally`);
} catch {
// origin/branchName doesn't exist - use merge-base with default branch
debugLog(`Strategy 1 (full): origin/${branchName} not found, trying merge-base with ${defaultBranch}`);
// First check if origin/<defaultBranch> already exists locally (e.g., from checkout with fetch-depth: 0)
// This is important for cross-repo checkouts where persist-credentials: false prevents fetching
let hasLocalDefaultBranch = false;
// origin/<defaultBranch> doesn't exist locally, try to fetch it
debugLog(`Strategy 1 (full): origin/${defaultBranch} not found locally, attempting fetch`);
try {
execGitSync(["show-ref", "--verify", "--quiet", `refs/remotes/origin/${defaultBranch}`], { cwd });
// Configure git authentication via GIT_CONFIG_* environment variables.
// This ensures the fetch works when .git/config credentials are unavailable
// (e.g. after clean_git_credentials.sh) and on GitHub Enterprise Server (GHES).
// Use options.token when provided (cross-repo PAT), falling back to GITHUB_TOKEN.
// SECURITY: The auth header is passed via env vars so it is never written to
// .git/config on disk, preventing file-monitoring attacks.
const fullFetchEnv = { ...process.env, ...getGitAuthEnv(options.token) };
// Use "--" to prevent branch names starting with "-" from being interpreted as options
execGitSync(["fetch", "origin", "--", defaultBranch], { cwd, env: fullFetchEnv });
hasLocalDefaultBranch = true;
debugLog(`Strategy 1 (full): origin/${defaultBranch} exists locally`);
} catch {
// origin/<defaultBranch> doesn't exist locally, try to fetch it
debugLog(`Strategy 1 (full): origin/${defaultBranch} not found locally, attempting fetch`);
try {
// Configure git authentication via GIT_CONFIG_* environment variables.
// This ensures the fetch works when .git/config credentials are unavailable
// (e.g. after clean_git_credentials.sh) and on GitHub Enterprise Server (GHES).
// Use options.token when provided (cross-repo PAT), falling back to GITHUB_TOKEN.
// SECURITY: The auth header is passed via env vars so it is never written to
// .git/config on disk, preventing file-monitoring attacks.
const fullFetchEnv = { ...process.env, ...getGitAuthEnv(options.token) };
// Use "--" to prevent branch names starting with "-" from being interpreted as options
execGitSync(["fetch", "origin", "--", defaultBranch], { cwd, env: fullFetchEnv });
hasLocalDefaultBranch = true;
debugLog(`Strategy 1 (full): Successfully fetched origin/${defaultBranch}`);
} catch (fetchErr) {
// Fetch failed (likely due to persist-credentials: false in cross-repo checkout)
// We'll try other strategies below
debugLog(`Strategy 1 (full): Fetch failed - ${getErrorMessage(fetchErr)} (will try other strategies)`);
}
debugLog(`Strategy 1 (full): Successfully fetched origin/${defaultBranch}`);
} catch (fetchErr) {
// Fetch failed (likely due to persist-credentials: false in cross-repo checkout)
// We'll try other strategies below
debugLog(`Strategy 1 (full): Fetch failed - ${getErrorMessage(fetchErr)} (will try other strategies)`);
}
}

if (hasLocalDefaultBranch) {
baseRef = execGitSync(["merge-base", "--", `origin/${defaultBranch}`, branchName], { cwd }).trim();
debugLog(`Strategy 1 (full): Computed merge-base: ${baseRef}`);
} else {
// No remote refs available - fall through to Strategy 2
debugLog(`Strategy 1 (full): No remote refs available, falling through to Strategy 2`);
throw new Error(`${ERR_SYSTEM}: No remote refs available for merge-base calculation`);
}
if (hasLocalDefaultBranch) {
baseRef = execGitSync(["merge-base", "--", `origin/${defaultBranch}`, branchName], { cwd }).trim();
debugLog(`Strategy 1 (full): Computed merge-base: ${baseRef}`);
} else {
// No remote refs available - fall through to Strategy 2
debugLog(`Strategy 1 (full): No remote refs available, falling through to Strategy 2`);
throw new Error(`${ERR_SYSTEM}: No remote refs available for merge-base calculation`);
}
}

Expand Down
112 changes: 112 additions & 0 deletions actions/setup/js/generate_git_patch.test.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -485,3 +485,115 @@ describe("generateGitPatch – excludedFiles option", () => {
expect(result.success).toBe(false);
});
});

// ──────────────────────────────────────────────────────────────────────────
// Full mode base ref selection — must use merge-base, never stale origin/<branch>
// Regression test for: create_pull_request patch uses stale origin/branchName
// instead of merge-base with default branch.
// ──────────────────────────────────────────────────────────────────────────

describe("generateGitPatch – full mode base ref (merge-base, not stale origin)", () => {
let repoDir;
let originalEnv;

beforeEach(() => {
originalEnv = { GITHUB_WORKSPACE: process.env.GITHUB_WORKSPACE, GITHUB_SHA: process.env.GITHUB_SHA };

global.core = { debug: () => {}, info: () => {}, warning: () => {}, error: () => {} };

repoDir = fs.mkdtempSync(path.join(os.tmpdir(), "gh-aw-patch-fullmode-"));
execSync("git init -b main", { cwd: repoDir });
execSync('git config user.email "test@example.com"', { cwd: repoDir });
execSync('git config user.name "Test"', { cwd: repoDir });

// Initial commit on main
fs.writeFileSync(path.join(repoDir, "README.md"), "# Repo\n");
execSync("git add .", { cwd: repoDir });
execSync('git commit -m "init"', { cwd: repoDir });

delete process.env.GITHUB_WORKSPACE;
delete process.env.GITHUB_SHA;
delete require.cache[require.resolve("./generate_git_patch.cjs")];
});

afterEach(() => {
Object.entries(originalEnv).forEach(([k, v]) => {
if (v !== undefined) process.env[k] = v;
else delete process.env[k];
});
if (repoDir && fs.existsSync(repoDir)) {
fs.rmSync(repoDir, { recursive: true, force: true });
}
delete require.cache[require.resolve("./generate_git_patch.cjs")];
delete global.core;
});

it("should NOT include phantom commits when stale origin/<branch> exists (regression test)", async () => {
// Reproduce the stale-remote-tracking-ref bug scenario:
// 1. A feature branch was pushed in the past at some old commit
// (origin/feature-branch points there — this is the "stale" remote-tracking ref)
// 2. main has since advanced with many "phantom" commits the agent never made
// 3. The agent fast-forwards the local branch to main, then makes one new commit
// 4. Full-mode patch (create_pull_request) must contain ONLY the agent's commit,
// NOT the phantom commits between the old branch tip and main.

// Set up a "remote" repo to act as origin
const remoteDir = fs.mkdtempSync(path.join(os.tmpdir(), "gh-aw-patch-fullmode-remote-"));
try {
execSync("git init --bare -b main", { cwd: remoteDir });
execSync(`git remote add origin ${remoteDir}`, { cwd: repoDir });
execSync("git push origin main", { cwd: repoDir });

// Step 1: Create the feature branch at the initial commit and push it.
// This becomes the "old" position of origin/feature-branch.
execSync("git checkout -b feature-branch", { cwd: repoDir });
execSync("git push origin feature-branch", { cwd: repoDir });
// Explicitly fetch to ensure refs/remotes/origin/feature-branch exists in the
// local repo. `git push` typically updates this automatically, but in repos
// created via `git init` + `git remote add` (no clone) the remote-tracking
// ref population can be inconsistent across git versions, so be explicit.
execSync("git fetch origin feature-branch:refs/remotes/origin/feature-branch", { cwd: repoDir });
const oldBranchSha = execSync("git rev-parse HEAD", { cwd: repoDir }).toString().trim();

// Step 2: main advances with phantom commits the agent will not make
execSync("git checkout main", { cwd: repoDir });
fs.writeFileSync(path.join(repoDir, "phantom1.md"), "# phantom 1\n");
execSync("git add phantom1.md", { cwd: repoDir });
execSync('git commit -m "phantom commit 1"', { cwd: repoDir });
fs.writeFileSync(path.join(repoDir, "phantom2.md"), "# phantom 2\n");
execSync("git add phantom2.md", { cwd: repoDir });
execSync('git commit -m "phantom commit 2"', { cwd: repoDir });
execSync("git push origin main", { cwd: repoDir });

// Step 3: Simulate the agent run — fast-forward the local feature branch to
// main, then make one new commit. Note that origin/feature-branch is still
// pointing at oldBranchSha (we deliberately do NOT push the branch update).
execSync("git checkout feature-branch", { cwd: repoDir });
execSync("git reset --hard main", { cwd: repoDir });
fs.writeFileSync(path.join(repoDir, "agent-change.txt"), "the only real change\n");
execSync("git add agent-change.txt", { cwd: repoDir });
execSync('git commit -m "agent change"', { cwd: repoDir });

// Sanity check: origin/feature-branch is still stale (points to oldBranchSha)
const remoteBranchSha = execSync("git rev-parse origin/feature-branch", { cwd: repoDir }).toString().trim();
expect(remoteBranchSha).toBe(oldBranchSha);

Comment on lines +543 to +580
// Step 4: Generate the full-mode patch
const { generateGitPatch } = require("./generate_git_patch.cjs");
const result = await generateGitPatch("feature-branch", "main", { cwd: repoDir, mode: "full" });

expect(result.success).toBe(true);
const patch = fs.readFileSync(result.patchPath, "utf8");

// The patch MUST contain the agent's commit
expect(patch).toContain("agent-change.txt");
// The patch MUST NOT contain the phantom commits from main
expect(patch).not.toContain("phantom1.md");
expect(patch).not.toContain("phantom2.md");
} finally {
if (fs.existsSync(remoteDir)) {
fs.rmSync(remoteDir, { recursive: true, force: true });
}
}
});
});
2 changes: 1 addition & 1 deletion actions/setup/js/safe_outputs_handlers.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -643,7 +643,7 @@ function createHandlers(server, appendSafeOutput, config = {}) {
// validate `max_patch_size` against the actual incremental change relative
// to the existing PR branch head, not the (potentially much larger) size of
// the format-patch transport file. This is critical for the long-running
// branch pattern (e.g. autoloop) where the format-patch can include many
// branch pattern where the format-patch can include many
// commits but each iteration only changes a few KB.
if (typeof patchResult.diffSize === "number" && patchResult.diffSize >= 0) {
entry.diff_size = patchResult.diffSize;
Expand Down
2 changes: 1 addition & 1 deletion pkg/workflow/compiler_safe_outputs_config_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1420,7 +1420,7 @@ func TestHandlerConfigPatchSize(t *testing.T) {

// TestHandlerConfigPatchFiles tests that the max-patch-files configuration is
// propagated into the create_pull_request handler config (regression for the
// hardcoded 100-file limit reported in the tsessebe autoloop scenario).
// hardcoded 100-file limit for long-running branches with multi-commit patches).
func TestHandlerConfigPatchFiles(t *testing.T) {
tests := []struct {
name string
Expand Down
Loading