Skip to content
Closed
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
5 changes: 5 additions & 0 deletions apps/server/src/git/GitManager.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ import * as VcsProcess from "../vcs/VcsProcess.ts";
import * as GitHubSourceControlProvider from "../sourceControl/GitHubSourceControlProvider.ts";
import * as SourceControlProviderRegistry from "../sourceControl/SourceControlProviderRegistry.ts";
import { makeGitManager } from "./GitManager.ts";
import { preferRealGitOnPathForTests } from "./testHelpers.ts";
import { ServerConfig } from "../config.ts";
import { ServerSettingsService } from "../serverSettings.ts";
import {
Expand Down Expand Up @@ -63,6 +64,8 @@ interface FakeGhScenario {
failWith?: GitHubCliError;
}

preferRealGitOnPathForTests();

function fakeGhOutput(stdout: string): VcsProcess.VcsProcessOutput {
return {
exitCode: ChildProcessSpawner.ExitCode(0),
Expand Down Expand Up @@ -910,6 +913,7 @@ it.layer(GitManagerTestLayer)("GitManager", (it) => {
},
hasUpstream: false,
aheadCount: 0,
aheadOfBaseCount: 0,
behindCount: 0,
aheadOfDefaultCount: 0,
pr: null,
Expand Down Expand Up @@ -940,6 +944,7 @@ it.layer(GitManagerTestLayer)("GitManager", (it) => {
},
hasUpstream: false,
aheadCount: 0,
aheadOfBaseCount: 0,
behindCount: 0,
aheadOfDefaultCount: 0,
pr: null,
Expand Down
2 changes: 2 additions & 0 deletions apps/server/src/git/GitManager.ts
Original file line number Diff line number Diff line change
Expand Up @@ -708,6 +708,7 @@ export const makeGitManager = Effect.fn("makeGitManager")(function* () {
workingTree: { files: [], insertions: 0, deletions: 0 },
hasUpstream: false,
aheadCount: 0,
aheadOfBaseCount: 0,
behindCount: 0,
aheadOfDefaultCount: 0,
} satisfies GitStatusDetails;
Expand Down Expand Up @@ -767,6 +768,7 @@ export const makeGitManager = Effect.fn("makeGitManager")(function* () {
return {
hasUpstream: details.hasUpstream,
aheadCount: details.aheadCount,
aheadOfBaseCount: details.aheadOfBaseCount,
behindCount: details.behindCount,
aheadOfDefaultCount: details.aheadOfDefaultCount,
pr,
Expand Down
1 change: 1 addition & 0 deletions apps/server/src/git/GitWorkflowService.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,7 @@ describe("GitWorkflowService", () => {
},
hasUpstream: false,
aheadCount: 0,
aheadOfBaseCount: 0,
behindCount: 0,
aheadOfDefaultCount: 0,
pr: null,
Expand Down
1 change: 1 addition & 0 deletions apps/server/src/git/GitWorkflowService.ts
Original file line number Diff line number Diff line change
Expand Up @@ -113,6 +113,7 @@ function nonRepositoryStatus(): VcsStatusResult {
...nonRepositoryLocalStatus(),
hasUpstream: false,
aheadCount: 0,
aheadOfBaseCount: 0,
behindCount: 0,
aheadOfDefaultCount: 0,
pr: null,
Expand Down
25 changes: 25 additions & 0 deletions apps/server/src/git/testHelpers.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
// @effect-diagnostics nodeBuiltinImport:off
import path from "node:path";
import { spawnSync } from "node:child_process";

export function preferRealGitOnPathForTests(): void {
process.env.GIT_CONFIG_GLOBAL = process.platform === "win32" ? "NUL" : "/dev/null";
process.env.GIT_CONFIG_NOSYSTEM = "1";

const result = spawnSync("git", ["ai", "git-path"], { encoding: "utf8" });
if (result.status !== 0) {
return;
}

const gitPath = result.stdout.trim();
if (gitPath.length === 0) {
return;
}

const gitDir = path.dirname(gitPath);
const pathEntries = (process.env.PATH ?? "")
.split(path.delimiter)
.filter((entry) => entry.length > 0 && entry !== gitDir);

process.env.PATH = [gitDir, ...pathEntries].join(path.delimiter);
}
Original file line number Diff line number Diff line change
Expand Up @@ -58,10 +58,13 @@ import { checkpointRefForThreadTurn } from "../../checkpointing/Utils.ts";
import { ServerConfig } from "../../config.ts";
import { WorkspaceEntriesLive } from "../../workspace/Layers/WorkspaceEntries.ts";
import { WorkspacePathsLive } from "../../workspace/Layers/WorkspacePaths.ts";
import { preferRealGitOnPathForTests } from "../../git/testHelpers.ts";

const asProjectId = (value: string): ProjectId => ProjectId.make(value);
const asTurnId = (value: string): TurnId => TurnId.make(value);

preferRealGitOnPathForTests();

type LegacyProviderRuntimeEvent = {
readonly type: string;
readonly eventId: EventId;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -260,6 +260,7 @@ describe("ProviderCommandReactor", () => {
},
hasUpstream: true,
aheadCount: 0,
aheadOfBaseCount: 0,
behindCount: 0,
pr: null,
}),
Expand Down
10 changes: 10 additions & 0 deletions apps/server/src/server.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2586,6 +2586,7 @@ it.layer(NodeServices.layer)("server router seam", (it) => {
Effect.succeed({
hasUpstream: true,
aheadCount: 0,
aheadOfBaseCount: 0,
behindCount: 0,
pr: null,
}),
Expand All @@ -2599,6 +2600,7 @@ it.layer(NodeServices.layer)("server router seam", (it) => {
workingTree: { files: [], insertions: 0, deletions: 0 },
hasUpstream: true,
aheadCount: 0,
aheadOfBaseCount: 0,
behindCount: 0,
pr: null,
}),
Expand Down Expand Up @@ -2860,6 +2862,7 @@ it.layer(NodeServices.layer)("server router seam", (it) => {
return {
hasUpstream: true,
aheadCount: 0,
aheadOfBaseCount: 0,
behindCount: 0,
pr: null,
};
Expand All @@ -2876,6 +2879,7 @@ it.layer(NodeServices.layer)("server router seam", (it) => {
workingTree: { files: [], insertions: 0, deletions: 0 },
hasUpstream: true,
aheadCount: 0,
aheadOfBaseCount: 0,
behindCount: 0,
pr: null,
};
Expand Down Expand Up @@ -2937,6 +2941,7 @@ it.layer(NodeServices.layer)("server router seam", (it) => {
return {
hasUpstream: true,
aheadCount: 0,
aheadOfBaseCount: 0,
behindCount: 0,
pr: null,
};
Expand All @@ -2953,6 +2958,7 @@ it.layer(NodeServices.layer)("server router seam", (it) => {
workingTree: { files: [], insertions: 0, deletions: 0 },
hasUpstream: true,
aheadCount: 0,
aheadOfBaseCount: 0,
behindCount: 0,
pr: null,
};
Expand Down Expand Up @@ -3008,6 +3014,7 @@ it.layer(NodeServices.layer)("server router seam", (it) => {
Effect.as({
hasUpstream: true,
aheadCount: 0,
aheadOfBaseCount: 0,
behindCount: 0,
pr: null,
}),
Expand Down Expand Up @@ -3054,6 +3061,7 @@ it.layer(NodeServices.layer)("server router seam", (it) => {
Effect.as({
hasUpstream: true,
aheadCount: 0,
aheadOfBaseCount: 0,
behindCount: 0,
pr: null,
}),
Expand Down Expand Up @@ -3135,6 +3143,7 @@ it.layer(NodeServices.layer)("server router seam", (it) => {
Effect.as({
hasUpstream: true,
aheadCount: 0,
aheadOfBaseCount: 0,
behindCount: 0,
pr: null,
}),
Expand Down Expand Up @@ -3827,6 +3836,7 @@ it.layer(NodeServices.layer)("server router seam", (it) => {
},
hasUpstream: true,
aheadCount: 0,
aheadOfBaseCount: 0,
behindCount: 0,
pr: null,
}),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -300,6 +300,7 @@ it.effect("publish succeeds with status remote_added when the local repo has no
workingTree: { files: [], insertions: 0, deletions: 0 },
hasUpstream: false,
aheadCount: 0,
aheadOfBaseCount: 0,
behindCount: 0,
aheadOfDefaultCount: 0,
}),
Expand Down
1 change: 1 addition & 0 deletions apps/server/src/vcs/GitVcsDriver.ts
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@ export interface GitStatusDetails {
workingTree: VcsStatusResult["workingTree"];
hasUpstream: boolean;
aheadCount: number;
aheadOfBaseCount: number;
behindCount: number;
aheadOfDefaultCount: number;
}
Expand Down
17 changes: 11 additions & 6 deletions apps/server/src/vcs/GitVcsDriverCore.ts
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@ const NON_REPOSITORY_STATUS_DETAILS = Object.freeze<GitVcsDriver.GitStatusDetail
workingTree: { files: [], insertions: 0, deletions: 0 },
hasUpstream: false,
aheadCount: 0,
aheadOfBaseCount: 0,
behindCount: 0,
aheadOfDefaultCount: 0,
});
Expand Down Expand Up @@ -1214,6 +1215,7 @@ export const makeGitVcsDriverCore = Effect.fn("makeGitVcsDriverCore")(function*
let refName: string | null = null;
let upstreamRef: string | null = null;
let aheadCount = 0;
let aheadOfBaseCount = 0;
let behindCount = 0;
let aheadOfDefaultCount = 0;
let hasWorkingTreeChanges = false;
Expand Down Expand Up @@ -1244,12 +1246,13 @@ export const makeGitVcsDriverCore = Effect.fn("makeGitVcsDriverCore")(function*
}
}

const fallbackAheadCount =
!upstreamRef && refName
? yield* computeAheadCountAgainstBase(cwd, refName).pipe(
Effect.catch(() => Effect.succeed(0)),
)
: null;
if (refName) {
aheadOfBaseCount = yield* computeAheadCountAgainstBase(cwd, refName).pipe(
Effect.catch(() => Effect.succeed(0)),
);
}

const fallbackAheadCount = !upstreamRef && refName ? aheadOfBaseCount : null;
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.

Duplicate git process spawn for same computation

Medium Severity

computeAheadCountAgainstBase(cwd, refName) is called unconditionally at line 1250 to compute aheadOfBaseCount, but the aheadOfDefaultCount fallback at line 1270 calls the same function with the same arguments again when fallbackAheadCount === null (i.e., when there IS an upstream). This spawns redundant git processes (resolveBaseBranchForNoUpstream + rev-list --count) on every status poll for non-default branches with an upstream — the exact scenario this PR targets. The value at line 1270 could simply reuse aheadOfBaseCount.

Additional Locations (1)
Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit bce2d06. Configure here.


if (fallbackAheadCount !== null) {
aheadCount = fallbackAheadCount;
Expand Down Expand Up @@ -1309,6 +1312,7 @@ export const makeGitVcsDriverCore = Effect.fn("makeGitVcsDriverCore")(function*
},
hasUpstream: upstreamRef !== null,
aheadCount,
aheadOfBaseCount,
behindCount,
aheadOfDefaultCount,
};
Expand Down Expand Up @@ -1341,6 +1345,7 @@ export const makeGitVcsDriverCore = Effect.fn("makeGitVcsDriverCore")(function*
workingTree: details.workingTree,
hasUpstream: details.hasUpstream,
aheadCount: details.aheadCount,
aheadOfBaseCount: details.aheadOfBaseCount,
behindCount: details.behindCount,
aheadOfDefaultCount: details.aheadOfDefaultCount,
pr: null,
Expand Down
1 change: 1 addition & 0 deletions apps/server/src/vcs/VcsStatusBroadcaster.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ const baseLocalStatus: VcsStatusLocalResult = {
const baseRemoteStatus: VcsStatusRemoteResult = {
hasUpstream: true,
aheadCount: 0,
aheadOfBaseCount: 0,
behindCount: 0,
pr: null,
};
Expand Down
1 change: 1 addition & 0 deletions apps/web/src/components/GitActionsControl.browser.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -128,6 +128,7 @@ vi.mock("~/lib/gitStatusState", () => ({
workingTree: { files: [], insertions: 0, deletions: 0 },
hasUpstream: true,
aheadCount: 1,
aheadOfBaseCount: 1,
behindCount: 0,
pr: null,
},
Expand Down
13 changes: 10 additions & 3 deletions apps/web/src/components/GitActionsControl.logic.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,9 @@ import {
} from "./GitActionsControl.logic";

function status(overrides: Partial<VcsStatusResult> = {}): VcsStatusResult {
const aheadOfBaseCount =
overrides.aheadOfBaseCount ?? overrides.aheadOfDefaultCount ?? overrides.aheadCount ?? 0;
const aheadOfDefaultCount = overrides.aheadOfDefaultCount ?? aheadOfBaseCount;
return {
isRepo: true,
hasPrimaryRemote: true,
Expand All @@ -25,6 +28,8 @@ function status(overrides: Partial<VcsStatusResult> = {}): VcsStatusResult {
},
hasUpstream: true,
aheadCount: 0,
aheadOfBaseCount,
aheadOfDefaultCount,
behindCount: 0,
pr: null,
...overrides,
Expand Down Expand Up @@ -214,7 +219,10 @@ describe("when: ref is clean, ahead, and has an open PR", () => {

describe("when: ref is clean, ahead, and has no open PR", () => {
it("resolveQuickAction pushes and creates a PR", () => {
const quick = resolveQuickAction(status({ aheadCount: 2, pr: null }), false);
const quick = resolveQuickAction(
status({ aheadCount: 2, aheadOfBaseCount: 2, pr: null }),
false,
);
assert.deepInclude(quick, {
kind: "run_action",
action: "create_pr",
Expand All @@ -223,7 +231,7 @@ describe("when: ref is clean, ahead, and has no open PR", () => {
});

it("buildMenuItems enables push and create PR, with commit disabled", () => {
const items = buildMenuItems(status({ aheadCount: 2, pr: null }), false);
const items = buildMenuItems(status({ aheadCount: 2, aheadOfBaseCount: 2, pr: null }), false);
assert.deepEqual(items, [
{
id: "commit",
Expand Down Expand Up @@ -299,7 +307,6 @@ describe("when: ref is clean, up to date, and has no open PR", () => {
const items = buildMenuItems(syncedFeature, false);
assert.equal(items.find((item) => item.id === "pr")?.disabled, false);
});

it("resolveQuickAction returns disabled no-action state", () => {
const quick = resolveQuickAction(
status({ aheadCount: 0, behindCount: 0, hasWorkingTreeChanges: false, pr: null }),
Expand Down
16 changes: 8 additions & 8 deletions apps/web/src/components/GitActionsControl.logic.ts
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,7 @@ export function buildMenuItems(
const hasChanges = gitStatus.hasWorkingTreeChanges;
const hasOpenPr = gitStatus.pr?.state === "open";
const isBehind = gitStatus.behindCount > 0;
const hasDefaultBranchDelta = (gitStatus.aheadOfDefaultCount ?? gitStatus.aheadCount) > 0;
const isAheadOfBase = gitStatus.aheadOfBaseCount > 0;
const canPushWithoutUpstream = hasPrimaryRemote && !gitStatus.hasUpstream;
const canCommit = !isBusy && hasChanges;
const canPush =
Expand All @@ -117,7 +117,7 @@ export function buildMenuItems(
hasBranch &&
!hasChanges &&
!hasOpenPr &&
hasDefaultBranchDelta &&
isAheadOfBase &&
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.

Missing default branch guard in menu items

Low Severity

canCreatePr in buildMenuItems uses isAheadOfBase without checking !gitStatus.isDefaultRef. The old hasDefaultBranchDelta was implicitly zero on the default branch because aheadOfDefaultCount was only computed for non-default branches. The new aheadOfBaseCount is computed unconditionally and can be positive on the default branch if an alternative base branch candidate (like "master") exists, incorrectly enabling the "Create PR" menu item on the default branch.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit bce2d06. Configure here.

!isBehind &&
(gitStatus.hasUpstream || canPushWithoutUpstream);
const canOpenPr = !isBusy && hasOpenPr;
Expand Down Expand Up @@ -187,7 +187,7 @@ export function resolveQuickAction(
const hasChanges = gitStatus.hasWorkingTreeChanges;
const hasOpenPr = gitStatus.pr?.state === "open";
const isAhead = gitStatus.aheadCount > 0;
const hasDefaultBranchDelta = (gitStatus.aheadOfDefaultCount ?? gitStatus.aheadCount) > 0;
const isAheadOfBase = gitStatus.aheadOfBaseCount > 0;
const isBehind = gitStatus.behindCount > 0;
const isDiverged = isAhead && isBehind;
const terminology = resolveChangeRequestTerminology(gitStatus);
Expand Down Expand Up @@ -288,11 +288,7 @@ export function resolveQuickAction(
};
}

if (hasOpenPr && gitStatus.hasUpstream) {
return { label: `View ${terminology.shortLabel}`, disabled: false, kind: "open_pr" };
}

if (hasDefaultBranchDelta && !isDefaultRef) {
if (isAheadOfBase && !hasOpenPr && !isDefaultRef) {
return {
label: `Create ${terminology.shortLabel}`,
disabled: false,
Expand All @@ -301,6 +297,10 @@ export function resolveQuickAction(
};
}

if (hasOpenPr && gitStatus.hasUpstream) {
return { label: `View ${terminology.shortLabel}`, disabled: false, kind: "open_pr" };
}

return {
label: "Commit",
disabled: true,
Expand Down
5 changes: 3 additions & 2 deletions apps/web/src/components/GitActionsControl.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -250,6 +250,7 @@ function getMenuActionDisabledReason({
const hasChanges = gitStatus.hasWorkingTreeChanges;
const hasOpenPr = gitStatus.pr?.state === "open";
const isAhead = gitStatus.aheadCount > 0;
const isAheadOfBase = gitStatus.aheadOfBaseCount > 0;
const isBehind = gitStatus.behindCount > 0;
const terminology = getSourceControlPresentation(gitStatus.sourceControlProvider).terminology;

Expand Down Expand Up @@ -291,8 +292,8 @@ function getMenuActionDisabledReason({
if (!gitStatus.hasUpstream && !hasPrimaryRemote) {
return `Add an "origin" remote before creating a ${terminology.singular}.`;
}
if (!isAhead) {
return `No local commits to include in a ${terminology.singular}.`;
if (!isAheadOfBase) {
return `No commits on this ref to include in a ${terminology.singular}.`;
}
if (isBehind) {
return `Branch is behind upstream. Pull/rebase before creating a ${terminology.singular}.`;
Expand Down
Loading
Loading