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)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
bot
added
size:XXL
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| branch.name === currentStatus.branch | ||
| ? workspaceRoot | ||
| : (branch.worktreePath ?? | ||
| (branch.name === currentStatus.branch ? registryCurrentPath : null)); |
There was a problem hiding this comment.
Redundant condition makes registryCurrentPath dead code
Medium Severity
In the adjustedLocalBranches mapping, the else branch of branch.name === currentStatus.branch contains a nested condition branch.name === currentStatus.branch which is always false, since we only enter the else branch when the names differ. This makes registryCurrentPath permanently unreachable dead code, and the fallback always resolves to null instead of the intended registry lookup.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 37a711d. Configure here.
| return; | ||
| } | ||
| if (!isGitWorkspace(sessionRuntime.value.cwd)) { | ||
| if (!isVersionControlledWorkspace(sessionRuntime.value.cwd)) { |
There was a problem hiding this comment.
Error message references git after VCS abstraction
Low Severity
The check was updated from isGitWorkspace to isVersionControlledWorkspace (covering both git and jj), but the failure message at line 594 still reads "this project is not a git repository." For a jj-managed project that fails this check, the message would be misleading. The detail string doesn't reflect the broadened VCS support.
Reviewed by Cursor Bugbot for commit 37a711d. Configure here.
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
| export function runJj( | ||
| cwd: string, | ||
| args: readonly string[], | ||
| allowNonZeroExit = false, | ||
| ): Effect.Effect<ProcessRunResult, Error> { | ||
| return Effect.promise(() => | ||
| runProcess("jj", args, { |
There was a problem hiding this comment.
🟢 Low Layers/JjTestUtils.ts:50
Effect.promise treats rejections as defects rather than typed errors, so runProcess rejections (non-zero exit, buffer overflow, spawn errors) crash the program instead of appearing in the error channel. The return type Effect.Effect<ProcessRunResult, Error> is misleading—no error will ever reach that channel. Consider using Effect.tryPromise to capture rejections as typed errors.
- return Effect.promise(() =>
+ return Effect.tryPromise(() =>
runProcess("jj", args, {Also found in 1 other location(s)
apps/server/src/checkpointing/Layers/CheckpointStore.ts:129
At line 129,
JSON.parse(raw)can throw aSyntaxErrorif the checkpoint file contains malformed JSON. This synchronous exception becomes a defect that escapes theEffect.catch(() => Effect.succeed(null))at line 141, sinceEffect.catchonly handles expected errors (typedE), not defects. This causesresolveJjCheckpointCommitto die instead of returningnullas intended when a checkpoint file exists but is corrupted. Wrap the parse inEffect.tryor useSchema.decodeUnknownto handle parse failures as expected errors.
🤖 Copy this AI Prompt to have your agent fix this:
In file apps/server/src/jj/Layers/JjTestUtils.ts around lines 50-56:
`Effect.promise` treats rejections as defects rather than typed errors, so `runProcess` rejections (non-zero exit, buffer overflow, spawn errors) crash the program instead of appearing in the error channel. The return type `Effect.Effect<ProcessRunResult, Error>` is misleading—no error will ever reach that channel. Consider using `Effect.tryPromise` to capture rejections as typed errors.
Evidence trail:
1. apps/server/src/jj/Layers/JjTestUtils.ts lines 50-61 (REVIEWED_COMMIT): `runJj` function uses `Effect.promise()` with return type `Effect.Effect<ProcessRunResult, Error>`
2. apps/server/src/processRunner.ts lines 128-268 (REVIEWED_COMMIT): `runProcess` function can reject via `reject()` on non-zero exit (line 244), spawn error (line 230), buffer overflow (lines 222, 227), stdin error (line 248)
3. Web search results from https://github.com/Effect-TS/effect/issues/3563 confirming Effect.promise treats rejections as defects: "Promise rejections are 'defects' if you use Effect.promise"
4. git_grep results showing the codebase uses `Effect.tryPromise` extensively elsewhere for failable promises (e.g., Manager.ts:310, Utils.ts:100, JjManager.ts:764)
Also found in 1 other location(s):
- apps/server/src/checkpointing/Layers/CheckpointStore.ts:129 -- At line 129, `JSON.parse(raw)` can throw a `SyntaxError` if the checkpoint file contains malformed JSON. This synchronous exception becomes a defect that escapes the `Effect.catch(() => Effect.succeed(null))` at line 141, since `Effect.catch` only handles expected errors (typed `E`), not defects. This causes `resolveJjCheckpointCommit` to die instead of returning `null` as intended when a checkpoint file exists but is corrupted. Wrap the parse in `Effect.try` or use `Schema.decodeUnknown` to handle parse failures as expected errors.
| function truncateOutput( | ||
| value: string, | ||
| maxOutputBytes: number, | ||
| ): { text: string; truncated: boolean } { | ||
| const byteLength = Buffer.byteLength(value); | ||
| if (byteLength <= maxOutputBytes) { | ||
| return { text: value, truncated: false }; | ||
| } | ||
|
|
||
| let text = value; | ||
| while (Buffer.byteLength(text) > maxOutputBytes) { | ||
| text = text.slice(0, Math.max(0, Math.floor(text.length * 0.9))); | ||
| } | ||
|
|
||
| return { | ||
| text: `${text}${OUTPUT_TRUNCATED_MARKER}`, | ||
| truncated: true, | ||
| }; | ||
| } |
There was a problem hiding this comment.
🟢 Low jj/Utils.ts:53
After the truncation loop, truncateOutput appends OUTPUT_TRUNCATED_MARKER unconditionally, so the final string exceeds maxOutputBytes by 13 bytes. If callers enforce strict size limits (e.g., for storage or transmission), this causes downstream failures. Consider reserving space for the marker during truncation, or truncating again after appending.
+ const markerByteLength = Buffer.byteLength(OUTPUT_TRUNCATED_MARKER);
const byteLength = Buffer.byteLength(value);
- if (byteLength <= maxOutputBytes) {
+ if (byteLength <= maxOutputBytes - markerByteLength) {
return { text: value, truncated: false };
}
let text = value;
- while (Buffer.byteLength(text) > maxOutputBytes) {
+ while (Buffer.byteLength(text) > maxOutputBytes - markerByteLength) {
text = text.slice(0, Math.max(0, Math.floor(text.length * 0.9)));
}
return {
text: `${text}${OUTPUT_TRUNCATED_MARKER}`,
truncated: true,
};
+}🤖 Copy this AI Prompt to have your agent fix this:
In file apps/server/src/jj/Utils.ts around lines 53-71:
After the truncation loop, `truncateOutput` appends `OUTPUT_TRUNCATED_MARKER` unconditionally, so the final string exceeds `maxOutputBytes` by 13 bytes. If callers enforce strict size limits (e.g., for storage or transmission), this causes downstream failures. Consider reserving space for the marker during truncation, or truncating again after appending.
Evidence trail:
apps/server/src/jj/Utils.ts lines 53-70 (REVIEWED_COMMIT): The `truncateOutput` function truncates text in a while loop until `Buffer.byteLength(text) <= maxOutputBytes`, then appends `OUTPUT_TRUNCATED_MARKER` (line 68). apps/server/src/jj/Utils.ts line 31: `OUTPUT_TRUNCATED_MARKER = "\n\n[truncated]"` which is 13 bytes. The returned text can exceed `maxOutputBytes` by 13 bytes.
| const commit = wantsCommit | ||
| ? yield* Effect.gen(function* () { | ||
| yield* Ref.set(currentPhase, "commit"); | ||
| return yield* runCommitStep( | ||
| modelSelection, | ||
| input.cwd, | ||
| currentBranch, | ||
| commitMessageForStep, | ||
| preResolvedCommitSuggestion, | ||
| input.filePaths, | ||
| ); | ||
| }) |
There was a problem hiding this comment.
🟡 Medium Layers/JjManager.ts:883
The runCommitStep phase at line 883 emits Ref.set(currentPhase, "commit") but never calls progress.emit({ kind: "phase_started", phase: "commit", ... }), so clients never receive the "commit" phase start event despite it being listed in the phases array. All other phases (branch, push, pr) emit both. Add the missing progress.emit call before runCommitStep.
const commit = wantsCommit
? yield* Effect.gen(function* () {
yield* Ref.set(currentPhase, "commit");
+ yield* progress.emit({
+ kind: "phase_started",
+ phase: "commit",
+ label: "Committing...",
+ });
return yield* runCommitStep(🤖 Copy this AI Prompt to have your agent fix this:
In file apps/server/src/jj/Layers/JjManager.ts around lines 883-894:
The `runCommitStep` phase at line 883 emits `Ref.set(currentPhase, "commit")` but never calls `progress.emit({ kind: "phase_started", phase: "commit", ... })`, so clients never receive the "commit" phase start event despite it being listed in the `phases` array. All other phases (`branch`, `push`, `pr`) emit both. Add the missing `progress.emit` call before `runCommitStep`.
Evidence trail:
apps/server/src/jj/Layers/JjManager.ts lines 860-866 (branch phase with both Ref.set and progress.emit), lines 883-885 (commit phase with only Ref.set, no progress.emit), lines 897-903 (push phase with both), lines 908-915 (pr phase with both). git_grep confirmed phase_started only at lines 863, 901, 913 - no occurrence for commit phase.
| export const resolveJjRepoDir = (cwd: string) => | ||
| resolveJjRoot(cwd).pipe( | ||
| Effect.flatMap((workspaceRoot) => | ||
| Effect.tryPromise({ | ||
| try: async () => { | ||
| const repoPointerPath = path.join(workspaceRoot, ".jj", "repo"); | ||
| try { | ||
| const repoPointerStats = await fsPromises.stat(repoPointerPath); | ||
| if (repoPointerStats.isFile()) { | ||
| const repoDir = (await fsPromises.readFile(repoPointerPath, "utf8")).trim(); | ||
| if (repoDir.length > 0) { | ||
| return repoDir; | ||
| } | ||
| } |
There was a problem hiding this comment.
🟡 Medium jj/Utils.ts:288
When .jj/repo contains a relative path (e.g., ../../shared-repo), resolveJjRepoDir returns it as-is. Callers then use path.join() with this value, which treats the relative path as relative to the current working directory instead of the workspace root, producing incorrect paths. Consider resolving the relative path against workspaceRoot before returning.
+ if (repoDir.length > 0) {
+ return path.isAbsolute(repoDir) ? repoDir : path.resolve(workspaceRoot, ".jj", repoDir);
- if (repoDir.length > 0) {
- return repoDir;🤖 Copy this AI Prompt to have your agent fix this:
In file apps/server/src/jj/Utils.ts around lines 288-301:
When `.jj/repo` contains a relative path (e.g., `../../shared-repo`), `resolveJjRepoDir` returns it as-is. Callers then use `path.join()` with this value, which treats the relative path as relative to the current working directory instead of the workspace root, producing incorrect paths. Consider resolving the relative path against `workspaceRoot` before returning.
Evidence trail:
apps/server/src/jj/Utils.ts:288-315 (REVIEWED_COMMIT) - `resolveJjRepoDir` function showing line 299 returns `repoDir` as-is without resolving against `workspaceRoot`; apps/server/src/jj/Layers/JjCore.ts:273 (REVIEWED_COMMIT) - `workspaceRegistryPath` uses `path.join(repoDir, WORKSPACE_REGISTRY_FILE)` which would produce incorrect paths when `repoDir` is relative; apps/server/src/jj/Layers/JjCore.ts:275-276,509,1202 (REVIEWED_COMMIT) - callers that pass the result to `readWorkspaceRegistry`
ApprovabilityVerdict: Needs human review 3 blocking correctness issues 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. |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
There are 4 total unresolved issues (including 2 from previous reviews).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit f686821. Configure here.
| } | ||
| return Effect.succeed(null); | ||
| }), | ||
| Effect.catch(() => Effect.succeed(null)), |
There was a problem hiding this comment.
JSON.parse defect escapes Effect.catch in checkpoint resolution
High Severity
JSON.parse(raw) inside Effect.map throws a SyntaxError on malformed JSON, which becomes a defect (not a typed error). Effect.catch only handles typed errors in the error channel, so the defect propagates as an unrecoverable fiber death instead of returning null. This means resolveJjCheckpointCommit will crash the fiber instead of gracefully degrading when a checkpoint file is corrupted. The same pattern appears in readWorkspaceRegistry in JjCore.ts. Wrapping the parse in Effect.try would convert the thrown exception into a recoverable error.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit f686821. Configure here.
| const gitCore = yield* GitCore; | ||
| const jjCore = yield* JjCore; | ||
|
|
||
| const selectCore = (cwd: string) => (detectRepoKind(cwd) === "jj" ? jjCore : gitCore); |
There was a problem hiding this comment.
Synchronous blocking I/O on every VCS operation
Medium Severity
detectRepoKind uses existsSync and statSync to walk up the entire directory tree on every VCS operation. In VcsCoreLive, selectCore is called for each routed method invocation, and in CheckpointStore, repoKindOf is called multiple times per checkpoint capture/restore/diff. This synchronous blocking I/O on the main thread can cause noticeable latency, especially for deeply nested workspaces or networked filesystems, and the result is never cached.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit f686821. Configure here.
| const registryCurrentPath = currentStatus.branch | ||
| ? (registry.branches[currentStatus.branch] ?? null) | ||
| : null; | ||
| const adjustedLocalBranches: GitBranch[] = localBranches.map((branch) => { |
There was a problem hiding this comment.
🟡 Medium Layers/JjCore.ts:1267
On line 1272, the inner check branch.name === currentStatus.branch is unreachable because line 1269 already established branch.name !== currentStatus.branch (it's in the else branch). This means registryCurrentPath — computed specifically for this lookup — can never be returned, so the worktree path for non-current branches always falls back to null instead of consulting the registry for the current branch's path. Consider restructuring to use registryCurrentPath when appropriate, or remove the dead code and the unused registryCurrentPath computation if the registry lookup is not needed.
🤖 Copy this AI Prompt to have your agent fix this:
In file apps/server/src/jj/Layers/JjCore.ts around line 1267:
On line 1272, the inner check `branch.name === currentStatus.branch` is unreachable because line 1269 already established `branch.name !== currentStatus.branch` (it's in the `else` branch). This means `registryCurrentPath` — computed specifically for this lookup — can never be returned, so the worktree path for non-current branches always falls back to `null` instead of consulting the registry for the current branch's path. Consider restructuring to use `registryCurrentPath` when appropriate, or remove the dead code and the unused `registryCurrentPath` computation if the registry lookup is not needed.
Evidence trail:
apps/server/src/jj/Layers/JjCore.ts lines 1264-1272 at REVIEWED_COMMIT. Line 1269 checks `branch.name === currentStatus.branch` and returns `workspaceRoot` if true. The else branch (lines 1271-1272) contains `branch.name === currentStatus.branch ? registryCurrentPath : null`, but this code path is only reached when `branch.name !== currentStatus.branch`, making the inner condition always false and `registryCurrentPath` unreachable.
1 participant
Summary
VcsCore,VcsManager) so the app can work with both Git and Jujutsu version control systemsjj) support:JjCorefor low-level operations (status, diff, log, bookmarks, conflicts) andJjManagerfor high-level workflows (checkpoint/restore, branch management)Test plan
bun run testpasses (new tests for JjCore, JjManager, VcsCore, VcsManager, and updated CheckpointStore tests)bun typecheckpasses🤖 Generated with Claude Code
Note
High Risk
High risk because it introduces a new VCS backend (
jj) and routes core workflows (checkpoint capture/restore/diff, branch rename, PR thread prep) through new selection logic based on repo markers, affecting critical developer operations across the server runtime.Overview
Adds Jujutsu (jj) support alongside Git via a new VCS abstraction. Introduces
JjCore/JjManagerimplementations and wires them into the server runtime, plusVcsCore/VcsManagerrouting that selectsgitvsjjbased on.jj/.gitmarkers.Checkpointing is generalized from git-only to VCS-aware.
CheckpointStoreLivenow usesVcsCorefor git repos and adds jj-native checkpoints stored as repo metadata (oplog-based) with corresponding restore/diff/delete logic; orchestration reactors switch fromisGitRepositorychecks toisVersionControlledRepositoryand tests are updated/expanded to cover jj behavior.Shared utilities and call sites are updated for the new VCS namespace. Branch-name sanitization/dedupe helpers move from
@t3tools/shared/gitto@t3tools/shared/vcs, and places that previously depended directly onGitCore/GitManagerfor workflow operations (e.g., branch rename) now depend onVcsCore/VcsManager(with new/updated integration and unit tests for routing and jj flows).Reviewed by Cursor Bugbot for commit f686821. Bugbot is set up for automated code reviews on this repo. Configure here.
Note
Add Jujutsu (jj) VCS support alongside Git with dynamic routing
JjCoreandJjManagerservices mirroring the Git backend, wired into a newVcsCore/VcsManagerabstraction layer that routes operations to git or jj based ondetectRepoKind(cwd)(checks for.git/.jjmarkers).GitCore/GitManagerusage across workspace indexing, orchestration reactors, websocket RPC handlers, and web UI React Query hooks withVcsCore/VcsManagerequivalents.CheckpointStoreto support jj repos: captures checkpoints as operation log entries stored in.jj/t3-checkpoints/, and restores/diffs viajj restore/jj diffinstead of git plumbing../vcssubpath topackages/sharedandpackages/contractsthat re-exports git utilities and types underVcs*aliases for downstream consumers..jjto the ignored directory names in workspace file scanning.VcsCorerather thanGitCoredirectly; any gap in the routing logic could silently fall back to the wrong backend.Macroscope summarized f686821.