Skip to content

Commit cc0d007

Browse files
dsymedsyme
authored
fix: query repo default branch instead of hardcoding 'main' (#20098) (#20099)
* fix: query repo default branch instead of hardcoding 'main' (#20098) When no explicit base branch is configured and the event context doesn't provide one, getBaseBranch() was falling back to hardcoded "main". This caused failures for repos whose default branch is not "main" (e.g. "master", "develop") - the git fetch for the base branch would fail with "fatal: couldn't find remote ref main". Fix: add a new step 5 that queries the GitHub API for the repository's default_branch before falling back to DEFAULT_BRANCH env var or "main". * fix: resolve base branch before cross-repo checkout; remove hardcoded 'master' fallback - In create_pull_request.cjs, move getBaseBranch() resolution to before checkoutManager.switchTo() so cross-repo checkout uses the correct default branch instead of hardcoded 'main' - In dynamic_checkout.cjs, remove the hardcoded 'master' fallback when 'main' is not found. This assumed only 'main' or 'master' could be the default branch, which is incorrect. The resolved branch is now always passed correctly by the caller. * test: add tests for no-silent-master-fallback behavior in dynamic_checkout * perf: use context.payload.repository.default_branch before API call in step 5 Most GitHub Actions event payloads include repository.default_branch, so we can read it directly from the payload without making an API call. Only fall back to repos.get() when: - targetRepo is explicitly provided (cross-repo: payload would have the wrong repo's default branch) - The payload doesn't include repository.default_branch This matches the pattern used in dispatch_workflow.cjs and avoids unnecessary API calls that count against rate limits. * update get base branch
1 parent 482aa92 commit cc0d007

5 files changed

Lines changed: 206 additions & 18 deletions

File tree

‎actions/setup/js/create_pull_request.cjs‎

Lines changed: 8 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -257,10 +257,17 @@ async function main(config = {}) {
257257
const { repo: itemRepo, repoParts } = repoResult;
258258
core.info(`Target repository: ${itemRepo}`);
259259

260+
// Resolve base branch for this target repository
261+
// Use config value if set, otherwise resolve dynamically for the specific target repo
262+
// Dynamic resolution is needed for issue_comment events on PRs where the base branch
263+
// is not available in GitHub Actions expressions and requires an API call
264+
// NOTE: Must be resolved before checkout so cross-repo checkout uses the correct branch
265+
let baseBranch = configBaseBranch || (await getBaseBranch(repoParts));
266+
260267
// Multi-repo support: Switch checkout to target repo if different from current
261268
// This enables creating PRs in multiple repos from a single workflow run
262269
if (checkoutManager && itemRepo) {
263-
const switchResult = await checkoutManager.switchTo(itemRepo, { baseBranch: configBaseBranch });
270+
const switchResult = await checkoutManager.switchTo(itemRepo, { baseBranch });
264271
if (!switchResult.success) {
265272
core.warning(`Failed to switch to repository ${itemRepo}: ${switchResult.error}`);
266273
return {
@@ -273,12 +280,6 @@ async function main(config = {}) {
273280
}
274281
}
275282

276-
// Resolve base branch for this target repository
277-
// Use config value if set, otherwise resolve dynamically for the specific target repo
278-
// Dynamic resolution is needed for issue_comment events on PRs where the base branch
279-
// is not available in GitHub Actions expressions and requires an API call
280-
let baseBranch = configBaseBranch || (await getBaseBranch(repoParts));
281-
282283
// SECURITY: Sanitize dynamically resolved base branch to prevent shell injection
283284
const originalBaseBranch = baseBranch;
284285
baseBranch = normalizeBranchName(baseBranch);

‎actions/setup/js/dynamic_checkout.cjs‎

Lines changed: 1 addition & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -108,13 +108,7 @@ async function checkoutRepo(repoSlug, token, options = {}) {
108108
try {
109109
await exec.exec("git", ["checkout", "-B", baseBranch, `origin/${baseBranch}`]);
110110
} catch {
111-
// Try 'master' if 'main' doesn't exist
112-
if (baseBranch === "main") {
113-
core.info("main branch not found, trying master");
114-
await exec.exec("git", ["checkout", "-B", "master", "origin/master"]);
115-
} else {
116-
throw new Error(`Base branch ${baseBranch} not found in ${repoSlug}`);
117-
}
111+
throw new Error(`Base branch ${baseBranch} not found in ${repoSlug}`);
118112
}
119113

120114
// Clean up any local changes

‎actions/setup/js/dynamic_checkout.test.cjs‎

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -167,6 +167,40 @@ describe("checkoutRepo slug validation", () => {
167167
expect(result.error).not.toContain("Invalid repository slug");
168168
}
169169
});
170+
171+
it("should fail with error when specified branch does not exist, not silently fall back", async () => {
172+
// Simulate git checkout failing for the specified branch
173+
let callCount = 0;
174+
mockExec.exec = vi.fn().mockImplementation((_cmd, args) => {
175+
if (args && args[0] === "checkout") {
176+
throw new Error("fatal: Remote branch develop not found");
177+
}
178+
return Promise.resolve(0);
179+
});
180+
181+
const result = await checkoutRepo("owner/repo", "fake-token", { baseBranch: "develop" });
182+
183+
// Should fail with an error about the branch, NOT silently fall back to master
184+
expect(result.success).toBe(false);
185+
expect(result.error).toContain("develop");
186+
expect(result.error).not.toContain("master");
187+
});
188+
189+
it("should fail with error when 'main' branch does not exist, not silently fall back to master", async () => {
190+
// Simulate git checkout failing for 'main' - previously this would silently try 'master'
191+
mockExec.exec = vi.fn().mockImplementation((_cmd, args) => {
192+
if (args && args[0] === "checkout") {
193+
throw new Error("fatal: Remote branch main not found");
194+
}
195+
return Promise.resolve(0);
196+
});
197+
198+
const result = await checkoutRepo("owner/repo", "fake-token", { baseBranch: "main" });
199+
200+
// Should fail rather than silently trying 'master'
201+
expect(result.success).toBe(false);
202+
expect(result.error).toContain("main");
203+
});
170204
});
171205

172206
describe("getCurrentCheckoutRepo URL parsing", () => {

‎actions/setup/js/get_base_branch.cjs‎

Lines changed: 52 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@
22
/// <reference types="@actions/github-script" />
33

44
const { validateTargetRepo, parseAllowedRepos, getDefaultTargetRepo } = require("./repo_helpers.cjs");
5+
const { getErrorMessage } = require("./error_helpers.cjs");
56

67
/**
78
* Get the base branch name, resolving dynamically based on event context.
@@ -11,10 +12,12 @@ const { validateTargetRepo, parseAllowedRepos, getDefaultTargetRepo } = require(
1112
* 2. github.base_ref env var (set for pull_request/pull_request_target events)
1213
* 3. Pull request payload base ref (pull_request_review, pull_request_review_comment events)
1314
* 4. API lookup for issue_comment events on PRs (the PR's base ref is not in the payload)
14-
* 5. Fallback to DEFAULT_BRANCH env var or "main"
15+
* 5. context.payload.repository.default_branch (included in most event payloads, no API call)
16+
* 5b. API lookup via repos.get() when payload doesn't have it (e.g. cross-repo scenarios)
17+
* 6. Fallback to DEFAULT_BRANCH env var or "main"
1518
*
1619
* @param {{owner: string, repo: string}|null} [targetRepo] - Optional target repository.
17-
* If provided, API calls (step 4) use this instead of context.repo,
20+
* If provided, API calls (steps 4 and 5) use this instead of context.repo,
1821
* which is needed for cross-repo scenarios where the target repo differs
1922
* from the workflow repository.
2023
* @returns {Promise<string>} The base branch name
@@ -67,12 +70,57 @@ async function getBaseBranch(targetRepo = null) {
6770
} catch (/** @type {any} */ error) {
6871
// Fall through to default if API call fails
6972
if (typeof core !== "undefined") {
70-
core.warning(`Failed to fetch PR base branch: ${error.message}`);
73+
core.warning(`Failed to fetch PR base branch: ${getErrorMessage(error)}`);
7174
}
7275
}
7376
}
7477

75-
// 5. Fallback to DEFAULT_BRANCH env var or "main"
78+
// 5. Repository default branch - from payload first, then API lookup
79+
// Many events include context.payload.repository.default_branch, so we check that
80+
// first to avoid an unnecessary API call and reduce rate-limit risk.
81+
// Only fall back to repos.get() when the payload doesn't have the value
82+
// (e.g. cross-repo scenarios where targetRepo differs from the workflow repo).
83+
{
84+
const repoOwner = targetRepo?.owner ?? (typeof context !== "undefined" ? context.repo?.owner : null);
85+
const repoName = targetRepo?.repo ?? (typeof context !== "undefined" ? context.repo?.repo : null);
86+
87+
// If no targetRepo override, check the payload first (free - no API call)
88+
if (!targetRepo && typeof context !== "undefined" && context.payload?.repository?.default_branch) {
89+
return context.payload.repository.default_branch;
90+
}
91+
92+
// Otherwise fall back to repos.get() API call
93+
if (repoOwner && repoName && typeof github !== "undefined") {
94+
try {
95+
// SECURITY: Validate target repo against allowlist before any API calls
96+
const targetRepoSlug = `${repoOwner}/${repoName}`;
97+
const allowedRepos = parseAllowedRepos(process.env.GH_AW_ALLOWED_REPOS);
98+
if (allowedRepos.size > 0) {
99+
const defaultRepo = getDefaultTargetRepo();
100+
const validation = validateTargetRepo(targetRepoSlug, defaultRepo, allowedRepos);
101+
if (!validation.valid) {
102+
if (typeof core !== "undefined") {
103+
core.warning(`ERR_VALIDATION: ${validation.error}`);
104+
}
105+
return process.env.DEFAULT_BRANCH || "main";
106+
}
107+
}
108+
109+
const { data: repoData } = await github.rest.repos.get({
110+
owner: repoOwner,
111+
repo: repoName,
112+
});
113+
return repoData.default_branch;
114+
} catch (/** @type {any} */ error) {
115+
// Fall through to default if API call fails
116+
if (typeof core !== "undefined") {
117+
core.warning(`Failed to fetch repository default branch: ${getErrorMessage(error)}`);
118+
}
119+
}
120+
}
121+
}
122+
123+
// 6. Fallback to DEFAULT_BRANCH env var or "main"
76124
return process.env.DEFAULT_BRANCH || "main";
77125
}
78126

‎actions/setup/js/get_base_branch.test.cjs‎

Lines changed: 111 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -99,4 +99,115 @@ describe("getBaseBranch", () => {
9999
const result3 = await getBaseBranch();
100100
expect(result3).toBe("custom-branch");
101101
});
102+
103+
it("should return context.payload.repository.default_branch without an API call", async () => {
104+
const mockGetRepo = vi.fn();
105+
global.github = {
106+
rest: { repos: { get: mockGetRepo } },
107+
};
108+
global.context = {
109+
repo: { owner: "test-owner", repo: "test-repo" },
110+
eventName: "push",
111+
payload: { repository: { default_branch: "trunk" } },
112+
};
113+
114+
const { getBaseBranch } = await import("./get_base_branch.cjs");
115+
const result = await getBaseBranch();
116+
117+
expect(result).toBe("trunk");
118+
expect(mockGetRepo).not.toHaveBeenCalled();
119+
});
120+
121+
it("should query GitHub API for repo default branch when payload does not have it", async () => {
122+
const mockGetRepo = vi.fn().mockResolvedValue({ data: { default_branch: "master" } });
123+
global.github = {
124+
rest: {
125+
repos: {
126+
get: mockGetRepo,
127+
},
128+
},
129+
};
130+
global.context = {
131+
repo: { owner: "test-owner", repo: "test-repo" },
132+
eventName: "workflow_dispatch",
133+
payload: {},
134+
};
135+
136+
const { getBaseBranch } = await import("./get_base_branch.cjs");
137+
const result = await getBaseBranch();
138+
139+
expect(mockGetRepo).toHaveBeenCalledWith({ owner: "test-owner", repo: "test-repo" });
140+
expect(result).toBe("master");
141+
});
142+
143+
it("should use repos.get() for targetRepo (cross-repo) even when payload has default_branch", async () => {
144+
const mockGetRepo = vi.fn().mockResolvedValue({ data: { default_branch: "develop" } });
145+
global.github = {
146+
rest: { repos: { get: mockGetRepo } },
147+
};
148+
global.context = {
149+
repo: { owner: "workflow-owner", repo: "workflow-repo" },
150+
eventName: "issue_comment",
151+
payload: { repository: { default_branch: "main" } },
152+
};
153+
154+
const { getBaseBranch } = await import("./get_base_branch.cjs");
155+
// targetRepo differs from the workflow repo - must use API, not payload
156+
const result = await getBaseBranch({ owner: "target-owner", repo: "target-repo" });
157+
158+
expect(mockGetRepo).toHaveBeenCalledWith({ owner: "target-owner", repo: "target-repo" });
159+
expect(result).toBe("develop");
160+
});
161+
162+
it("should use targetRepo for API default branch lookup", async () => {
163+
const mockGetRepo = vi.fn().mockResolvedValue({ data: { default_branch: "develop" } });
164+
global.github = {
165+
rest: {
166+
repos: {
167+
get: mockGetRepo,
168+
},
169+
},
170+
};
171+
172+
const { getBaseBranch } = await import("./get_base_branch.cjs");
173+
const result = await getBaseBranch({ owner: "target-owner", repo: "target-repo" });
174+
175+
expect(mockGetRepo).toHaveBeenCalledWith({ owner: "target-owner", repo: "target-repo" });
176+
expect(result).toBe("develop");
177+
});
178+
179+
it("should fall through to DEFAULT_BRANCH when API lookup for repo default branch fails", async () => {
180+
const mockWarning = vi.fn();
181+
global.github = {
182+
rest: {
183+
repos: {
184+
get: vi.fn().mockRejectedValue(new Error("API error")),
185+
},
186+
},
187+
};
188+
global.core = { warning: mockWarning };
189+
process.env.DEFAULT_BRANCH = "trunk";
190+
191+
const { getBaseBranch } = await import("./get_base_branch.cjs");
192+
const result = await getBaseBranch({ owner: "owner", repo: "repo" });
193+
194+
expect(result).toBe("trunk");
195+
expect(mockWarning).toHaveBeenCalledWith(expect.stringContaining("Failed to fetch repository default branch"));
196+
});
197+
198+
it("should fall through to hardcoded main when API lookup fails and no DEFAULT_BRANCH set", async () => {
199+
global.github = {
200+
rest: {
201+
repos: {
202+
get: vi.fn().mockRejectedValue(new Error("API error")),
203+
},
204+
},
205+
};
206+
global.core = { warning: vi.fn() };
207+
208+
const { getBaseBranch } = await import("./get_base_branch.cjs");
209+
const result = await getBaseBranch({ owner: "owner", repo: "repo" });
210+
211+
expect(result).toBe("main");
212+
});
102213
});

0 commit comments

Comments
 (0)