-
Notifications
You must be signed in to change notification settings - Fork 1.2k
fix: fetch remote base branch before PR range context to prevent stale diffs #1519
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.
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -902,7 +902,45 @@ export const makeGitManager = Effect.fn("makeGitManager")(function* () { | |
| } | ||
|
|
||
| const baseBranch = yield* resolveBaseBranch(cwd, branch, details.upstreamRef, headContext); | ||
| const rangeContext = yield* gitCore.readRangeContext(cwd, baseBranch); | ||
|
|
||
| // Fetch the remote tracking ref for the base branch so that the range | ||
| // context reflects any upstream changes (e.g. previously merged PRs). | ||
| // Without this fetch the local ref can be stale, causing readRangeContext | ||
| // to include commits/diffs that were already merged upstream (#1487). | ||
| const rangeRef = yield* Effect.gen(function* () { | ||
| // If baseBranch already contains a remote prefix, use it as-is. | ||
| if (baseBranch.includes("/")) { | ||
| return baseBranch; | ||
| } | ||
| // Try to fetch the base branch from the remote and use the remote | ||
| // tracking ref for range computation. Prefer the head context's | ||
| // remote when available (e.g. fork remotes) and fall back to "origin". | ||
| const remoteName = headContext.remoteName ?? "origin"; | ||
| const remoteRef = `${remoteName}/${baseBranch}`; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fork workflows fetch base branch from wrong remoteMedium Severity In cross-repository (fork) workflows, |
||
| yield* gitCore.execute({ | ||
| operation: "runPrStep.fetchBaseBranch", | ||
| cwd, | ||
| args: [ | ||
| "fetch", | ||
| "--quiet", | ||
| "--no-tags", | ||
| remoteName, | ||
| `+refs/heads/${baseBranch}:refs/remotes/${remoteRef}`, | ||
| ], | ||
| allowNonZeroExit: true, | ||
| timeoutMs: 30_000, | ||
| }); | ||
| // Verify the remote ref exists after fetch; if it does, prefer it. | ||
| const verifyResult = yield* gitCore.execute({ | ||
| operation: "runPrStep.verifyRemoteRef", | ||
| cwd, | ||
| args: ["rev-parse", "--verify", remoteRef], | ||
| allowNonZeroExit: true, | ||
| }); | ||
| return verifyResult.code === 0 ? remoteRef : baseBranch; | ||
| }).pipe(Effect.catch(() => Effect.succeed(baseBranch))); | ||
|
|
||
| const rangeContext = yield* gitCore.readRangeContext(cwd, rangeRef); | ||
|
|
||
| const generated = yield* textGeneration.generatePrContent({ | ||
| cwd, | ||
|
|
||


There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Slash check misidentifies local branches as remote refs
Medium Severity
The
baseBranch.includes("/")check is intended to detect remote-prefixed refs likeorigin/main, but it incorrectly matches any branch name containing a slash (e.g.,release/1.0,feature/main,hotfix/critical). These are common local branch naming conventions. WhenresolveBaseBranchreturns such a name — via git config,extractBranchFromRef, orgetDefaultBranch— the code skips the remote fetch and uses the potentially stale local ref, defeating the fix for issue #1487 in those cases.