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
20 changes: 17 additions & 3 deletions actions/setup/js/push_signed_commits.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -184,9 +184,23 @@ async function pushSignedCommits({ githubClient, owner, repo, branch, baseRef, c
// so skip it entirely and fall directly through to git push.
if (!baseRef) {
core.info(`pushSignedCommits: empty baseRef detected (orphan branch first push), using git push directly for branch ${branch}`);
const headSha = await pushBranchAndResolveHead({ branch, cwd, gitAuthEnv });
core.info(`pushSignedCommits: git push completed for orphan branch, HEAD=${headSha}`);
return headSha;
try {
const headSha = await pushBranchAndResolveHead({ branch, cwd, gitAuthEnv });
core.info(`pushSignedCommits: git push completed for orphan branch, HEAD=${headSha}`);
return headSha;
} catch (pushErr) {
const pushErrMsg = pushErr instanceof Error ? pushErr.message : String(pushErr);
throw new Error(
`pushSignedCommits: failed to push orphan branch '${branch}' (first commit). ` +
`If the repository requires signed commits, the branch must be seeded manually with a signed commit before this workflow can push to it. ` +
`Run the following commands locally (requires a GPG key configured with Git):\n\n` +
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/diagnose] The parenthetical "(requires a GPG key configured with Git)" is inaccurate — GitHub's Require signed commits rule also accepts SSH-signed commits (git config gpg.format ssh). A user with only an SSH signing key will follow this message and spend time on GPG setup they don't need.

Suggest widening the wording:

`Run the following commands locally (requires a signing key — GPG or SSH — configured with Git):`

Or simply drop the parenthetical and let the git commit --allow-empty -S command surface its own error for users who haven't configured signing.

` git switch --orphan ${branch}\n` +
` git commit --allow-empty -S -m "Initialize ${branch}"\n` +
` git push origin ${branch}\n\n` +
`Original error: ${pushErrMsg}`,
{ cause: pushErr }
);
}
}

// Collect the commits introduced (oldest-first) using topological order to ensure
Expand Down
43 changes: 43 additions & 0 deletions actions/setup/js/push_signed_commits.test.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -645,6 +645,49 @@ describe("push_signed_commits integration tests", () => {
// Return value should be the HEAD SHA.
expect(result).toBe(expectedSha);
});

it("should throw with manual seeding instructions when orphan-branch git push fails", async () => {
// Simulate a repo where "Require signed commits" is enforced. The orphan-branch
// first push uses git push directly, which will be rejected by the remote with
// GH013. We simulate this by using a bare repo that refuses the push via a
// pre-receive hook.
execGit(["checkout", "--orphan", "experiments/signed-required"], { cwd: workDir });
execGit(["read-tree", "--empty"], { cwd: workDir });
fs.writeFileSync(path.join(workDir, "state.json"), JSON.stringify({ runs: 1 }));
execGit(["add", "state.json"], { cwd: workDir });
execGit(["commit", "-m", "Initial experiment state"], { cwd: workDir });

// Install a pre-receive hook in the bare repo that mimics GH013 by rejecting all pushes.
const hooksDir = path.join(bareDir, "hooks");
fs.mkdirSync(hooksDir, { recursive: true });
const hookPath = path.join(hooksDir, "pre-receive");
fs.writeFileSync(hookPath, "#!/bin/sh\necho 'remote: error: GH013: Repository rule violations found.' >&2\necho 'remote: - Commits must have verified signatures.' >&2\nexit 1\n");
fs.chmodSync(hookPath, "0755");

// Use the real exec so git push actually runs and hits the hook.
global.exec = makeRealExec(workDir);
const githubClient = makeMockGithubClient();

let thrownErr;
try {
await pushSignedCommits({
githubClient,
owner: "test-owner",
repo: "test-repo",
branch: "experiments/signed-required",
baseRef: "",
cwd: workDir,
});
} catch (err) {
thrownErr = err;
}
expect(thrownErr).toBeDefined();
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/tdd] Consider using Vitest's idiomatic rejects pattern instead of the manual try/catch + thrownErr approach — it's more concise and produces clearer failure messages when no error is thrown:

await expect(
  pushSignedCommits({
    githubClient,
    owner: "test-owner",
    repo: "test-repo",
    branch: "experiments/signed-required",
    baseRef: "",
    cwd: workDir,
  })
).rejects.toThrow("failed to push orphan branch");

The other assertions on thrownErr.message can follow after const err = await ... with an expect(...).rejects chain, or use a helper. The existing pattern in lines 823, 888, 1166, 1456 already uses rejects.toThrow, so adopting it here keeps the test file consistent.

expect(thrownErr.message).toContain("failed to push orphan branch");
expect(thrownErr.message).toContain("git switch --orphan experiments/signed-required");
expect(thrownErr.message).toContain("git commit --allow-empty -S");
expect(thrownErr.message).toContain("git push origin experiments/signed-required");
expect(thrownErr.message).toContain("signed commits");
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/tdd] The { cause: pushErr } set in the implementation is never asserted here, so a regression that accidentally drops the error chain would go undetected. Consider adding:

expect(thrownErr.cause).toBeDefined();
expect(thrownErr.cause.message).toMatch(/GH013|exit code/i);

Preserving the original error as cause is important for downstream debuggability, and testing it locks down that contract.

});
});

// ──────────────────────────────────────────────────────
Expand Down