Skip to content

fix: fall back to repo default_branch instead of hardcoded "main" #1143

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
ashwin-ant merged 2 commits into from
Apr 1, 2026
Merged

fix: fall back to repo default_branch instead of hardcoded "main" #1143

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1e64a10
fix: fall back to repo default_branch instead of hardcoded "main"
ashwin-ant Apr 1, 2026
718d207
test: drop unused BASE_BRANCH env handling from default_branch test
ashwin-ant Apr 1, 2026
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
6 changes: 3 additions & 3 deletions src/entrypoints/run.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -229,8 +229,8 @@ async function run() {
// Restore them from the base branch before the CLI reads them.
//
// We read pull_request.base.ref from the payload directly because agent
// mode's branchInfo.baseBranch defaults to "main" rather than the PR's
// actual target (agent/index.ts). For issue_comment on a PR the payload
// mode's branchInfo.baseBranch defaults to the repo's default branch rather
// than the PR's actual target (agent/index.ts). For issue_comment on a PR the payload
// lacks base.ref, so we fall back to the mode-provided value — tag mode
// fetches it from GraphQL; agent mode on issue_comment is an edge case
// that at worst restores from the wrong trusted branch (still secure).
Expand Down Expand Up @@ -312,7 +312,7 @@ async function run() {
commentId,
githubToken,
claudeBranch,
baseBranch: baseBranch || "main",
baseBranch: baseBranch || context.repository.default_branch || "main",
triggerUsername: context.actor,
context,
octokit,
Expand Down
3 changes: 2 additions & 1 deletion src/entrypoints/update-comment-link.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -253,7 +253,8 @@ async function run() {
commentId: parseInt(process.env.CLAUDE_COMMENT_ID!),
githubToken,
claudeBranch: process.env.CLAUDE_BRANCH,
baseBranch: process.env.BASE_BRANCH || "main",
baseBranch:
process.env.BASE_BRANCH || context.repository.default_branch || "main",
triggerUsername: process.env.TRIGGER_USERNAME,
context,
octokit,
Expand Down
2 changes: 2 additions & 0 deletions src/github/context.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,7 @@ type BaseContext = {
owner: string;
repo: string;
full_name: string;
default_branch?: string;
};
actor: string;
inputs: {
Expand Down Expand Up @@ -140,6 +141,7 @@ export function parseGitHubContext(): GitHubContext {
owner: context.repo.owner,
repo: context.repo.repo,
full_name: `${context.repo.owner}/${context.repo.repo}`,
default_branch: context.payload.repository?.default_branch,
},
actor: context.actor,
inputs: {
Expand Down
6 changes: 3 additions & 3 deletions src/modes/agent/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,15 +85,15 @@ export async function prepareAgentMode({

// Check for branch info from environment variables (useful for auto-fix workflows)
const claudeBranch = process.env.CLAUDE_BRANCH || undefined;
const baseBranch =
process.env.BASE_BRANCH || context.inputs.baseBranch || "main";
const defaultBranch = context.repository.default_branch || "main";
const baseBranch = context.inputs.baseBranch || defaultBranch;

// Detect current branch from GitHub environment
const currentBranch =
claudeBranch ||
process.env.GITHUB_HEAD_REF ||
process.env.GITHUB_REF_NAME ||
"main";
defaultBranch;

// Get our GitHub MCP servers config
const ourMcpConfig = await prepareMcpConfig({
Expand Down
1 change: 1 addition & 0 deletions test/mockContext.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ const defaultRepository = {
owner: "test-owner",
repo: "test-repo",
full_name: "test-owner/test-repo",
default_branch: "main",
};

type MockContextOverrides = Omit<Partial<ParsedGitHubContext>, "inputs"> & {
Expand Down
56 changes: 55 additions & 1 deletion test/modes/agent.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,7 @@ describe("Agent Mode", () => {
expect(result.claudeArgs).toBe("--model claude-sonnet-4 --max-turns 10");
expect(result.claudeArgs).not.toContain("--mcp-config");

// Verify return structure - should use "main" as fallback when no env vars set
// Verify return structure - should fall back to repository.default_branch when no env vars set
expect(result).toEqual({
commentId: undefined,
branchInfo: {
Expand All @@ -108,6 +108,60 @@ describe("Agent Mode", () => {
process.env.GITHUB_REF_NAME = originalRefName;
});

test("prepare falls back to repository.default_branch when not 'main'", async () => {
const contextWithDevelop = createMockAutomationContext({
eventName: "workflow_dispatch",
repository: {
owner: "test-owner",
repo: "test-repo",
full_name: "test-owner/test-repo",
default_branch: "develop",
},
});
Comment on lines +113 to +120

@claude claude Bot Apr 1, 2026

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.

🟡 The new test "prepare falls back to repository.default_branch when not 'main'" does not save/restore CLAUDE_ARGS, unlike the adjacent test that explicitly manages it. If the preceding test fails at an assertion before its delete process.env.CLAUDE_ARGS cleanup (line 104), the leaked value would flow into prepareAgentMode and appear in claudeArgs output — though the test assertions only cover branchInfo fields so they would still pass.

Extended reasoning...

Analysis

The new test added at lines 113-163 of test/modes/agent.test.ts does not save or restore CLAUDE_ARGS, unlike the immediately preceding test "prepare passes through claude_args" which explicitly sets process.env.CLAUDE_ARGS = "--model claude-sonnet-4 --max-turns 10" and cleans it up with delete process.env.CLAUDE_ARGS at the end of the test body.

How the bug manifests

In src/modes/agent/index.ts, CLAUDE_ARGS is read unconditionally: const userClaudeArgs = process.env.CLAUDE_ARGS || "";. This value flows into the claudeArgs return field. If the preceding test fails mid-execution — for example, if one of its expect() assertions throws before reaching the cleanup block — CLAUDE_ARGS remains set in the process environment and is inherited by the new test.

Why existing code doesn't prevent it

The test cleanup for CLAUDE_ARGS in the preceding test is done inline in the test body (not in afterEach), so it is not guaranteed to run on failure. The afterEach hook only restores spies, not environment variables. This is a gap that the new test inherits rather than defending against.

Impact

If the leak occurs, prepareAgentMode will include --model claude-sonnet-4 --max-turns 10 in its claudeArgs output instead of an empty string. The new test only asserts on result.branchInfo.baseBranch and result.branchInfo.currentBranch, which are derived from context.repository.default_branch — completely unaffected by CLAUDE_ARGS. So the test passes even with leaked state, silently masking the unexpected behavior. This is a test quality/robustness issue, not a correctness issue for production code.

Addressing the refutation

The refutation correctly notes that (a) the prior test does clean up CLAUDE_ARGS under normal execution, and (b) the new test's assertions are unaffected by CLAUDE_ARGS. Both points are true. However, the fragility comes from the failure path: if the prior test throws before line 104, cleanup is skipped. The refutation does not address this failure-path scenario. The inconsistency with the surrounding cleanup pattern is also a readability/maintenance concern — future readers may not realize this test is not defensive against env leakage.

Proof of fragility

Step-by-step: (1) Prior test sets process.env.CLAUDE_ARGS = "--model claude-sonnet-4 --max-turns 10". (2) An assertion in the prior test fails, throwing before delete process.env.CLAUDE_ARGS at line 104. (3) The new test runs with CLAUDE_ARGS still set. (4) prepareAgentMode reads CLAUDE_ARGS and builds claudeArgs = "--model claude-sonnet-4 --max-turns 10". (5) The new test assertions expect(result.branchInfo.baseBranch).toBe("develop") and expect(result.branchInfo.currentBranch).toBe("develop") both pass since CLAUDE_ARGS does not affect branch info. (6) The test suite reports a passing test, but the behavior being tested has unexpected side effects that go undetected.

Fix

Add const originalClaudeArgs = process.env.CLAUDE_ARGS; before the test body and restore/delete it in the cleanup block alongside the other env var restorations, matching the pattern used in the same test for CLAUDE_BRANCH, GITHUB_HEAD_REF, and GITHUB_REF_NAME.


// Save and clear env vars that would otherwise override the fallback
const originalClaudeBranch = process.env.CLAUDE_BRANCH;
const originalHeadRef = process.env.GITHUB_HEAD_REF;
const originalRefName = process.env.GITHUB_REF_NAME;
delete process.env.CLAUDE_BRANCH;
delete process.env.GITHUB_HEAD_REF;
delete process.env.GITHUB_REF_NAME;

const mockOctokit = {
rest: {
users: {
getAuthenticated: mock(() =>
Promise.resolve({
data: { login: "test-user", id: 12345, type: "User" },
}),
),
getByUsername: mock(() =>
Promise.resolve({
data: { login: "test-user", id: 12345, type: "User" },
}),
),
},
},
} as any;

const result = await prepareAgentMode({
context: contextWithDevelop,
octokit: mockOctokit,
githubToken: "test-token",
});

expect(result.branchInfo.baseBranch).toBe("develop");
expect(result.branchInfo.currentBranch).toBe("develop");

// Restore env vars
if (originalClaudeBranch !== undefined)
process.env.CLAUDE_BRANCH = originalClaudeBranch;
if (originalHeadRef !== undefined)
process.env.GITHUB_HEAD_REF = originalHeadRef;
if (originalRefName !== undefined)
process.env.GITHUB_REF_NAME = originalRefName;
});

test("prepare rejects bot actors without allowed_bots", async () => {
const contextWithPrompts = createMockAutomationContext({
eventName: "workflow_dispatch",
Expand Down
Loading