Introduce pluggable VCS driver foundation#2435
Merged
Merged
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
github-actions
Bot
added
vouch:trusted
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Autofix Details
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Execute method silently drops env field breaking checkpoints
- Added
envtogitCommand's options type and forwardedinput.envfrom theexecutemethod togitCommand, so CheckpointStore'sGIT_INDEX_FILEand author/committer overrides are now properly passed through to the spawned git process.
- Added
Or push these changes by commenting:
@cursor push 6506f41f19
Preview (6506f41f19)
diff --git a/apps/server/src/vcs/Layers/GitVcsDriver.ts b/apps/server/src/vcs/Layers/GitVcsDriver.ts
--- a/apps/server/src/vcs/Layers/GitVcsDriver.ts
+++ b/apps/server/src/vcs/Layers/GitVcsDriver.ts
@@ -69,6 +69,7 @@
args: ReadonlyArray<string>,
options?: {
readonly stdin?: string;
+ readonly env?: NodeJS.ProcessEnv;
readonly allowNonZeroExit?: boolean;
readonly timeoutMs?: number;
readonly maxOutputBytes?: number;
@@ -81,6 +82,7 @@
args,
cwd,
...(options?.stdin !== undefined ? { stdin: options.stdin } : {}),
+ ...(options?.env !== undefined ? { env: options.env } : {}),
...(options?.allowNonZeroExit !== undefined
? { allowNonZeroExit: options.allowNonZeroExit }
: {}),
@@ -118,6 +120,7 @@
const execute: VcsDriverShape["execute"] = (input) =>
gitCommand(process, input.operation, input.cwd, input.args, {
...(input.stdin !== undefined ? { stdin: input.stdin } : {}),
+ ...(input.env !== undefined ? { env: input.env } : {}),
...(input.allowNonZeroExit !== undefined ? { allowNonZeroExit: input.allowNonZeroExit } : {}),
...(input.timeoutMs !== undefined ? { timeoutMs: input.timeoutMs } : {}),
...(input.maxOutputBytes !== undefined ? { maxOutputBytes: input.maxOutputBytes } : {}),You can send follow-ups to the cloud agent here.
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
Contributor
ApprovabilityVerdict: Needs human review 1 blocking correctness issue found. Diff is too large for automated approval analysis. A human reviewer should evaluate this PR. You can customize Macroscope's approvability policy. Learn more. |
Contributor
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 2 total unresolved issues (including 1 from previous review).
Autofix Details
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: VcsProcessExitError misused for non-exit validation failures
- Replaced all three misuses of VcsProcessExitError (with fabricated exitCode values) with CheckpointInvariantError, which correctly represents application-level invariant violations rather than process exit failures.
Or push these changes by commenting:
@cursor push 9cbdcf4648
Preview (9cbdcf4648)
diff --git a/apps/server/src/checkpointing/Layers/CheckpointStore.ts b/apps/server/src/checkpointing/Layers/CheckpointStore.ts
--- a/apps/server/src/checkpointing/Layers/CheckpointStore.ts
+++ b/apps/server/src/checkpointing/Layers/CheckpointStore.ts
@@ -14,7 +14,6 @@
import { Effect, Layer, FileSystem, Path } from "effect";
import { CheckpointInvariantError } from "../Errors.ts";
-import { VcsProcessExitError } from "@t3tools/contracts";
import { VcsDriver } from "../../vcs/VcsDriver.ts";
import { CheckpointStore, type CheckpointStoreShape } from "../Services/CheckpointStore.ts";
import { CheckpointRef } from "@t3tools/contracts";
@@ -128,11 +127,8 @@
});
const treeOid = writeTreeResult.stdout.trim();
if (treeOid.length === 0) {
- return yield* new VcsProcessExitError({
+ return yield* new CheckpointInvariantError({
operation,
- command: "git write-tree",
- cwd: input.cwd,
- exitCode: 0,
detail: "git write-tree returned an empty tree oid.",
});
}
@@ -146,11 +142,8 @@
});
const commitOid = commitTreeResult.stdout.trim();
if (commitOid.length === 0) {
- return yield* new VcsProcessExitError({
+ return yield* new CheckpointInvariantError({
operation,
- command: "git commit-tree",
- cwd: input.cwd,
- exitCode: 0,
detail: "git commit-tree returned an empty commit oid.",
});
}
@@ -234,11 +227,8 @@
}
if (!fromCommitOid || !toCommitOid) {
- return yield* new VcsProcessExitError({
+ return yield* new CheckpointInvariantError({
operation,
- command: "git diff",
- cwd: input.cwd,
- exitCode: 1,
detail: "Checkpoint ref is unavailable for diff operation.",
});
}You can send follow-ups to the cloud agent here.
Contributor
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Autofix Details
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Uncached repository detection on every checkpoint git command
- Added an in-memory Map cache in VcsDriverRegistry.resolve() keyed on cwd+requestedKind so that detectRepository() (which spawns 3 git processes) is only called once per unique path, eliminating ~18 unnecessary process spawns per checkpoint capture.
Or push these changes by commenting:
@cursor push 0f019fda3d
Preview (0f019fda3d)
diff --git a/apps/server/src/vcs/VcsDriverRegistry.ts b/apps/server/src/vcs/VcsDriverRegistry.ts
--- a/apps/server/src/vcs/VcsDriverRegistry.ts
+++ b/apps/server/src/vcs/VcsDriverRegistry.ts
@@ -40,6 +40,8 @@
git,
};
+ const resolveCache = new Map<string, VcsDriverHandle>();
+
const detectWithDriver = Effect.fn("VcsDriverRegistry.detectWithDriver")(function* (
kind: VcsDriverKind,
driver: VcsDriverShape,
@@ -78,12 +80,19 @@
const resolve: VcsDriverRegistryShape["resolve"] = Effect.fn("VcsDriverRegistry.resolve")(
function* (input) {
+ const requestedKind = input.requestedKind ?? "auto";
+ const cacheKey = `${input.cwd}\0${requestedKind}`;
+ const cached = resolveCache.get(cacheKey);
+ if (cached) {
+ return cached;
+ }
+
const detected = yield* detect(input);
if (detected) {
+ resolveCache.set(cacheKey, detected);
return detected;
}
- const requestedKind = input.requestedKind ?? "auto";
return yield* unsupported(
"VcsDriverRegistry.resolve",
requestedKind === "auto" ? "unknown" : requestedKind,You can send follow-ups to the cloud agent here.
Contributor
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Autofix Details
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: VcsStatusBroadcaster drops CWD symlink normalization from old code
- Restored the normalizeCwd helper (using realpathSync.native) and added calls in getStatus, refreshLocalStatus, refreshStatus, and streamStatus to resolve symlinks before cache lookups and PubSub filtering, matching the old GitStatusBroadcasterLive behavior.
Or push these changes by commenting:
@cursor push 1c1db2f34e
Preview (1c1db2f34e)
diff --git a/apps/server/src/vcs/VcsStatusBroadcaster.ts b/apps/server/src/vcs/VcsStatusBroadcaster.ts
--- a/apps/server/src/vcs/VcsStatusBroadcaster.ts
+++ b/apps/server/src/vcs/VcsStatusBroadcaster.ts
@@ -1,3 +1,5 @@
+import { realpathSync } from "node:fs";
+
import {
Duration,
Effect,
@@ -63,6 +65,14 @@
VcsStatusBroadcasterShape
>()("t3/vcs/VcsStatusBroadcaster") {}
+function normalizeCwd(cwd: string): string {
+ try {
+ return realpathSync.native(cwd);
+ } catch {
+ return cwd;
+ }
+}
+
function fingerprintStatusPart(status: unknown): string {
return JSON.stringify(status);
}
@@ -188,16 +198,18 @@
const getStatus: VcsStatusBroadcasterShape["getStatus"] = Effect.fn(
"VcsStatusBroadcaster.getStatus",
)(function* (input) {
+ const cwd = normalizeCwd(input.cwd);
const [local, remote] = yield* Effect.all([
- getOrLoadLocalStatus(input.cwd),
- getOrLoadRemoteStatus(input.cwd),
+ getOrLoadLocalStatus(cwd),
+ getOrLoadRemoteStatus(cwd),
]);
return mergeGitStatusParts(local, remote);
});
const refreshLocalStatus: VcsStatusBroadcasterShape["refreshLocalStatus"] = Effect.fn(
"VcsStatusBroadcaster.refreshLocalStatus",
- )(function* (cwd) {
+ )(function* (rawCwd) {
+ const cwd = normalizeCwd(rawCwd);
yield* workflow.invalidateLocalStatus(cwd);
const local = yield* workflow.localStatus({ cwd });
return yield* updateCachedLocalStatus(cwd, local, { publish: true });
@@ -213,7 +225,8 @@
const refreshStatus: VcsStatusBroadcasterShape["refreshStatus"] = Effect.fn(
"VcsStatusBroadcaster.refreshStatus",
- )(function* (cwd) {
+ )(function* (rawCwd) {
+ const cwd = normalizeCwd(rawCwd);
const [local, remote] = yield* Effect.all([
refreshLocalStatus(cwd),
refreshRemoteStatus(cwd),
@@ -299,12 +312,13 @@
const streamStatus: VcsStatusBroadcasterShape["streamStatus"] = (input) =>
Stream.unwrap(
Effect.gen(function* () {
+ const cwd = normalizeCwd(input.cwd);
const subscription = yield* PubSub.subscribe(changesPubSub);
- const initialLocal = yield* getOrLoadLocalStatus(input.cwd);
- const initialRemote = (yield* getCachedStatus(input.cwd))?.remote?.value ?? null;
- yield* retainRemotePoller(input.cwd);
+ const initialLocal = yield* getOrLoadLocalStatus(cwd);
+ const initialRemote = (yield* getCachedStatus(cwd))?.remote?.value ?? null;
+ yield* retainRemotePoller(cwd);
- const release = releaseRemotePoller(input.cwd).pipe(Effect.ignore, Effect.asVoid);
+ const release = releaseRemotePoller(cwd).pipe(Effect.ignore, Effect.asVoid);
return Stream.concat(
Stream.make({
@@ -313,7 +327,7 @@
remote: initialRemote,
}),
Stream.fromSubscription(subscription).pipe(
- Stream.filter((event) => event.cwd === input.cwd),
+ Stream.filter((event) => event.cwd === cwd),
Stream.map((event) => event.event),
),
).pipe(Stream.ensuring(release));You can send follow-ups to the cloud agent here.
Contributor
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Autofix Details
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Custom env silently dropped in VCS driver execute
- Added env to the gitCommand options signature and forwarded input.env from the execute method through gitCommand to process.run, so GIT_INDEX_FILE and other environment variables are now properly passed to the spawned git processes.
Or push these changes by commenting:
@cursor push e4920a3ee7
Preview (e4920a3ee7)
diff --git a/apps/server/src/vcs/GitVcsDriver.ts b/apps/server/src/vcs/GitVcsDriver.ts
--- a/apps/server/src/vcs/GitVcsDriver.ts
+++ b/apps/server/src/vcs/GitVcsDriver.ts
@@ -257,6 +257,7 @@
args: ReadonlyArray<string>,
options?: {
readonly stdin?: string;
+ readonly env?: NodeJS.ProcessEnv;
readonly allowNonZeroExit?: boolean;
readonly timeoutMs?: number;
readonly maxOutputBytes?: number;
@@ -269,6 +270,7 @@
args,
cwd,
...(options?.stdin !== undefined ? { stdin: options.stdin } : {}),
+ ...(options?.env !== undefined ? { env: options.env } : {}),
...(options?.allowNonZeroExit !== undefined
? { allowNonZeroExit: options.allowNonZeroExit }
: {}),
@@ -305,6 +307,7 @@
const execute: VcsDriverShape["execute"] = (input) =>
gitCommand(process, input.operation, input.cwd, input.args, {
...(input.stdin !== undefined ? { stdin: input.stdin } : {}),
+ ...(input.env !== undefined ? { env: input.env } : {}),
...(input.allowNonZeroExit !== undefined ? { allowNonZeroExit: input.allowNonZeroExit } : {}),
...(input.timeoutMs !== undefined ? { timeoutMs: input.timeoutMs } : {}),
...(input.maxOutputBytes !== undefined ? { maxOutputBytes: input.maxOutputBytes } : {}),You can send follow-ups to the cloud agent here.
Contributor
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Autofix Details
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Dual exit code field names create fragile inconsistency
- Renamed
ExecuteGitResult.codetoExecuteGitResult.exitCodeto matchVcsProcessOutput.exitCode, unifying the exit code field name across both execute signatures and updating all 30+ references in GitVcsDriverCore.ts, SourceControlProviderRegistry.ts, and test files.
- Renamed
Or push these changes by commenting:
@cursor push 5f17833408
Preview (5f17833408)
diff --git a/apps/server/src/git/GitManager.test.ts b/apps/server/src/git/GitManager.test.ts
--- a/apps/server/src/git/GitManager.test.ts
+++ b/apps/server/src/git/GitManager.test.ts
@@ -224,7 +224,7 @@
allowNonZeroExit,
});
return {
- code: result.code,
+ code: result.exitCode,
stdout: result.stdout,
stderr: result.stderr,
};
diff --git a/apps/server/src/sourceControl/SourceControlProviderRegistry.test.ts b/apps/server/src/sourceControl/SourceControlProviderRegistry.test.ts
--- a/apps/server/src/sourceControl/SourceControlProviderRegistry.test.ts
+++ b/apps/server/src/sourceControl/SourceControlProviderRegistry.test.ts
@@ -8,7 +8,7 @@
const processResult = (stdout: string) => ({
stdout,
stderr: "",
- code: 0,
+ exitCode: 0,
signal: null,
timedOut: false,
stdoutTruncated: false,
diff --git a/apps/server/src/sourceControl/SourceControlProviderRegistry.ts b/apps/server/src/sourceControl/SourceControlProviderRegistry.ts
--- a/apps/server/src/sourceControl/SourceControlProviderRegistry.ts
+++ b/apps/server/src/sourceControl/SourceControlProviderRegistry.ts
@@ -89,7 +89,7 @@
})
.pipe(
Effect.map((result) =>
- result.code === 0 ? firstRemoteUrlFromVerboseOutput(result.stdout) : null,
+ result.exitCode === 0 ? firstRemoteUrlFromVerboseOutput(result.stdout) : null,
),
Effect.mapError((error) => providerDetectionError("detectProvider", cwd, error)),
));
diff --git a/apps/server/src/vcs/GitVcsDriver.ts b/apps/server/src/vcs/GitVcsDriver.ts
--- a/apps/server/src/vcs/GitVcsDriver.ts
+++ b/apps/server/src/vcs/GitVcsDriver.ts
@@ -35,7 +35,7 @@
}
export interface ExecuteGitResult {
- readonly code: number;
+ readonly exitCode: number;
readonly stdout: string;
readonly stderr: string;
readonly stdoutTruncated: boolean;
diff --git a/apps/server/src/vcs/GitVcsDriverCore.ts b/apps/server/src/vcs/GitVcsDriverCore.ts
--- a/apps/server/src/vcs/GitVcsDriverCore.ts
+++ b/apps/server/src/vcs/GitVcsDriverCore.ts
@@ -693,7 +693,7 @@
}
return {
- code: exitCode,
+ exitCode,
stdout: stdout.text,
stderr: stderr.text,
stdoutTruncated: stdout.truncated,
@@ -761,7 +761,7 @@
...(options.progress ? { progress: options.progress } : {}),
}).pipe(
Effect.flatMap((result) => {
- if (options.allowNonZeroExit || result.code === 0) {
+ if (options.allowNonZeroExit || result.exitCode === 0) {
return Effect.succeed(result);
}
const stderr = result.stderr.trim();
@@ -778,7 +778,7 @@
operation,
cwd,
args,
- `${commandLabel(args)} failed: code=${result.code ?? "null"}`,
+ `${commandLabel(args)} failed: code=${result.exitCode ?? "null"}`,
),
);
}),
@@ -823,7 +823,7 @@
allowNonZeroExit: true,
timeoutMs: 5_000,
},
- ).pipe(Effect.map((result) => result.code === 0));
+ ).pipe(Effect.map((result) => result.exitCode === 0));
const resolveAvailableBranchName = Effect.fn("resolveAvailableBranchName")(function* (
cwd: string,
@@ -939,7 +939,7 @@
{ allowNonZeroExit: true },
).pipe(
Effect.map((result) => {
- if (result.code !== 0) {
+ if (result.exitCode !== 0) {
return null;
}
return parseDefaultBranchFromRemoteHeadRef(result.stdout, remoteName);
@@ -958,12 +958,12 @@
{
allowNonZeroExit: true,
},
- ).pipe(Effect.map((result) => result.code === 0));
+ ).pipe(Effect.map((result) => result.exitCode === 0));
const originRemoteExists = (cwd: string): Effect.Effect<boolean, GitCommandError> =>
executeGit("GitVcsDriver.originRemoteExists", cwd, ["remote", "get-url", "origin"], {
allowNonZeroExit: true,
- }).pipe(Effect.map((result) => result.code === 0));
+ }).pipe(Effect.map((result) => result.exitCode === 0));
const listRemoteNames = (cwd: string): Effect.Effect<ReadonlyArray<string>, GitCommandError> =>
runGitStdout("GitVcsDriver.listRemoteNames", cwd, ["remote"]).pipe(
@@ -1115,7 +1115,7 @@
["rev-list", "--count", `${baseRef}..HEAD`],
{ allowNonZeroExit: true },
);
- if (result.code !== 0) {
+ if (result.exitCode !== 0) {
return 0;
}
@@ -1140,7 +1140,7 @@
);
const branchLastCommit = new Map<string, number>();
- if (branchRecency.code !== 0) {
+ if (branchRecency.exitCode !== 0) {
return branchLastCommit;
}
@@ -1173,7 +1173,7 @@
return NON_REPOSITORY_STATUS_DETAILS;
}
- if (statusResult.code !== 0) {
+ if (statusResult.exitCode !== 0) {
const stderr = statusResult.stderr.trim();
return yield* createGitCommandError(
"GitVcsDriver.statusDetails.status",
@@ -1206,7 +1206,7 @@
);
const statusStdout = statusResult.stdout;
const defaultBranch =
- defaultRefResult.code === 0
+ defaultRefResult.exitCode === 0
? defaultRefResult.stdout.trim().replace(/^refs\/remotes\/origin\//, "")
: null;
@@ -1614,7 +1614,7 @@
).pipe(
Effect.catchIf(isMissingGitCwdError, () =>
Effect.succeed({
- code: 128,
+ exitCode: 128,
stdout: "",
stderr: "fatal: not a git repository",
stdoutTruncated: false,
@@ -1623,7 +1623,7 @@
),
);
- if (localBranchResult.code !== 0) {
+ if (localBranchResult.exitCode !== 0) {
const stderr = localBranchResult.stderr.trim();
if (stderr.toLowerCase().includes("not a git repository")) {
return {
@@ -1654,7 +1654,7 @@
Effect.catch((error) =>
Effect.logWarning(
`GitVcsDriver.listRefs: remote refName lookup failed for ${input.cwd}: ${error.message}. Falling back to an empty remote refName list.`,
- ).pipe(Effect.as({ code: 1, stdout: "", stderr: "" })),
+ ).pipe(Effect.as({ exitCode: 1, stdout: "", stderr: "" })),
),
);
@@ -1670,7 +1670,7 @@
Effect.catch((error) =>
Effect.logWarning(
`GitVcsDriver.listRefs: remote name lookup failed for ${input.cwd}: ${error.message}. Falling back to an empty remote name list.`,
- ).pipe(Effect.as({ code: 1, stdout: "", stderr: "" })),
+ ).pipe(Effect.as({ exitCode: 1, stdout: "", stderr: "" })),
),
);
@@ -1703,25 +1703,25 @@
);
const remoteNames =
- remoteNamesResult.code === 0 ? parseRemoteNames(remoteNamesResult.stdout) : [];
- if (remoteBranchResult.code !== 0 && remoteBranchResult.stderr.trim().length > 0) {
+ remoteNamesResult.exitCode === 0 ? parseRemoteNames(remoteNamesResult.stdout) : [];
+ if (remoteBranchResult.exitCode !== 0 && remoteBranchResult.stderr.trim().length > 0) {
yield* Effect.logWarning(
- `GitVcsDriver.listRefs: remote refName lookup returned code ${remoteBranchResult.code} for ${input.cwd}: ${remoteBranchResult.stderr.trim()}. Falling back to an empty remote refName list.`,
+ `GitVcsDriver.listRefs: remote refName lookup returned code ${remoteBranchResult.exitCode} for ${input.cwd}: ${remoteBranchResult.stderr.trim()}. Falling back to an empty remote refName list.`,
);
}
- if (remoteNamesResult.code !== 0 && remoteNamesResult.stderr.trim().length > 0) {
+ if (remoteNamesResult.exitCode !== 0 && remoteNamesResult.stderr.trim().length > 0) {
yield* Effect.logWarning(
- `GitVcsDriver.listRefs: remote name lookup returned code ${remoteNamesResult.code} for ${input.cwd}: ${remoteNamesResult.stderr.trim()}. Falling back to an empty remote name list.`,
+ `GitVcsDriver.listRefs: remote name lookup returned code ${remoteNamesResult.exitCode} for ${input.cwd}: ${remoteNamesResult.stderr.trim()}. Falling back to an empty remote name list.`,
);
}
const defaultBranch =
- defaultRef.code === 0
+ defaultRef.exitCode === 0
? defaultRef.stdout.trim().replace(/^refs\/remotes\/origin\//, "")
: null;
const worktreeMap = new Map<string, string>();
- if (worktreeList.code === 0) {
+ if (worktreeList.exitCode === 0) {
let currentPath: string | null = null;
for (const line of worktreeList.stdout.split("\n")) {
if (line.startsWith("worktree ")) {
@@ -1762,7 +1762,7 @@
});
const remoteBranches =
- remoteBranchResult.code === 0
+ remoteBranchResult.exitCode === 0
? remoteBranchResult.stdout
.split("\n")
.map(parseBranchLine)
@@ -1943,7 +1943,7 @@
timeoutMs: 5_000,
allowNonZeroExit: true,
},
- ).pipe(Effect.map((result) => result.code === 0)),
+ ).pipe(Effect.map((result) => result.exitCode === 0)),
executeGit(
"GitVcsDriver.switchRef.remoteExists",
input.cwd,
@@ -1952,7 +1952,7 @@
timeoutMs: 5_000,
allowNonZeroExit: true,
},
- ).pipe(Effect.map((result) => result.code === 0)),
+ ).pipe(Effect.map((result) => result.exitCode === 0)),
],
{ concurrency: "unbounded" },
);
@@ -1968,7 +1968,7 @@
},
).pipe(
Effect.map((result) =>
- result.code === 0
+ result.exitCode === 0
? parseTrackingBranchByUpstreamRef(result.stdout, input.refName)
: null,
),
@@ -1986,7 +1986,7 @@
timeoutMs: 5_000,
allowNonZeroExit: true,
},
- ).pipe(Effect.map((result) => result.code === 0))
+ ).pipe(Effect.map((result) => result.exitCode === 0))
: false;
const checkoutArgs = localInputExistsYou can send follow-ups to the cloud agent here.
Contributor
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 2 total unresolved issues (including 1 from previous review).
Autofix Details
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Missing empty-stdout guard breaks PR lookup loop
- Added an empty-string guard before JSON decoding in the non-open state branch of listChangeRequests, returning an empty array when stdout is empty so the findLatestPr loop can continue trying remaining head selectors.
Or push these changes by commenting:
@cursor push d5fa9c0593
Preview (d5fa9c0593)
diff --git a/apps/server/src/sourceControl/GitHubSourceControlProvider.ts b/apps/server/src/sourceControl/GitHubSourceControlProvider.ts
--- a/apps/server/src/sourceControl/GitHubSourceControlProvider.ts
+++ b/apps/server/src/sourceControl/GitHubSourceControlProvider.ts
@@ -76,8 +76,12 @@
],
})
.pipe(
- Effect.flatMap((result) =>
- Effect.sync(() => decodeGitHubPullRequestListJson(result.stdout.trim())).pipe(
+ Effect.flatMap((result) => {
+ const raw = result.stdout.trim();
+ if (raw.length === 0) {
+ return Effect.succeed([]);
+ }
+ return Effect.sync(() => decodeGitHubPullRequestListJson(raw)).pipe(
Effect.flatMap((decoded) =>
Result.isSuccess(decoded)
? Effect.succeed(
@@ -95,8 +99,8 @@
}),
),
),
- ),
- ),
+ );
+ }),
Effect.mapError((error) =>
Schema.is(SourceControlProviderError)(error)
? errorYou can send follow-ups to the cloud agent here.
Contributor
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 2 total unresolved issues (including 1 from previous review).
Autofix Details
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Provider resolution failure bypasses default branch fallback
- Wrapped both
sourceControlProvider(cwd)resolution and.getDefaultBranch()in a single Effect pipeline usingEffect.flatMap, soEffect.catchnow handles failures from either step and the"main"fallback is always reachable.
- Wrapped both
Or push these changes by commenting:
@cursor push b73e16cbcc
Preview (b73e16cbcc)
diff --git a/apps/server/src/git/GitManager.ts b/apps/server/src/git/GitManager.ts
--- a/apps/server/src/git/GitManager.ts
+++ b/apps/server/src/git/GitManager.ts
@@ -1045,9 +1045,10 @@
}
}
- const defaultFromProvider = yield* (yield* sourceControlProvider(cwd))
- .getDefaultBranch({ cwd })
- .pipe(Effect.catch(() => Effect.succeed(null)));
+ const defaultFromProvider = yield* sourceControlProvider(cwd).pipe(
+ Effect.flatMap((provider) => provider.getDefaultBranch({ cwd })),
+ Effect.catch(() => Effect.succeed(null)),
+ );
if (defaultFromProvider) {
return defaultFromProvider;
}You can send follow-ups to the cloud agent here.
Contributor
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 3 total unresolved issues (including 2 from previous reviews).
Autofix Details
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Local variable shadows outer
sourceControlProviderfunction- Renamed the local variable from
sourceControlProvidertohostingProviderinsidereadLocalStatusto eliminate the shadowing of the outersourceControlProviderfunction, while preserving thesourceControlProviderkey in the returned object.
- Renamed the local variable from
Or push these changes by commenting:
@cursor push fde1ecb449
Preview (fde1ecb449)
diff --git a/apps/server/src/git/GitManager.ts b/apps/server/src/git/GitManager.ts
--- a/apps/server/src/git/GitManager.ts
+++ b/apps/server/src/git/GitManager.ts
@@ -699,13 +699,13 @@
.pipe(
Effect.catchIf(isNotGitRepositoryError, () => Effect.succeed(nonRepositoryStatusDetails)),
);
- const sourceControlProvider = details.isRepo
+ const hostingProvider = details.isRepo
? yield* resolveHostingProvider(cwd, details.branch)
: null;
return {
isRepo: details.isRepo,
- ...(sourceControlProvider ? { sourceControlProvider } : {}),
+ ...(hostingProvider ? { sourceControlProvider: hostingProvider } : {}),
hasPrimaryRemote: details.hasOriginRemote,
isDefaultRef: details.isDefaultBranch,
refName: details.branch,You can send follow-ups to the cloud agent here.
Member
Author
juliusmarminge
force-pushed
the
t3code/pluggable-git-integration
branch
from
84826c9 to
a73408c
Compare
juliusmarminge
force-pushed
the
t3code/pluggable-git-integration
branch
from
476e591 to
47942a9
Compare
| @@ -496,9 +448,12 @@ const createTrace2Monitor = Effect.fn("createTrace2Monitor")(function* ( | |||
|
|
|||
| if (event === "child_exit") { | |||
Contributor
There was a problem hiding this comment.
🟡 Medium vcs/GitVcsDriverCore.ts:449
Line 451 references traceRecord.success.exitCode, but git trace2 child_exit events use the field name code, not exitCode. Since exitCode does not exist on the trace record, code is always undefined, so the typeof code === "number" check on line 452 always fails and exitCode is always null for hook finished events. The diff shows this was incorrectly changed from .code to .exitCode.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file apps/server/src/vcs/GitVcsDriverCore.ts around line 449:
Line 451 references `traceRecord.success.exitCode`, but git trace2 `child_exit` events use the field name `code`, not `exitCode`. Since `exitCode` does not exist on the trace record, `code` is always `undefined`, so the `typeof code === "number"` check on line 452 always fails and `exitCode` is always `null` for hook finished events. The diff shows this was incorrectly changed from `.code` to `.exitCode`.
Evidence trail:
1. apps/server/src/vcs/GitVcsDriverCore.ts line 451: `const code = traceRecord.success.exitCode;` (at REVIEWED_COMMIT)
2. apps/server/src/vcs/GitVcsDriverCore.ts line 452: `const exitCode = typeof code === "number" && Number.isInteger(code) ? code : null;`
3. Trace2Record schema at line 387: `const Trace2Record = Schema.Record(Schema.String, Schema.Unknown);` — produces Record<string, unknown>
4. Git trace2 documentation: https://git-scm.com/docs/api-trace2 — child_exit event format shows field name is `code`, not `exitCode`:
[code fence]json
{"event":"child_exit", "child_id":2, "pid":14708, "code":0, "t_rel":0.110605}
[code fence]
5. PR diff shows file is entirely new (`--- /dev/null`, `new file mode 100644`) — sub-claim about 'diff showing change from .code to .exitCode' is incorrect but ancillary
added 8 commits
May 2, 2026 14:22
- Add phase 1 VCS driver plan - Add phase 2 source control provider plan - Index both plans in `.plans/README.md`
- Replace direct GitCore dependencies with VcsDriver and VcsProcess - Move checkpointing, workspace, and orchestration flows onto the new VCS abstraction - Drop workspace-specific git helpers from GitCore
- Move VcsDriver and VcsProcess out of nested service/layer paths - Update server, workspace, orchestration, and tests to use new exports - Rename layer exports to `layer` for consistency
- Switch GitVcsDriver and VcsProcess usages to namespace imports - Update server and integration wiring to use `.layer` consistently - Keep test type annotations aligned with the exported service types
- Remove the aliased service import - Reference `VcsProcess.VcsProcess` explicitly in checkpoint and workspace tests
- Import `VcsProcess` as a namespace in the test - Update layer injection to target `VcsProcess.VcsProcess`
- Switch freshness and scan timestamps to `DateTime.now` from Effect - Keep timestamps consistent within each async operation
- Introduce a dedicated `GitVcsDriver` service and `vcsLayer` - Update server, orchestration, and workspace wiring to use the new layer - Adjust tests and mocks for the new git-specific dependency
- Move GitManager and GitHubCli contracts/layers to their new modules - Update server wiring and tests for the relocated pluggable git setup
- Resolve symlinked working directories before caching and workflow calls - Pass execute env through the Git VCS driver
- Rename branch fixtures to refs and primary remote flags - Align Git actions and keybinding tests with new ref labels
- Switch VCS process and git driver results to `ChildProcessSpawner.ExitCode` - Update status checks and fallback results to use the new typed exit code - Refresh tests to match the new result shape
- Add a reusable `spawn` API for VCS processes - Factor stream collection into `collectText` - Preserve existing `run` behavior on top of the new handle
- Replace raw spawn access with `withProcess` for scoped use - Ensure child processes are finalized even on early exits - Keep `run` built on the scoped helper for safer cleanup
…hadowing outer function Applied via @cursor push command
- Treat empty `gh pr list` output as no results - Fall back to `main` when provider default-branch detection fails - Add coverage for PR creation fallback behavior
Co-authored-by: codex <codex@users.noreply.github.com>
juliusmarminge
force-pushed
the
t3code/pluggable-git-integration
branch
from
7aa00ff to
f4180a4
Compare
Contributor
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Unhandled provider error aborts action before commit/push
- Wrapped the sourceControlProvider() call in runStackedAction with Effect.catch fallback to getChangeRequestTerminologyForKind("unknown"), matching the existing pattern in buildCompletionToast.
Or push these changes by commenting:
@cursor push 6dd13497dc
Preview (6dd13497dc)
diff --git a/apps/server/src/git/GitManager.ts b/apps/server/src/git/GitManager.ts
--- a/apps/server/src/git/GitManager.ts
+++ b/apps/server/src/git/GitManager.ts
@@ -1663,7 +1663,10 @@
const currentBranch = branchStep.name ?? initialStatus.branch;
const commitAction = isCommitAction(input.action) ? input.action : null;
const changeRequestTerms = wantsPr
- ? getChangeRequestTerminologyForKind((yield* sourceControlProvider(input.cwd)).kind)
+ ? yield* sourceControlProvider(input.cwd).pipe(
+ Effect.map((provider) => getChangeRequestTerminologyForKind(provider.kind)),
+ Effect.catch(() => Effect.succeed(getChangeRequestTerminologyForKind("unknown"))),
+ )
: null;
const commit = commitActionYou can send follow-ups to the cloud agent here.
Reviewed by Cursor Bugbot for commit 76dc275. Configure here.
Wrap sourceControlProvider() call at line 1665 with Effect.catch fallback, matching the existing pattern in buildCompletionToast. The provider resolution is only used for cosmetic progress labels and should never abort the commit/push action.
Member
Author
Wrap sourceControlProvider() call at line 1665 with Effect.catch fallback, matching the existing pattern in buildCompletionToast. The provider resolution is only used for cosmetic progress labels and should never abort the commit/push action. Applied via @cursor push command
Morphexe
pushed a commit
to Morphexe/t3code
that referenced
this pull request
May 4, 2026
Co-authored-by: Julius Marminge <julius@macmini.local> Co-authored-by: Cursor Agent <cursoragent@cursor.com> Co-authored-by: codex <codex@users.noreply.github.com> Co-authored-by: cursor[bot] <206951365+cursor[bot]@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
this is a core rearchitecture we can add
JJ,GitLab,BitBucketon top of laterSummary
vcscontract layer and typed VCS errors to replace Git-specific plumbing with provider-neutral abstractions.GitVcsDriverandVcsProcessservices, then rewired server code that previously depended onGitCore.GitCoreimplementation and its tests in favor of the new driver-backed execution path.Testing
bun fmtbun lintbun typecheckNote
High Risk
Large refactor of core Git/VCS and source-control orchestration paths (status/PR/worktrees/checkpointing) to new
VcsProcess/driver registries, which can break repo detection, caching, and PR workflows if behavior differs. Touches subprocess execution and typed error plumbing, increasing regression risk across many integration tests.Overview
Introduces a new VCS foundation:
VcsProcess(Effect-native subprocess execution with bounded output/timeouts and typed errors) plusVcsDriverRegistrythat detects/caches repositories and routes toGitVcsDriver.Refactors server workflows to use these abstractions: checkpointing now executes Git commands via the registry and returns
VcsErrortypes; orchestration/integration harness wiring swapsGitCore/GitStatusBroadcasterforVcsDriverRegistry/VcsStatusBroadcasterand addsGitWorkflowServicerouting.Reworks
GitManagerto depend onGitVcsDriverfor local operations andSourceControlProviderRegistryfor change-request operations (provider-neutral terminology, updated status/PR field names likerefName/hasPrimaryRemote), with updated tests and a new provider-fallback test. Adds Phase 1/2 version-control plan docs under.plans/.Reviewed by Cursor Bugbot for commit 9e3db6e. Bugbot is set up for automated code reviews on this repo. Configure here.
Note
Introduce pluggable VCS driver foundation replacing Git-specific services with provider-neutral abstractions
VcsDriverinterface andGitVcsDriverimplementation behind aVcsDriverRegistrythat resolves drivers by kind, with detection caching (2s TTL) and project-config discovery via.t3code/vcs.jsonVcsProcessas a standardized subprocess abstraction for all VCS command execution, replacing directGitCoreusage throughout checkpointing, workspace indexing, and orchestrationgit.*tovcs.*(e.g.vcsListRefs,vcsSwitchRef,vcsCreateRef) and replacessubscribeGitStatuswithsubscribeVcsStatus; field names change frombranch/hasOriginRemote/isDefaultBranchtorefName/hasPrimaryRemote/isDefaultRefthroughoutSourceControlProviderRegistrywith aGitHubSourceControlProvideradapter and aSourceControlDiscoveryservice that probes locally available VCS tools and providers (GitHub, GitLab, Azure DevOps, Bitbucket); a new/settings/source-controlsettings panel surfaces discovery resultsGitWorkflowServiceas a facade overGitVcsDriver+GitManagerthat returns empty non-repo results when no repository is detected, replacing directGitCore/GitStatusBroadcasterdependencies in orchestrationMacroscope summarized 9e3db6e.