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
273 changes: 273 additions & 0 deletions apps/server/src/vcs/GitVcsDriverCore.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -712,6 +712,279 @@ it.layer(TestLayer)("GitVcsDriver core integration", (it) => {
assert.notInclude(error.message, secret);
assert.notProperty(error, "args");
assert.notProperty(error, "stderr");
assert.notProperty(error, "reason");
}),
);

it.effect("names the branch conflict behind a failed worktree add", () =>
Effect.gen(function* () {
const parent = yield* makeTmpDir();
const pathService = yield* Path.Path;
const cwd = pathService.join(parent, "repo");
const fileSystem = yield* FileSystem.FileSystem;
yield* fileSystem.makeDirectory(cwd);
const driver = yield* GitVcsDriver.GitVcsDriver;
yield* initRepoWithCommit(cwd);
const firstWorktree = pathService.join(parent, "first");
yield* git(cwd, ["worktree", "add", firstWorktree, "-b", "shared-branch"]);

const error = yield* driver
.execute({
operation: "GitVcsDriver.test.worktreeAddConflict",
cwd,
args: ["worktree", "add", pathService.join(parent, "second"), "shared-branch"],
env: { LC_ALL: "C" },
})
.pipe(Effect.flip);

assert.deepInclude(error, {
_tag: "GitCommandError",
operation: "GitVcsDriver.test.worktreeAddConflict",
reason: "branch_checked_out_in_worktree",
});
assert.include(error.message, "(branch_checked_out_in_worktree)");
assert.notInclude(error.message, firstWorktree);
}),
);

it.effect("names a missing repository behind a failed command", () =>
Effect.gen(function* () {
const cwd = yield* makeTmpDir();
const driver = yield* GitVcsDriver.GitVcsDriver;

const error = yield* driver
.execute({
operation: "GitVcsDriver.test.notARepository",
cwd,
args: ["rev-parse", "--abbrev-ref", "HEAD"],
env: { LC_ALL: "C" },
})
.pipe(Effect.flip);

assert.deepInclude(error, {
_tag: "GitCommandError",
operation: "GitVcsDriver.test.notARepository",
reason: "not_a_repository",
});
assert.include(error.message, "(not_a_repository)");
}),
);

it.effect("appends the reason tag to a caller-supplied detail", () =>
Effect.gen(function* () {
const parent = yield* makeTmpDir();
const pathService = yield* Path.Path;
const cwd = pathService.join(parent, "repo");
const fileSystem = yield* FileSystem.FileSystem;
yield* fileSystem.makeDirectory(cwd);
const driver = yield* GitVcsDriver.GitVcsDriver;
yield* initRepoWithCommit(cwd);
yield* git(cwd, [
"worktree",
"add",
pathService.join(parent, "taken"),
"-b",
"taken-branch",
]);

const error = yield* driver
.createWorktree({
cwd,
refName: "taken-branch",
path: pathService.join(parent, "second"),
})
.pipe(Effect.flip);

assert.equal(error.detail, "git worktree add failed");
assert.include(error.message, "git worktree add failed (branch_checked_out_in_worktree)");
}),
);

for (const { label, sshMessage, expectedReason, expectedText } of [
{
label: "a refused key",
sshMessage: "git@example.invalid: Permission denied (publickey).",
expectedReason: "authentication_failed" as const,
expectedText: "(authentication_failed)",
},
{
label: "a refused key among several methods",
sshMessage: "git@example.invalid: Permission denied (publickey,password).",
expectedReason: "authentication_failed" as const,
expectedText: "(authentication_failed)",
},
{
label: "a refused keyboard-interactive attempt",
sshMessage: "git@example.invalid: Permission denied (keyboard-interactive).",
expectedReason: "authentication_failed" as const,
expectedText: "(authentication_failed)",
},
{
label: "a rejected password",
sshMessage: "Permission denied, please try again.",
expectedReason: "authentication_failed" as const,
expectedText: "(authentication_failed)",
},
{
label: "an untrusted host key",
sshMessage: "Host key verification failed.",
expectedReason: "host_key_unverified" as const,
expectedText: "(host_key_unverified)",
},
]) {
it.effect(`names ${label} rather than an unreachable remote`, () =>
Effect.gen(function* () {
const parent = yield* makeTmpDir();
const pathService = yield* Path.Path;
const cwd = pathService.join(parent, "repo");
const fileSystem = yield* FileSystem.FileSystem;
const driver = yield* GitVcsDriver.GitVcsDriver;
yield* fileSystem.makeDirectory(cwd);
yield* initRepoWithCommit(cwd);
yield* git(cwd, ["remote", "add", "origin", "git@example.invalid:owner/repo.git"]);
// Stands in for ssh, which prints its refusal unprefixed and leaves
// git to add only a generic "could not read" line after it.
const fakeSsh = pathService.join(parent, "fake-ssh");
yield* writeTextFile(
parent,
"fake-ssh",
`#!/bin/sh\necho "${sshMessage}" >&2\nexit 255\n`,
);
yield* fileSystem.chmod(fakeSsh, 0o755);

const error = yield* driver
.execute({
operation: "GitVcsDriver.test.sshRefusal",
cwd,
args: ["fetch", "origin"],
env: { LC_ALL: "C", GIT_SSH_COMMAND: fakeSsh },
})
.pipe(Effect.flip);

assert.deepInclude(error, { reason: expectedReason });
assert.include(error.message, expectedText);
}),
);
}

it.effect("does not read a filesystem permission error as an ssh refusal", () =>
Effect.gen(function* () {
const parent = yield* makeTmpDir();
const pathService = yield* Path.Path;
const cwd = pathService.join(parent, "repo");
const fileSystem = yield* FileSystem.FileSystem;
const driver = yield* GitVcsDriver.GitVcsDriver;
yield* fileSystem.makeDirectory(cwd);
yield* initRepoWithCommit(cwd);
yield* git(cwd, ["remote", "add", "origin", "git@example.invalid:owner/repo.git"]);
const fakeSsh = pathService.join(parent, "fake-ssh");
yield* writeTextFile(
parent,
"fake-ssh",
'#!/bin/sh\necho "fatal: cannot open backup file: Permission denied (os error 13)" >&2\nexit 255\n',
);
yield* fileSystem.chmod(fakeSsh, 0o755);

const error = yield* driver
.execute({
operation: "GitVcsDriver.test.osPermission",
cwd,
args: ["fetch", "origin"],
env: { LC_ALL: "C", GIT_SSH_COMMAND: fakeSsh },
})
.pipe(Effect.flip);

assert.notInclude(error.message, "(authentication_failed)");
}),
);

it.effect("does not read remote hook output as a git failure reason", () =>
Effect.gen(function* () {
const parent = yield* makeTmpDir();
const pathService = yield* Path.Path;
const remote = pathService.join(parent, "origin.git");
const cwd = pathService.join(parent, "work");
const fileSystem = yield* FileSystem.FileSystem;
const driver = yield* GitVcsDriver.GitVcsDriver;
yield* fileSystem.makeDirectory(cwd);
yield* initRepoWithCommit(cwd);
yield* git(cwd, ["init", "--bare", remote]);
yield* git(cwd, ["remote", "add", "origin", remote]);
// Git prefixes everything the server says with `remote:`, so a remote
// hook can put arbitrary text in front of the classifier.
const preReceive = pathService.join(remote, "hooks", "pre-receive");
yield* writeTextFile(
remote,
"hooks/pre-receive",
'#!/bin/sh\necho "authentication failed" >&2\nexit 1\n',
);
yield* fileSystem.chmod(preReceive, 0o755);

const error = yield* driver
.execute({
operation: "GitVcsDriver.test.remoteHookOutput",
cwd,
args: ["push", "origin", "HEAD"],
env: { LC_ALL: "C" },
})
.pipe(Effect.flip);

assert.notProperty(error, "reason");
assert.notInclude(error.message, "(authentication_failed)");
}),
);

it.effect("does not read hook output as a git failure reason", () =>
Effect.gen(function* () {
const parent = yield* makeTmpDir();
const pathService = yield* Path.Path;
const remote = pathService.join(parent, "origin.git");
const cwd = pathService.join(parent, "work");
const fileSystem = yield* FileSystem.FileSystem;
const driver = yield* GitVcsDriver.GitVcsDriver;
yield* fileSystem.makeDirectory(cwd);
yield* initRepoWithCommit(cwd);
yield* git(cwd, ["init", "--bare", remote]);
yield* git(cwd, ["remote", "add", "origin", remote]);
yield* writeTextFile(
cwd,
".git/hooks/pre-push",
'#!/bin/sh\necho "authentication failed" >&2\nexit 1\n',
);
yield* fileSystem.chmod(pathService.join(cwd, ".git/hooks/pre-push"), 0o755);

const error = yield* driver
.execute({
operation: "GitVcsDriver.test.hookOutput",
cwd,
args: ["push", "origin", "HEAD"],
env: { LC_ALL: "C" },
})
.pipe(Effect.flip);

assert.notProperty(error, "reason");
assert.notInclude(error.message, "(authentication_failed)");
}),
);

it.effect("leaves a tag collision unclassified rather than calling it a path", () =>
Effect.gen(function* () {
const cwd = yield* makeTmpDir();
const driver = yield* GitVcsDriver.GitVcsDriver;
yield* initRepoWithCommit(cwd);
yield* git(cwd, ["tag", "v1"]);

const error = yield* driver
.execute({
operation: "GitVcsDriver.test.tagCollision",
cwd,
args: ["tag", "v1"],
env: { LC_ALL: "C" },
})
.pipe(Effect.flip);

assert.notProperty(error, "reason");
assert.notInclude(error.message, "(path_already_exists)");
}),
);

Expand Down
61 changes: 61 additions & 0 deletions apps/server/src/vcs/GitVcsDriverCore.ts
Comment thread
macroscopeapp[bot] marked this conversation as resolved.
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import { ChildProcess, ChildProcessSpawner } from "effect/unstable/process";

import {
GitCommandError,
type GitCommandFailureReason,
type ReviewDiffFileContentsInput,
type ReviewDiffPreviewInput,
type ReviewDiffPreviewSource,
Expand Down Expand Up @@ -391,6 +392,62 @@ function gitCommandContext(
} as const;
}

// Git states the actual cause on stderr, but stderr never leaves this module:
// it echoes argv and remote URLs, which can carry credentials. Matching it
// against fixed patterns yields a tag the caller can act on and a message that
// quotes none of the matched text. Patterns run in order, specific first.
const GIT_FAILURE_REASON_PATTERNS: ReadonlyArray<readonly [RegExp, GitCommandFailureReason]> = [
[/would clobber existing tag/i, "tag_would_be_clobbered"],
[/is already (?:used by worktree at|checked out at)/i, "branch_checked_out_in_worktree"],
[/a branch named .+ already exists/i, "branch_already_exists"],
// ssh names the methods it tried, so the parenthesized list varies:
// `(publickey)`, `(publickey,password)`, `(keyboard-interactive)`.
[
/(?:authentication failed|could not read Username|could not read Password|permission denied \([a-z-]+(?:,[a-z-]+)*\)|permission denied, please try again)/i,
"authentication_failed",
],
// Distinct from a credential failure: the remote answered and the key it
// presented is untrusted, so pointing the user at credentials would misdirect.
[/host key verification failed/i, "host_key_unverified"],
[
/(?:could not read from remote repository|does not appear to be a git repository|repository .+ not found)/i,
"remote_unreachable",
],
// Quoted-path forms only: an unquoted `fatal: <thing> already exists` also
// covers tag and ref collisions, which are not path collisions.
[/fatal: '[^']+' already exists|destination path .+ already exists/i, "path_already_exists"],
];

// Hooks write to the same stream git does, and nothing distinguishes their
// text from git's: a pre-push hook echoing "authentication failed" would
// otherwise be read as a credential failure. Git prefixes its own diagnostics
// and states a push rejection on a `! [rejected]` line, so only those are
// classified. `remote:` is deliberately excluded — git prefixes every byte the
// server sends that way, remote hook output included, so trusting it would
// reintroduce the same false positive from the other end of the connection.
const GIT_DIAGNOSTIC_LINE_PATTERN = /^(?:fatal|error):|^!\s|^\s+!\s/;
// ssh reports the refusal itself, unprefixed, and git only adds a generic
// "Could not read from remote repository" after it. Dropping ssh's line would
// leave that generic one to be read as an unreachable remote when the real
// cause is credentials, so these specific refusals are classified too.
const SSH_TRANSPORT_REFUSAL_PATTERN =
/Permission denied \((?:publickey|password|keyboard-interactive)|Permission denied, please try again|Host key verification failed/i;
Comment thread
cursor[bot] marked this conversation as resolved.

function classifyGitFailure(stderr: string): GitCommandFailureReason | null {
const diagnostics = stderr
.split(/\r?\n/)
.filter(
(line) => GIT_DIAGNOSTIC_LINE_PATTERN.test(line) || SSH_TRANSPORT_REFUSAL_PATTERN.test(line),
)
.join("\n");
if (diagnostics.length === 0) return null;
if (isNonRepositoryGitStderr(diagnostics)) return "not_a_repository";
for (const [pattern, reason] of GIT_FAILURE_REASON_PATTERNS) {
if (pattern.test(diagnostics)) return reason;
Comment thread
cursor[bot] marked this conversation as resolved.
}
return null;
}

function parseDefaultBranchFromRemoteHeadRef(value: string, remoteName: string): string | null {
const trimmed = value.trim();
const prefix = `refs/remotes/${remoteName}/`;
Expand Down Expand Up @@ -811,8 +868,10 @@ export const makeGitVcsDriverCore = Effect.fn("makeGitVcsDriverCore")(function*
yield* trace2Monitor.flush;

if (!input.allowNonZeroExit && exitCode !== 0) {
const reason = classifyGitFailure(stderr.text);
return yield* new GitCommandError({
...gitCommandContext(commandInput),
...(reason === null ? {} : { reason }),
detail: "Git command exited with a non-zero status.",
exitCode,
stdoutLength: stdout.text.length,
Expand Down Expand Up @@ -895,9 +954,11 @@ export const makeGitVcsDriverCore = Effect.fn("makeGitVcsDriverCore")(function*
if (options.allowNonZeroExit || result.exitCode === 0) {
return Effect.succeed(result);
}
const reason = classifyGitFailure(result.stderr);
Comment thread
macroscopeapp[bot] marked this conversation as resolved.
return Effect.fail(
new GitCommandError({
...gitCommandContext({ operation, cwd, args }),
...(reason === null ? {} : { reason }),
detail: options.fallbackErrorDetail ?? "Git command exited with a non-zero status.",
...(result.exitCode === null ? {} : { exitCode: result.exitCode }),
stdoutLength: result.stdout.length,
Expand Down
Loading
Loading