Skip to content

Commit efe940e

Browse files
authored
safe-outputs: make create-pull-request bundling resilient to diverged-history replay conflicts (#40720)
1 parent 42a1743 commit efe940e

6 files changed

Lines changed: 235 additions & 6 deletions

File tree

.github/workflows/agentic-auto-upgrade.yml

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -23,10 +23,10 @@
2323
#
2424
# Alternative regeneration methods:
2525
# make recompile
26-
#
26+
#
2727
# Or use the gh-aw CLI directly:
2828
# ./gh-aw compile --validate --verbose
29-
#
29+
#
3030
# The workflow is generated when auto_upgrade is set to true in aw.json.
3131
# The weekly schedule is deterministically scattered based on the repository slug.
3232
#

.github/workflows/agentics-maintenance.yml

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -23,13 +23,13 @@
2323
#
2424
# This file defines the generated agentic maintenance workflow for this repository.
2525
# It runs scheduled cleanup for expiring safe outputs and supports manual maintenance operations.
26-
#
26+
#
2727
# This workflow is generated automatically when workflows use expiring safe outputs
2828
# or when repository maintenance features are enabled in .github/workflows/aw.json.
29-
#
29+
#
3030
# To disable maintenance workflow generation, set in .github/workflows/aw.json:
3131
# {"maintenance": false}
32-
#
32+
#
3333
# Agentic maintenance docs:
3434
# https://github.github.com/gh-aw/reference/ephemerals/#manual-maintenance-operations
3535
#

actions/setup/js/create_pull_request.cjs

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1625,13 +1625,18 @@ async function main(config = {}) {
16251625
const artifactFileName = bundleFilePath ? bundleFilePath.replace("/tmp/gh-aw/", "") : "aw-unknown.bundle";
16261626
const fallbackBundleSourceRef = `refs/heads/${originalAgentBranch || branchName}`;
16271627
const fallbackBundleTempRef = createBundleTempRef(branchName);
1628+
const pushFailureMessage = sanitizeContent(neutralizeClosingKeywordsForIssueBody(pushError instanceof Error ? pushError.message : String(pushError)), { allowedAliases: allowedMentionAliases })
1629+
.replace(/\s+/g, " ")
1630+
.trim();
16281631
const fallbackBody = `${issueSafeBody}
16291632
16301633
---
16311634
16321635
> [!NOTE]
16331636
> This was originally intended as a pull request, but the git push operation failed.
16341637
>
1638+
> **Original error:** ${pushFailureMessage}
1639+
>
16351640
> **Workflow Run:** [View run details and download bundle artifact](${runUrl})
16361641
>
16371642
> The bundle file is available in the \`agent\` artifact in the workflow run linked above.
@@ -1966,13 +1971,18 @@ gh pr create --title '${title}' --base ${baseBranch} --head ${branchName} --repo
19661971
}
19671972

19681973
const patchFileName = patchFilePath ? patchFilePath.replace("/tmp/gh-aw/", "") : "aw-unknown.patch";
1974+
const pushFailureMessage = sanitizeContent(neutralizeClosingKeywordsForIssueBody(pushError instanceof Error ? pushError.message : String(pushError)), { allowedAliases: allowedMentionAliases })
1975+
.replace(/\s+/g, " ")
1976+
.trim();
19691977
const fallbackBody = `${issueSafeBody}
19701978
19711979
---
19721980
19731981
> [!NOTE]
19741982
> This was originally intended as a pull request, but the git push operation failed.
19751983
>
1984+
> **Original error:** ${pushFailureMessage}
1985+
>
19761986
> **Workflow Run:** [View run details and download patch artifact](${runUrl})
19771987
>
19781988
> The patch file is available in the \`agent\` artifact in the workflow run linked above.

actions/setup/js/create_pull_request.test.cjs

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1018,6 +1018,48 @@ index 0000000..abc1234
10181018
expect(fallbackIssueBody).toContain("git reset --hard");
10191019
expect(fallbackIssueBody).toContain(`git update-ref -d ${fallbackBundleTempRef}`);
10201020
expect(fallbackIssueBody).not.toContain("refs/heads/autoloop/perf-comparison:refs/heads/autoloop/perf-comparison");
1021+
expect(fallbackIssueBody).toContain("**Original error:** push rejected");
1022+
expect(fallbackIssueBody).toContain("Test body");
1023+
expect(fallbackIssueBody).toContain("Closes \\#57");
1024+
expect(fallbackIssueBody).toContain("Resolves test-owner/test-repo\\#58");
1025+
expect(fallbackIssueBody).not.toContain("Closes #57");
1026+
expect(fallbackIssueBody).not.toContain("Resolves test-owner/test-repo#58");
1027+
});
1028+
1029+
it("should include original error in fallback issue patch instructions when push fails (no bundle)", async () => {
1030+
const patchPath = canonicalPatchPath("autoloop/perf-comparison");
1031+
fs.writeFileSync(
1032+
patchPath,
1033+
`From abc123 Mon Sep 17 00:00:00 2001
1034+
From: Test Author <test@example.com>
1035+
Date: Mon, 1 Jan 2024 00:00:00 +0000
1036+
Subject: [PATCH] Test commit
1037+
1038+
diff --git a/test.txt b/test.txt
1039+
new file mode 100644
1040+
index 0000000..abc1234
1041+
--- /dev/null
1042+
+++ b/test.txt
1043+
@@ -0,0 +1 @@
1044+
+Hello World
1045+
--
1046+
2.34.1
1047+
`
1048+
);
1049+
// No bundle file - forces the patch transport fallback path
1050+
pushSignedSpy.mockRejectedValueOnce(new Error("push rejected"));
1051+
1052+
const { main } = require("./create_pull_request.cjs");
1053+
const handler = await main({ base_branch: "main", preserve_branch_name: true });
1054+
const result = await handler({ title: "Test PR", body: "Test body\n\nCloses #57\nResolves test-owner/test-repo#58", branch: "autoloop/perf-comparison" }, {});
1055+
1056+
expect(result.success).toBe(true);
1057+
expect(result.fallback_used).toBe(true);
1058+
1059+
const fallbackIssueBody = global.github.rest.issues.create.mock.calls[0][0].body;
1060+
expect(fallbackIssueBody).toContain("**Original error:** push rejected");
1061+
expect(fallbackIssueBody).toContain("git am --3way");
1062+
expect(fallbackIssueBody).not.toContain("git update-ref");
10211063
expect(fallbackIssueBody).toContain("Test body");
10221064
expect(fallbackIssueBody).toContain("Closes \\#57");
10231065
expect(fallbackIssueBody).toContain("Resolves test-owner/test-repo\\#58");

actions/setup/js/create_pull_request_bundle_integration.test.cjs

Lines changed: 173 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -262,4 +262,177 @@ describe("create_pull_request bundle integration", () => {
262262
expect(fs.readFileSync(path.join(targetRepo, "feature.txt"), "utf8")).toBe("feature branch commit\n");
263263
expect(fs.readFileSync(path.join(targetRepo, "main.txt"), "utf8")).toBe("main branch commit\n");
264264
});
265+
266+
it("captures a push rejection error after applying a reconcile-spark diverged-history bundle", async () => {
267+
// ─── Why this test exists ────────────────────────────────────────────────
268+
//
269+
// The "reconcile-spark" chaos scenario exposed a gap in the
270+
// create_pull_request bundle path:
271+
//
272+
// 1. An agent works on a feature branch and makes commits.
273+
// 2. The main branch receives new commits while the agent is working
274+
// (history diverges).
275+
// 3. The agent reconciles by merging main into their branch, producing
276+
// a non-linear (merge-commit) history — this is the "reconcile-spark"
277+
// topology.
278+
// 4. A git bundle is created from that non-linear history.
279+
// 5. The bundle is applied to the safe-outputs runner via applyBundleToBranch.
280+
// 6. The subsequent push to origin fails because the remote branch has
281+
// also diverged (or a policy hook rejects the push).
282+
//
283+
// Previously, pushSignedCommits attempted a linear cherry-pick replay of
284+
// the commit range onto the current GraphQL parent. That path choked on
285+
// merge commits and produced a CONFLICT error, dropping the flow into the
286+
// fallback-issue path with no useful context.
287+
//
288+
// The fix adds a sanitized `pushFailureMessage` to the fallback issue body
289+
// so that manual recovery is deterministic. This integration test verifies:
290+
//
291+
// • applyBundleToBranch correctly imports a reconcile-spark merge topology
292+
// (merge commits are preserved, not flattened).
293+
// • A real git push to a diverged bare remote fails with an error — the
294+
// kind of raw error string that create_pull_request captures and sanitizes
295+
// before embedding in the fallback issue body.
296+
// • The raw error produced by git contains content that sanitization must
297+
// handle (the test injects an @-mention into the hook rejection message
298+
// to document the attack surface; sanitizeContent strips it in prod).
299+
//
300+
// ─────────────────────────────────────────────────────────────────────────
301+
302+
const branchName = "scratchpad/chaos/reconcile-spark";
303+
304+
// 1. Set up a bare "origin" repo and a working clone — this mimics the
305+
// relationship between GitHub and the safe-outputs runner checkout.
306+
const bareRemote = fs.mkdtempSync(path.join(os.tmpdir(), "create-pr-reconcile-spark-bare-"));
307+
const agentRepo = fs.mkdtempSync(path.join(os.tmpdir(), "create-pr-reconcile-spark-agent-"));
308+
const safeOutputsRepo = fs.mkdtempSync(path.join(os.tmpdir(), "create-pr-reconcile-spark-so-"));
309+
tempDirs.push(bareRemote, agentRepo, safeOutputsRepo);
310+
311+
// Initialize the bare remote and push a first commit onto main.
312+
// Use -b main so we never need to run git symbolic-ref inside the bare repo
313+
// (git 2.36+ restricts bare-repo commands unless safe.bareRepository=all).
314+
execGit(["init", "--bare", "-b", "main"], { cwd: bareRemote });
315+
execGit(["clone", bareRemote, "."], { cwd: agentRepo });
316+
execGit(["config", "user.name", "Agent"], { cwd: agentRepo });
317+
execGit(["config", "user.email", "agent@example.com"], { cwd: agentRepo });
318+
319+
fs.writeFileSync(path.join(agentRepo, "README.md"), "# Chaos scenario\n");
320+
execGit(["add", "README.md"], { cwd: agentRepo });
321+
execGit(["commit", "-m", "Initial commit on main"], { cwd: agentRepo });
322+
execGit(["branch", "-M", "main"], { cwd: agentRepo });
323+
execGit(["push", "-u", "origin", "main"], { cwd: agentRepo });
324+
core.info("[reconcile-spark] bare remote initialized with main");
325+
326+
// 2. Agent creates the feature branch and makes a first content commit.
327+
execGit(["checkout", "-b", branchName], { cwd: agentRepo });
328+
fs.writeFileSync(path.join(agentRepo, "notes.md"), "# Agent notes\n");
329+
execGit(["add", "notes.md"], { cwd: agentRepo });
330+
execGit(["commit", "-m", "feat: add initial notes"], { cwd: agentRepo });
331+
core.info("[reconcile-spark] agent made first commit on feature branch");
332+
333+
// 3. While the agent is working, a collaborator pushes a commit directly
334+
// to main on the remote. The agent's local main diverges from origin/main.
335+
// We simulate this by making a commit on main in the agent repo and then
336+
// force-pushing it so the bare remote has a commit the agent branch doesn't.
337+
execGit(["checkout", "main"], { cwd: agentRepo });
338+
fs.writeFileSync(path.join(agentRepo, "collab.md"), "# Collaborator change\n");
339+
execGit(["add", "collab.md"], { cwd: agentRepo });
340+
execGit(["commit", "-m", "collab: landing from main"], { cwd: agentRepo });
341+
execGit(["push", "origin", "main"], { cwd: agentRepo });
342+
core.info("[reconcile-spark] collaborator commit pushed to origin/main — histories now diverged");
343+
344+
// 4. Agent reconciles: merges the updated main back into the feature branch.
345+
// This creates the "reconcile-spark" non-linear merge commit.
346+
execGit(["checkout", branchName], { cwd: agentRepo });
347+
execGit(["merge", "--no-ff", "main", "-m", "reconcile: merge main into feature"], { cwd: agentRepo });
348+
core.info("[reconcile-spark] merge commit created — non-linear history established");
349+
350+
// Add one more commit after the reconcile merge to ensure the bundle tip is
351+
// beyond the merge (the pathological shape that the original linear-replay
352+
// path could not handle: a non-empty range starting with a merge parent).
353+
fs.writeFileSync(path.join(agentRepo, "notes.md"), "# Agent notes\n\nPost-reconcile edit\n");
354+
execGit(["commit", "-am", "feat: post-reconcile update"], { cwd: agentRepo });
355+
const expectedBundleTip = execGit(["rev-parse", "HEAD"], { cwd: agentRepo }).stdout.trim();
356+
const mergeCommitCount = Number(execGit(["rev-list", "--count", "--merges", `main..${branchName}`], { cwd: agentRepo }).stdout.trim());
357+
core.info(`[reconcile-spark] feature branch tip: ${expectedBundleTip.slice(0, 8)}, merge commits in range: ${mergeCommitCount}`);
358+
// Confirm the branch contains at least one merge commit — the test topology
359+
// is only valid when the reconcile merge is present.
360+
expect(mergeCommitCount).toBeGreaterThanOrEqual(1);
361+
362+
// 5. Create a git bundle from the reconcile-spark branch. The bundle
363+
// includes the full history so that the safe-outputs runner can apply it
364+
// without access to origin.
365+
const bundlePath = path.join(agentRepo, "reconcile-spark.bundle");
366+
execGit(["bundle", "create", bundlePath, `refs/heads/${branchName}`], { cwd: agentRepo });
367+
core.info(`[reconcile-spark] bundle created: ${bundlePath}`);
368+
369+
// 6. Set up the safe-outputs runner checkout — a fresh clone of origin/main.
370+
// This is the state the runner is in before it applies the agent's bundle.
371+
execGit(["clone", bareRemote, "."], { cwd: safeOutputsRepo });
372+
execGit(["config", "user.name", "Runner"], { cwd: safeOutputsRepo });
373+
execGit(["config", "user.email", "runner@example.com"], { cwd: safeOutputsRepo });
374+
execGit(["checkout", "-b", branchName], { cwd: safeOutputsRepo });
375+
core.info("[reconcile-spark] safe-outputs runner checkout ready");
376+
377+
// 7. Apply the bundle via applyBundleToBranch — the function under test.
378+
const { applyBundleToBranch } = require("./create_pull_request.cjs");
379+
await applyBundleToBranch(bundlePath, branchName, `refs/heads/${branchName}`, createExecApi(safeOutputsRepo));
380+
381+
// Verify that the merge-commit topology survived the bundle round-trip.
382+
const appliedTip = execGit(["rev-parse", "HEAD"], { cwd: safeOutputsRepo }).stdout.trim();
383+
const appliedMergeCount = Number(execGit(["rev-list", "--count", "--merges", "HEAD"], { cwd: safeOutputsRepo }).stdout.trim());
384+
core.info(`[reconcile-spark] bundle applied; tip: ${appliedTip.slice(0, 8)}, merges: ${appliedMergeCount}`);
385+
expect(appliedTip).toBe(expectedBundleTip);
386+
expect(appliedMergeCount).toBeGreaterThanOrEqual(1);
387+
expect(fs.readFileSync(path.join(safeOutputsRepo, "notes.md"), "utf8")).toContain("Post-reconcile edit");
388+
expect(fs.readFileSync(path.join(safeOutputsRepo, "collab.md"), "utf8")).toBe("# Collaborator change\n");
389+
390+
// 8. Simulate a push rejection.
391+
//
392+
// In the reconcile-spark scenario the push fails because:
393+
// (a) The remote branch may not accept non-fast-forward pushes, or
394+
// (b) A policy hook (e.g. "require signed commits") rejects the push.
395+
//
396+
// We reproduce (b) by installing a pre-receive hook that emits a message
397+
// containing an @-mention — deliberately chosen to document the attack
398+
// surface that sanitizeContent must neutralise before the error is
399+
// embedded in the fallback issue body.
400+
const hooksDir = path.join(bareRemote, "hooks");
401+
fs.mkdirSync(hooksDir, { recursive: true });
402+
const hookPath = path.join(hooksDir, "pre-receive");
403+
// The @org/team mention in the hook message is intentional: it demonstrates
404+
// the class of content (@ mentions, URLs, closing keywords) that can appear
405+
// in raw git push errors and must be stripped by sanitizeContent before the
406+
// message is interpolated into the fallback issue markdown body.
407+
fs.writeFileSync(
408+
hookPath,
409+
[
410+
"#!/bin/sh",
411+
"echo 'remote: error: pushSignedCommits: failed to rebase commit range onto current GraphQL parent (merge commit detected)' >&2",
412+
"echo 'remote: - CONFLICT (content): Merge conflict in scratchpad/chaos/reconcile-spark.md' >&2",
413+
"echo 'remote: - See @org/team for recovery steps.' >&2",
414+
"exit 1",
415+
].join("\n") + "\n"
416+
);
417+
fs.chmodSync(hookPath, "0755");
418+
419+
// Attempt the real git push — this MUST fail so we can capture the error.
420+
const pushResult = execGit(["push", "origin", branchName], { cwd: safeOutputsRepo, allowFailure: true });
421+
core.info(`[reconcile-spark] push exit code: ${pushResult.status}`);
422+
core.info(`[reconcile-spark] push stderr: ${pushResult.stderr.trim()}`);
423+
expect(pushResult.status).not.toBe(0);
424+
425+
// 9. Verify the raw push error contains the content that create_pull_request
426+
// must sanitize and embed in the fallback issue body.
427+
// This is the value that will be passed through:
428+
// sanitizeContent(neutralizeClosingKeywordsForIssueBody(pushError.message), ...)
429+
// before being written into the fallback issue markdown.
430+
const rawPushError = pushResult.stderr.trim();
431+
expect(rawPushError).toContain("merge commit detected");
432+
expect(rawPushError).toContain("CONFLICT");
433+
// The @-mention in the hook output confirms that unsanitized error text can
434+
// contain @ tokens — sanitizeContent replaces them with safe equivalents.
435+
expect(rawPushError).toContain("@org/team");
436+
core.info("[reconcile-spark] push error captured — ready for sanitization and fallback issue embedding");
437+
});
265438
});

pkg/workflow/header.go

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -70,7 +70,11 @@ func GenerateWorkflowHeader(sourceFile string, generatedBy string, customInstruc
7070
instructionLines := strings.Split(strings.TrimSpace(customInstructions), "\n")
7171
headerLog.Printf("Adding %d lines of custom instructions", len(instructionLines))
7272
for _, line := range instructionLines {
73-
fmt.Fprintf(&header, "# %s\n", strings.TrimSpace(line))
73+
if trimmedLine := strings.TrimSpace(line); trimmedLine == "" {
74+
header.WriteString("#\n")
75+
} else {
76+
fmt.Fprintf(&header, "# %s\n", trimmedLine)
77+
}
7478
}
7579
}
7680

0 commit comments

Comments
 (0)