Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
452 changes: 365 additions & 87 deletions apps/server/src/git/GitManager.test.ts

Large diffs are not rendered by default.

69 changes: 45 additions & 24 deletions apps/server/src/git/GitManager.ts
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,8 @@ import {
import {
detectSourceControlProviderFromGitRemoteUrl,
mergeGitStatusParts,
normalizeGitRemoteUrl,
parseGitHubRepositoryNameWithOwnerFromRemoteUrl,
resolveAutoFeatureBranchName,
sanitizeBranchFragment,
sanitizeFeatureBranchName,
Expand Down Expand Up @@ -93,6 +95,7 @@ const COMMIT_TIMEOUT_MS = 10 * 60_000;
const MAX_PROGRESS_TEXT_LENGTH = 500;
const SHORT_SHA_LENGTH = 7;
const TOAST_DESCRIPTION_MAX = 72;
const OPEN_PULL_REQUEST_CANDIDATE_LIMIT = 100;
const STATUS_RESULT_CACHE_TTL = Duration.seconds(1);
const STATUS_RESULT_CACHE_CAPACITY = 2_048;
type StripProgressContext<T> = T extends any ? Omit<T, "actionId" | "cwd" | "action"> : never;
Expand Down Expand Up @@ -187,20 +190,6 @@ function resolvePullRequestWorktreeLocalBranchName(
return `t3code/pr-${pullRequest.number}/${suffix}`;
}

function parseGitHubRepositoryNameWithOwnerFromRemoteUrl(url: string | null): string | null {
const trimmed = url?.trim() ?? "";
if (trimmed.length === 0) {
return null;
}

const match =
/^(?:git@github\.com:|ssh:\/\/git@github\.com\/|https:\/\/github\.com\/|git:\/\/github\.com\/)([^/\s]+\/[^/\s]+?)(?:\.git)?\/?$/i.exec(
trimmed,
);
const repositoryNameWithOwner = match?.[1]?.trim() ?? "";
return repositoryNameWithOwner.length > 0 ? repositoryNameWithOwner : null;
}

function parseRepositoryOwnerLogin(nameWithOwner: string | null): string | null {
const trimmed = nameWithOwner?.trim() ?? "";
if (trimmed.length === 0) {
Expand Down Expand Up @@ -278,17 +267,10 @@ function matchesBranchHeadContext(
if ((expectedHeadRepository || expectedHeadOwner) && !prHeadRepository && !prHeadOwner) {
return false;
}
if (expectedHeadRepository && prHeadRepository && expectedHeadRepository !== prHeadRepository) {
} else if (pr.isCrossRepository === true) {
if (!expectedHeadRepository || !prHeadRepository) {
return false;
Comment on lines +270 to 272

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Match GHES fork PRs before creating duplicates

For self-hosted GitHub fork checkouts, resolveRemoteRepositoryContext leaves expectedHeadRepository empty because parseGitHubRepositoryNameWithOwnerFromRemoteUrl only recognizes github.com remotes, while the new upstream-targeted listPullRequests can still return isCrossRepository: true PRs from the fork. This branch rejects those matches whenever the expected repo is missing, so create_pr on an existing GHES upstream PR will miss it and proceed to gh pr create (or, after creation, fail to recover the PR URL/number).

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in 849a0d275 — GitHub Enterprise remote URLs now resolve their owner/repository identity and existing cross-repository PRs are matched without duplicate creation.

}
if (expectedHeadOwner && prHeadOwner && expectedHeadOwner !== prHeadOwner) {
return false;
}
return true;
}

if (pr.isCrossRepository === true) {
return false;
}
if (expectedHeadRepository && prHeadRepository && expectedHeadRepository !== prHeadRepository) {
return false;
Expand Down Expand Up @@ -937,7 +919,7 @@ export const make = Effect.gen(function* () {
cwd,
headSelector,
state: "open",
limit: 1,
limit: OPEN_PULL_REQUEST_CANDIDATE_LIMIT,
});
const normalizedPullRequests = pullRequests.map(toPullRequestInfo);

Expand Down Expand Up @@ -1119,6 +1101,45 @@ export const make = Effect.gen(function* () {
cwd: string,
baseBranch: string,
) {
const provider = yield* sourceControlProvider(cwd);
const targetCloneUrls = provider.getTargetRepositoryCloneUrls
? yield* provider.getTargetRepositoryCloneUrls({ cwd }).pipe(Effect.orElseSucceed(() => null))
: null;
if (targetCloneUrls) {
const originUrl = yield* readConfigValueNullable(cwd, "remote.origin.url");
const targetMatchesOrigin =
originUrl !== null &&
[targetCloneUrls.url, targetCloneUrls.sshUrl].some(
(targetUrl) => normalizeGitRemoteUrl(targetUrl) === normalizeGitRemoteUrl(originUrl),
);
const resolveTargetBaseRangeRef = Effect.gen(function* () {
const targetUrl = shouldPreferSshRemote(originUrl)
? targetCloneUrls.sshUrl
: targetCloneUrls.url;
const remoteName = yield* gitCore.ensureRemote({
cwd,
preferredName: "upstream",
url: targetUrl,
});
yield* gitCore.fetchRemoteTrackingBranch({
cwd,
remoteName,
remoteBranch: baseBranch,
});
return yield* gitCore
.resolveRemoteTrackingCommit({
cwd,
refName: baseBranch,
fallbackRemoteName: remoteName,
})
.pipe(Effect.map((resolved) => resolved.commitSha));
});
const targetBaseRangeRef = targetMatchesOrigin

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.

🟡 Medium git/GitManager.ts:1137

When targetMatchesOrigin is false (fork workflow), resolveBaseRangeRef lets failures from adding/fetching the upstream remote abort the entire operation — the primary-remote fallback below is never reached. In the matching-origin case the same fetch is wrapped with Effect.orElseSucceed(() => null), so a transient network or auth failure degrades gracefully, but in the fork case a similar failure propagates and prevents PR creation even though the fallback could still resolve the base range. Consider applying the same Effect.orElseSucceed(() => null) wrapper to resolveTargetBaseRangeRef in the non-matching branch.

🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/git/GitManager.ts around line 1137:

When `targetMatchesOrigin` is `false` (fork workflow), `resolveBaseRangeRef` lets failures from adding/fetching the `upstream` remote abort the entire operation — the primary-remote fallback below is never reached. In the matching-origin case the same fetch is wrapped with `Effect.orElseSucceed(() => null)`, so a transient network or auth failure degrades gracefully, but in the fork case a similar failure propagates and prevents PR creation even though the fallback could still resolve the base range. Consider applying the same `Effect.orElseSucceed(() => null)` wrapper to `resolveTargetBaseRangeRef` in the non-matching branch.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in a6246954b — the regression test now makes the intended fork behavior explicit: if the upstream base cannot be fetched, PR content generation stops rather than silently using a stale fork origin. Applying the suggested fallback would reintroduce the incorrect-base bug identified in the preceding review.

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.

Sorry, I'm unable to act on this request because you do not have permissions within this repository.

? yield* resolveTargetBaseRangeRef.pipe(Effect.orElseSucceed(() => null))
: yield* resolveTargetBaseRangeRef;

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.

Same-repo check misses SSH aliases

Medium Severity

targetMatchesOrigin equates remotes via full normalizeGitRemoteUrl keys, so an SSH config host alias on origin (for example git@github.com-work:owner/repo) never matches GitHub’s canonical clone URLs. That same-repo checkout is treated like a fork, and a failed fetch of the canonical remote no longer falls back to the local tracking ref—create_pr fails instead.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 2edec75. Configure here.

Comment on lines +1137 to +1139

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Fall back for same-repo alternate remotes

For same-repo GitHub checkouts whose origin is a valid alternate URL, such as ssh://git@ssh.github.com:443/owner/repo.git or a corporate mirror/rewrite, targetMatchesOrigin is false because the comparison only recognizes the two clone URLs returned by gh repo view. This branch then treats a failed fetch of the canonical target URL as fatal instead of falling back to the existing origin/<base> tracking ref, so create_pr can abort before content generation even though the configured origin is usable; only fork-parent targets need this strict no-fallback behavior.

Useful? React with 👍 / 👎.

if (targetBaseRangeRef) return targetBaseRangeRef;
Comment thread
cursor[bot] marked this conversation as resolved.
}

const remoteName = yield* gitCore
.resolvePrimaryRemoteName(cwd)
.pipe(Effect.orElseSucceed(() => null));
Expand Down
Loading
Loading