Fix bundle-apply race: capture git stderr to recover missing prerequisite commits#32360
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
…uisite recovery Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
Hey A couple of things to address before this is ready for review:
If you'd like a hand completing the implementation, you can assign this prompt to your coding agent:
|
There was a problem hiding this comment.
Pull request overview
This PR fixes a race where applying a git bundle can fail in safe_outputs if the bundle’s prerequisite commits are no longer reachable (e.g., due to shallow checkout + main advancing). It does so by capturing the actual git fetch stderr (instead of losing it via exec() throwing a generic error), extracting missing prerequisite SHAs, fetching them from origin, and retrying the bundle fetch.
Changes:
- Switch initial bundle fetch to
getExecOutput(..., { ignoreReturnCode: true })so git stderr is available for prerequisite SHA extraction/recovery. - Add/enable prerequisite recovery for both
create_pull_requestandpush_to_pull_request_branch, and centralize SHA extraction ingit_helpers.cjs. - Update unit/integration tests and exec mocks to reflect the new
getExecOutput-first behavior.
Show a summary per file
| File | Description |
|---|---|
| actions/setup/js/git_helpers.cjs | Adds/export extractBundlePrerequisiteCommits so multiple handlers can share the bundle prerequisite parsing logic. |
| actions/setup/js/git_helpers.test.cjs | Adds unit tests for extractBundlePrerequisiteCommits behavior (empty, single/multi SHA, dedupe, case-insensitive header). |
| actions/setup/js/create_pull_request.cjs | Uses getExecOutput to capture bundle-fetch stderr and activate prerequisite recovery; removes local extractor in favor of shared helper. |
| actions/setup/js/create_pull_request.test.cjs | Updates mocks/assertions for getExecOutput-first fetch and adds coverage for prerequisite recovery scenarios. |
| actions/setup/js/create_pull_request_bundle_integration.test.cjs | Updates the test exec API to honor ignoreReturnCode and to record getExecOutput invocations consistently. |
| actions/setup/js/push_to_pull_request_branch.cjs | Adds bundle prerequisite recovery (extract SHAs from stderr, fetch missing SHAs from origin, retry bundle fetch). |
| actions/setup/js/push_to_pull_request_branch.test.cjs | Updates bundle-fetch expectations (now getExecOutput) and adds a test for prerequisite recovery + retry. |
| .github/workflows/smoke-otel-backends.lock.yml | Lockfile regen/update; includes new Sentry MCP permissions entries. |
| .github/workflows/mcp-inspector.lock.yml | Lockfile regen/update; includes new Sentry MCP permissions entries. |
| .github/workflows/daily-token-consumption-report.lock.yml | Lockfile regen/update; includes new Sentry MCP permissions entries and reordered commented tool list. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comments suppressed due to low confidence (1)
actions/setup/js/create_pull_request.cjs:138
- The prerequisite-recovery path fetches missing SHAs from
originwithout providingGIT_CONFIG_*auth env. In runs whereclean_git_credentials.shhas stripped credentials from.git/config,git fetch origin <sha>(and the--unshallowinsideensureFullHistoryForBundle) can fail, which would make this recovery unreliable. Consider threading an auth-aware exec options object intoapplyBundleToBranch(e.g.,{ env: { ...process.env, ...getGitAuthEnv(config['github-token']) } }) and using it forensureFullHistoryForBundle(...)and thegit fetch origin ...call(s).
if (prerequisiteCommits.length > 0) {
core.warning(`Bundle fetch with ${bundleBranchRef} failed due to ${prerequisiteCommits.length} missing prerequisite commit(s); fetching prerequisites from origin and retrying`);
core.info(`Prerequisite commits: ${summarizeListForLog(prerequisiteCommits)}`);
core.info(`Fetching ${prerequisiteCommits.length} prerequisite commit(s) from origin`);
await execApi.exec("git", ["fetch", "origin", ...prerequisiteCommits]);
- Files reviewed: 10/10 changed files
- Comments generated: 0
|
@copilot merge main and recompile |
…ply-race-issue # 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>
Done — merged |
Bug Fix
What was the bug?
The
safe_outputsjob usesfetch-depth: 1, so if another PR merges tomainbetween agent-time and safe_outputs-time, the bundle's base commit (present when the agent ran) is no longer reachable.git fetch <bundle>then fails withRepository lacks these prerequisite commits: <SHA>.A recovery path already existed in
create_pull_request.cjsto parse missing SHAs and fetch them from origin — but it was dead code.@actions/exec'sexec()throws only"The process '/usr/bin/git' failed with exit code 1"; the actual git stderr (which contains the prerequisite SHA lines) is printed to the Actions log but never surfaced in the thrownError. The SHA regex never matched, so recovery never ran.push_to_pull_request_branch.cjshad no recovery path at all.How did you fix it?
Switch the initial bundle fetch from
exec()togetExecOutput(..., { ignoreReturnCode: true }), which returns{ exitCode, stdout, stderr }without throwing. Pass the capturedstderrtoextractBundlePrerequisiteCommits, fetch the missing SHAs from origin, and retry.Changes
git_helpers.cjs— movedextractBundlePrerequisiteCommitshere fromcreate_pull_request.cjs; exported it for both handlerscreate_pull_request.cjs— initial bundle fetch switched togetExecOutputwithignoreReturnCode: true; prerequisite recovery path now actually receives git stderrpush_to_pull_request_branch.cjs— added prerequisite recovery path (was entirely absent); same patterngetExecOutput-first pattern; added unit tests forextractBundlePrerequisiteCommits; new integration and recovery-path tests; fixedcreateExecApiin the bundle integration test to honourignoreReturnCodeand fire theonExeccallback ingetExecOutput