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
36 changes: 36 additions & 0 deletions apps/server/src/mcp/toolkits/project/handlers.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -504,3 +504,39 @@ it.effect("a launch into a new worktree creates nothing when its base ref has no
expect(launched).toHaveLength(1);
}),
);

it.effect("a non-Git worktree refusal points the caller to the project folder", () =>
Effect.gen(function* () {
const launched: Array<ThreadLaunch.ThreadLaunchInput> = [];
const { projectId, dependencies } = clientLaunchHarness({
runtimeModeCeiling: "auto",
launched,
checkWorktreeBase: ({ projectId }) =>
Effect.fail(
new ThreadLaunch.ThreadLaunchNotRepositoryError({
projectId,
projectTitle: "Plain folder",
workspaceRoot: "/plain",
}),
),
});
const toolkit = yield* ProjectToolkit.pipe(
Effect.provide(
McpToolAccess.HandlersLayer.layer(ProjectHandlers.layer).pipe(Layer.provide(dependencies)),
),
);
const replies = yield* toolkit
.handle("t3_thread_launch", {
title: "Fix",
projectId,
workspaceStrategy: { type: "worktree", baseRef: "main" },
})
.pipe(Stream.unwrap, Stream.runCollect, Effect.provide(dependencies));
expect(replies.at(-1)?.result).toMatchObject({
code: "invalid_request",
message:
'Project "Plain folder" (/plain) is not a Git repository. No thread was created. Use workspaceStrategy {type:"root"} for this project.',
});
expect(launched).toHaveLength(0);
}),
);
13 changes: 8 additions & 5 deletions apps/server/src/mcp/toolkits/project/handlers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -114,11 +114,14 @@ export const layer = McpToolAccess.toLayer(ProjectToolkit, {
(error) =>
new OrchestratorMcpFailure({
code: "invalid_request",
message: `${error.message} No thread was created. Pass a branch, tag, or commit that exists there${
strategy.startFromOrigin === true
? "; the project has no origin remote to fetch it from."
: ", or set startFromOrigin:true if it exists only on origin."
}`,
message:
error._tag === "ThreadLaunchNotRepositoryError"
? `${error.message} No thread was created. Use workspaceStrategy {type:"root"} for this project.`
: `${error.message} No thread was created. Pass a branch, tag, or commit that exists there${
strategy.startFromOrigin === true
? "; the project has no origin remote to fetch it from."
: ", or set startFromOrigin:true if it exists only on origin."
}`,
}),
),
);
Expand Down
51 changes: 48 additions & 3 deletions apps/server/src/orchestration-v2/ThreadLaunchService.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ import * as Exit from "effect/Exit";
import * as Fiber from "effect/Fiber";
import * as Layer from "effect/Layer";
import * as Option from "effect/Option";
import * as Path from "effect/Path";
import * as Ref from "effect/Ref";
import * as Result from "effect/Result";
import * as Stream from "effect/Stream";
Expand Down Expand Up @@ -117,6 +118,7 @@ const claudeAdapter = {
} as ProviderAdapter.ProviderAdapterV2["Service"];

interface HarnessOptions {
readonly existingPaths?: ReadonlyArray<string>;
readonly closeTerminal?: TerminalManager.TerminalManager["Service"]["close"];
readonly adapters?: ReadonlyArray<ProviderAdapter.ProviderAdapterV2["Service"]>;
readonly managedFolders?: Layer.Layer<ManagedProjectFolders.ManagedProjectFolders>;
Expand Down Expand Up @@ -200,6 +202,7 @@ function makeHarness(options: HarnessOptions = {}) {
renameBranch,
fetchRemote: options.fetchRemote ?? (() => Effect.void),
hasCommit: options.hasCommit ?? (() => Effect.succeed(false)),
isRepository: () => Effect.succeed(true),
remoteExists: options.remoteExists ?? (() => Effect.succeed(true)),
remoteBranchExists: () => Effect.succeed(true),
removeWorktree,
Expand Down Expand Up @@ -230,6 +233,10 @@ function makeHarness(options: HarnessOptions = {}) {
layerThreadManagement,
layerReceipts,
IdAllocator.layer,
FileSystem.layerNoop({
exists: (path) => Effect.succeed(options.existingPaths?.includes(path) === true),
}),
Path.layer,
),
),
);
Expand Down Expand Up @@ -3080,20 +3087,41 @@ it.effect.each([
{ name: "a repository with no commits yet", baseRef: "main", commits: [] },
{ name: "the previous checkout", baseRef: "-", commits: ["@{-1}", "HEAD"] },
{ name: "a commit search", baseRef: ":/fix", commits: ["HEAD"] },
{ name: "a folder that is not a repository", baseRef: "main", commits: null },
{ name: "a Git that cannot answer", baseRef: "main", commits: null },
{ name: "a failing repository probe", baseRef: "main", commits: ["HEAD"], probeFails: true },
{
name: "a repository Git refuses to read",
baseRef: "main",
commits: ["HEAD"],
existingPaths: ["/.git"],
},
])("leaves $name to the launch", (testCase) =>
Effect.gen(function* () {
const { commits } = testCase;
const harness = makeHarness({
git: { hasRefNamed: () => Effect.succeed(testCase.remoteBranch === true) },
existingPaths: testCase.existingPaths ?? [],
git: {
isRepository: () =>
testCase.probeFails === true
? Effect.fail(
new GitCommandError({
operation: "GitWorkflowService.isRepository",
command: "git rev-parse",
cwd: "/repo",
detail: "Repository probe failed.",
}),
)
: Effect.succeed(testCase.existingPaths === undefined),
hasRefNamed: () => Effect.succeed(testCase.remoteBranch === true),
},
hasCommit: ({ refName }) =>
commits === null
? Effect.fail(
new GitCommandError({
operation: "GitWorkflowService.hasCommit",
command: "git rev-parse",
cwd: "/repo",
detail: "Not a Git repository.",
detail: "fatal: bad object",
}),
)
: Effect.succeed(commits.includes(refName)),
Expand Down Expand Up @@ -3447,3 +3475,20 @@ it.effect.each(
}).pipe(Effect.provide(Layer.merge(harness.layer, layerService)));
}),
);

it.effect("refuses a new worktree in a non-Git folder even for a commit-search base", () => {
const harness = makeHarness({ git: { isRepository: () => Effect.succeed(false) } });
return Effect.gen(function* () {
const launches = yield* ThreadLaunch.ThreadLaunchService;
const result = yield* Effect.result(
launches.checkWorktreeBase({
projectId,
workspaceStrategy: { type: "worktree", baseRef: ":/fix", startFromOrigin: true },
}),
);
assert.isTrue(Result.isFailure(result));
if (Result.isFailure(result)) {
assert.match(result.failure.message, /not a Git repository/i);
}
}).pipe(Effect.provide(harness.layer));
});
66 changes: 53 additions & 13 deletions apps/server/src/orchestration-v2/ThreadLaunchService.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,8 +24,10 @@ import * as Context from "effect/Context";
import * as Effect from "effect/Effect";
import * as Exit from "effect/Exit";
import * as Fiber from "effect/Fiber";
import * as FileSystem from "effect/FileSystem";
import * as Layer from "effect/Layer";
import * as Option from "effect/Option";
import * as Path from "effect/Path";
import * as Ref from "effect/Ref";
import * as Schema from "effect/Schema";
import * as Scope from "effect/Scope";
Expand Down Expand Up @@ -157,6 +159,19 @@ export class ThreadLaunchBaseRefError extends Schema.TaggedError<ThreadLaunchBas
}
}

export class ThreadLaunchNotRepositoryError extends Schema.TaggedError<ThreadLaunchNotRepositoryError>()(
"ThreadLaunchNotRepositoryError",
{
projectId: ProjectId,
projectTitle: Schema.String,
workspaceRoot: Schema.String,
},
) {
override get message(): string {
return `Project "${this.projectTitle}" (${this.workspaceRoot}) is not a Git repository.`;
}
}

export class ThreadLaunchService extends Context.Service<
ThreadLaunchService,
{
Expand All @@ -167,12 +182,12 @@ export class ThreadLaunchService extends Context.Service<
ThreadLaunchError | ThreadManagement.ThreadManagementProviderInstanceDisabledError
>;
/**
* `launch` accepts such a launch and reports the failure inside the
* thread; a caller that needs it before a thread exists checks first.
* `launch` reports preparation failures inside an accepted thread.
* Callers that need to refuse impossible worktrees before creating one check here.
*/
readonly checkWorktreeBase: (
input: Pick<ThreadLaunchInput, "projectId" | "workspaceStrategy">,
) => Effect.Effect<void, ThreadLaunchBaseRefError>;
) => Effect.Effect<void, ThreadLaunchBaseRefError | ThreadLaunchNotRepositoryError>;
readonly retryPreparation: (
input: ThreadLaunchPreparationInput,
) => Effect.Effect<
Expand Down Expand Up @@ -209,6 +224,8 @@ const make = Effect.gen(function* () {
const ids = yield* IdAllocator.IdAllocatorV2;
const threads = yield* ThreadManagement.ThreadManagementService;
const managedFolders = yield* ManagedProjectFolders.ManagedProjectFolders;
const fileSystem = yield* FileSystem.FileSystem;
const path = yield* Path.Path;
const preparationScope = yield* Scope.make("sequential");
const scheduledLaunches = yield* Ref.make<ReadonlySet<CommandId>>(new Set());
yield* Effect.addFinalizer(() => Scope.close(preparationScope, Exit.void));
Expand Down Expand Up @@ -253,17 +270,40 @@ const make = Effect.gen(function* () {
}
});

// Git can also refuse an existing repository (for example, dubious ownership).
// A .git marker at any ancestor keeps that ambiguous result for launch to report.
const hasGitAbove = Effect.fn("ThreadLaunchService.hasGitAbove")(function* (cwd: string) {
let folder = path.resolve(cwd);
while (true) {
if (yield* fileSystem.exists(path.join(folder, ".git"))) return true;
const parent = path.dirname(folder);
if (parent === folder) return false;
folder = parent;
}
});

const checkWorktreeBase: ThreadLaunchService["Service"]["checkWorktreeBase"] = Effect.fn(
"ThreadLaunchService.checkWorktreeBase",
)(function* (input) {
const strategy = input.workspaceStrategy;
// `:/text` is a commit search, which cannot be asked for a commit type.
if (strategy.type !== "worktree" || strategy.baseRef.startsWith(":")) return;
if (strategy.type !== "worktree") return;
// Launch normalizes Scratch to a thread-owned folder before preparation.
if (yield* managedFolders.isScratchProject(input.projectId)) return;
const { baseRef } = strategy;
const unresolved = yield* Effect.gen(function* () {
const problem = yield* Effect.gen(function* () {
const project = Option.getOrUndefined(yield* projects.getById(input.projectId));
if (project === undefined) return undefined;
const cwd = project.workspaceRoot;
if (!(yield* git.isRepository(cwd))) {
if (yield* hasGitAbove(cwd)) return undefined;
return new ThreadLaunchNotRepositoryError({
projectId: project.id,
projectTitle: project.title,
workspaceRoot: cwd,
});
}
// `:/text` is a commit search, which cannot be asked for a commit type.
if (baseRef.startsWith(":")) return undefined;
// `git worktree add` reads `-` as the previous checkout.
if (yield* git.hasCommit({ cwd, refName: baseRef === "-" ? "@{-1}" : baseRef }))
return undefined;
Expand All @@ -275,15 +315,15 @@ const make = Effect.gen(function* () {
// `git worktree add` also starts from a remote-tracking branch of that
// name; any ref carrying it is reason enough to let Git decide.
if (yield* git.hasRefNamed({ cwd, refName: baseRef })) return undefined;
return (yield* git.hasCommit({ cwd, refName: "HEAD" })) ? project : undefined;
}).pipe(Effect.orElseSucceed(() => undefined));
if (unresolved !== undefined)
return yield* new ThreadLaunchBaseRefError({
projectId: unresolved.id,
projectTitle: unresolved.title,
workspaceRoot: unresolved.workspaceRoot,
if (!(yield* git.hasCommit({ cwd, refName: "HEAD" }))) return undefined;
return new ThreadLaunchBaseRefError({
projectId: project.id,
projectTitle: project.title,
workspaceRoot: cwd,
baseRef,
});
}).pipe(Effect.orElseSucceed(() => undefined));
if (problem !== undefined) return yield* problem;
});

const prepareInBackground = Effect.fn("ThreadLaunchService.prepareInBackground")(function* (
Expand Down
Loading
Loading