From 61b898117f28bef778989fbcde49c50a4d0b5c69 Mon Sep 17 00:00:00 2001 From: Walid Baharwal Date: Sat, 29 Aug 2026 13:11:13 +0500 Subject: [PATCH] fix(server): name the cause of a failed git command MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Git states why a command failed on stderr, but stderr is deliberately kept off GitCommandError — it echoes argv and remote URLs, which can carry credentials. Callers were left with "git fetch origin failed" and no way to tell a tag conflict from an auth failure without re-running git by hand. Match stderr at the driver against a fixed set of well-known failures and carry the result as a closed set of diagnostic tags, appended to the message the way EnvironmentInternalError does it. The tag names the cause; it selects no text, so nothing matched from stderr is ever quoted and the existing redaction guarantee is unchanged. Only git's own diagnostic lines are classified, plus the refusals ssh prints. Hooks write to the same stream unprefixed, so a pre-push hook echoing "authentication failed" would otherwise be reported as a credential failure. ssh states its refusal unprefixed too, naming whichever methods it tried, and git adds only a generic "could not read from remote repository" afterwards — so every refusal shape is matched, and an untrusted host key gets its own tag rather than being blamed on credentials. Fixes #4380 --- apps/server/src/vcs/GitVcsDriverCore.test.ts | 273 +++++++++++++++++++ apps/server/src/vcs/GitVcsDriverCore.ts | 61 +++++ packages/contracts/src/git.ts | 21 +- 3 files changed, 354 insertions(+), 1 deletion(-) diff --git a/apps/server/src/vcs/GitVcsDriverCore.test.ts b/apps/server/src/vcs/GitVcsDriverCore.test.ts index 587a3e4abbde..6f1d9f0743cc 100644 --- a/apps/server/src/vcs/GitVcsDriverCore.test.ts +++ b/apps/server/src/vcs/GitVcsDriverCore.test.ts @@ -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)"); }), ); diff --git a/apps/server/src/vcs/GitVcsDriverCore.ts b/apps/server/src/vcs/GitVcsDriverCore.ts index 71e478cbaa3d..17efe00070c7 100644 --- a/apps/server/src/vcs/GitVcsDriverCore.ts +++ b/apps/server/src/vcs/GitVcsDriverCore.ts @@ -21,6 +21,7 @@ import { ChildProcess, ChildProcessSpawner } from "effect/unstable/process"; import { GitCommandError, + type GitCommandFailureReason, type ReviewDiffFileContentsInput, type ReviewDiffPreviewInput, type ReviewDiffPreviewSource, @@ -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 = [ + [/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: 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; + +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; + } + return null; +} + function parseDefaultBranchFromRemoteHeadRef(value: string, remoteName: string): string | null { const trimmed = value.trim(); const prefix = `refs/remotes/${remoteName}/`; @@ -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, @@ -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); 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, diff --git a/packages/contracts/src/git.ts b/packages/contracts/src/git.ts index 915c3627c9b9..302e56cef400 100644 --- a/packages/contracts/src/git.ts +++ b/packages/contracts/src/git.ts @@ -335,6 +335,23 @@ export const VcsPullResult = Schema.Struct({ export type VcsPullResult = typeof VcsPullResult.Type; // RPC / domain errors + +// Well-known git failures, recognized from stderr at the driver and carried as +// a closed set of diagnostic tags. Git's stderr itself stays off the error: it +// echoes argv and remote URLs, which can hold credentials. The tag names the +// cause for logs and callers; it does not select a message. +export const GitCommandFailureReason = Schema.Literals([ + "authentication_failed", + "branch_already_exists", + "branch_checked_out_in_worktree", + "host_key_unverified", + "not_a_repository", + "path_already_exists", + "remote_unreachable", + "tag_would_be_clobbered", +]); +export type GitCommandFailureReason = typeof GitCommandFailureReason.Type; + export class GitCommandError extends Schema.TaggedErrorClass()("GitCommandError", { operation: Schema.String, command: Schema.String, @@ -344,11 +361,13 @@ export class GitCommandError extends Schema.TaggedErrorClass()( stdoutLength: Schema.optional(Schema.Number), stderrLength: Schema.optional(Schema.Number), outputLength: Schema.optional(Schema.Number), + reason: Schema.optional(GitCommandFailureReason), detail: Schema.String, cause: Schema.optional(Schema.Defect()), }) { override get message(): string { - return `Git command failed in ${this.operation} (${this.cwd}): ${this.detail}`; + const reason = this.reason === undefined ? "" : ` (${this.reason})`; + return `Git command failed in ${this.operation} (${this.cwd}): ${this.detail}${reason}`; } }