pushSignedCommits: surface actionable error with GPG seed instructions on orphan-branch push failure#32365
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
…nual seeding instructions When an orphan branch's first push is rejected (e.g. GH013 from a "Require signed commits" ruleset), catch the error and re-throw with step-by-step instructions for manually seeding the branch with a signed commit using a local GPG key: git switch --orphan <branch> git commit --allow-empty -S -m "Initialize <branch>" git push origin <branch> Also adds a test that installs a pre-receive hook in the bare repo to simulate the GH013 rejection and verifies the error message contains the correct manual seeding commands. Addresses the regression from #30463 and the original issue #29301. Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Adds an actionable error message when the orphan-branch first-push path in pushSignedCommits fails (typically due to Require signed commits enforcement / GH013 rejection), instructing the operator to seed the branch manually with a signed commit.
Changes:
- Wrap
pushBranchAndResolveHeadcall in the!baseReforphan branch path with try/catch, re-throwing with branch-specific seeding commands and preserving the original error viacause. - Add an integration test that installs a
pre-receivehook simulating GH013 rejection and asserts the thrown error contains the expected manual seeding commands. - Unrelated workflow
.lock.ymlregenerations (hash/heredoc churn, Sentry tool list reordering to addlist_events/list_issue_events).
Show a summary per file
| File | Description |
|---|---|
| actions/setup/js/push_signed_commits.cjs | Adds try/catch around orphan-branch git push to surface an actionable error. |
| actions/setup/js/push_signed_commits.test.cjs | New integration test simulating GH013 via pre-receive hook. |
| .github/workflows/smoke-otel-backends.lock.yml | Regenerated lockfile; adds Sentry list_events/list_issue_events tools. |
| .github/workflows/mcp-inspector.lock.yml | Regenerated lockfile; adds Sentry list_events/list_issue_events tools. |
| .github/workflows/daily-token-consumption-report.lock.yml | Regenerated lockfile; reorders Sentry tool comments. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 5/5 changed files
- Comments generated: 0
|
@copilot merge main, recompile, review comments |
🧪 Test Quality Sentinel ReportTest Quality Score: 80/100✅ Excellent test quality
Test Classification Details
Analysis
Classification: Design test (behavioral contract) What design invariant does this test enforce? When What would break if deleted? A regression where the error reverts to a generic, non-actionable message (or swallows the error entirely) would go undetected. The GPG seed instructions are the core behavioral contract added by this PR. Strengths:
Minor note — test inflation: The test file grew by 46 lines vs. 17 lines in the production file (2.7:1). For integration-style tests that spin up real git repos and hooks, a higher line ratio is expected and not a quality concern here. Language SupportTests analyzed:
Verdict
📖 Understanding Test ClassificationsDesign Tests (High Value) verify what the system does:
Implementation Tests (Low Value) verify how the system does it:
Goal: Shift toward tests that describe the system's behavioral contract — the promises it makes to its users and collaborators. References: §25919942189
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ Test Quality Sentinel: 80/100. Test quality is excellent — 0% of new tests are implementation tests (threshold: 30%). The new integration-style test directly verifies the behavioral contract for the orphan-branch error message introduced by this PR.
|
Hey One thing worth tidying up before review:
If you'd like a hand cleaning this up, you can assign the following prompt to your coding agent:
|
There was a problem hiding this comment.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · ● 14.2M
| } catch (err) { | ||
| thrownErr = err; | ||
| } | ||
| expect(thrownErr).toBeDefined(); |
There was a problem hiding this comment.
[/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("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"); |
There was a problem hiding this comment.
[/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.
| 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` + |
There was a problem hiding this comment.
[/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.
…anch-signed-commits # Conflicts: # .github/workflows/daily-token-consumption-report.lock.yml # .github/workflows/mcp-inspector.lock.yml # .github/workflows/smoke-otel-backends.lock.yml Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Done — merged main, rebuilt the binary, and recompiled all 229 lock files. All 40 tests pass. Commit: |
|
@copilot review all review comments |
The automated review found no issues — 0 comments generated across all 5 changed files. Nothing to address. |
|
@copilot review all comments from the github actions bot |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
The CJS lint job was failing because |
|
@copilot merge main and recompile |
…anch-signed-commits Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Done — merged main (4 new commits) and recompiled all 229 lock files. Commit: |
Bug Fix
What was the bug?
The
!baseRefearly-return guard (added in #30463 to fix silently-dropped orphan commits) falls back to plaingit pushfor root commits — bypassing the GraphQL signed-commit path. On repos withRequire signed commitsenforced, this produces a GH013 rejection on every attempt. Because the branch is never created, every subsequent run re-enters the same orphan path and fails identically; memory never persists.There is no GitHub API surface that can produce a server-signed root commit on a brand-new orphan branch today (
createCommitOnBranchrequires a non-nullexpectedHeadOid; REST contents API requires the branch to already exist). The branch must be seeded manually.How did you fix it?
Wrapped the
pushBranchAndResolveHeadcall in the!baseRefblock with a try/catch. On failure, re-throw with a structured error that tells the operator exactly what to run:push_signed_commits.cjs—try/catcharoundpushBranchAndResolveHeadin the!baseRefguard; re-throw with branch-specific commands on failure.push_signed_commits.test.cjs— new test in theorphan branch first pushdescribe block: installs apre-receivehook in the bare repo that mimics GH013, asserts the thrown error contains all three manual-seeding commands.Testing
New test simulates GH013 rejection via a bare-repo
pre-receivehook and asserts the error message contains the correctgit switch --orphan,git commit --allow-empty -S, andgit push origincommands for the branch. All 40 tests pass.