diff --git a/apps/server/src/vcs/GitVcsDriverCore.test.ts b/apps/server/src/vcs/GitVcsDriverCore.test.ts index 6e352f013fe5..a383da3d9fae 100644 --- a/apps/server/src/vcs/GitVcsDriverCore.test.ts +++ b/apps/server/src/vcs/GitVcsDriverCore.test.ts @@ -42,6 +42,53 @@ const makeNonRepositoryHandle = () => getOutputFd: () => Stream.empty, }); +const makeFailingHandle = (stderr: string, exitCode = 128) => + ChildProcessSpawner.makeHandle({ + pid: ChildProcessSpawner.ProcessId(1), + exitCode: Effect.succeed(ChildProcessSpawner.ExitCode(exitCode)), + isRunning: Effect.succeed(false), + kill: () => Effect.void, + unref: Effect.succeed(Effect.void), + stdin: Sink.drain, + stdout: Stream.empty, + stderr: Stream.encodeText(Stream.make(stderr)), + all: Stream.empty, + getInputFd: () => Sink.drain, + getOutputFd: () => Stream.empty, + }); + +/** + * A driver whose every git command exits non-zero with `stderr`. + * + * `spawns` records what was actually launched, so a test can prove the stub — + * and not real git on the developer's machine — produced the failure it is + * asserting about. + */ +const makeFailingGitLayer = (stderr: string) => { + const spawns: Array> = []; + const spawner = ChildProcessSpawner.make((command) => + Effect.sync(() => { + if (!ChildProcess.isStandardCommand(command)) { + return assert.fail("expected a standard Git command"); + } + spawns.push(command.args); + return makeFailingHandle(stderr); + }), + ); + return { + spawns, + layer: GitVcsDriver.layer.pipe( + Layer.provide(ServerConfigLayer), + Layer.provideMerge( + Layer.merge( + NodeServices.layer, + Layer.succeed(ChildProcessSpawner.ChildProcessSpawner, spawner), + ), + ), + ), + }; +}; + const makeSuccessfulHandle = (stdout: string) => ChildProcessSpawner.makeHandle({ pid: ChildProcessSpawner.ProcessId(1), @@ -635,19 +682,161 @@ it.effect("backs off failed upstream refreshes across linked worktrees", () => ).pipe(Effect.provide(ServerConfigLayer.pipe(Layer.provideMerge(NodeServices.layer)))), ); +it.effect("classifies an authentication failure without retaining the remote it names", () => { + // The failure that motivated surfacing this at all: a host with no key + // registered for the remote. Git names the remote back on stderr, which is + // why the text itself cannot travel with the error. + const token = "ghp_secret_token_value"; + const failing = makeFailingGitLayer( + `Cloning into 'projects'...\n` + + `remote: Invalid username or password for https://${token}@github.com/owner/repo.git\n` + + `git@github.com: Permission denied (publickey).\r\n` + + `fatal: Could not read from remote repository.\n`, + ); + + return Effect.scoped( + Effect.gen(function* () { + const driver = yield* GitVcsDriver.GitVcsDriver; + const cwd = yield* makeTmpDir(); + const error = yield* driver + .execute({ + operation: "GitVcsDriver.test.authFailure", + cwd, + args: ["clone", "git@github.com:owner/repo.git", "projects"], + }) + .pipe(Effect.flip); + + // Guards against the stub being bypassed and real git answering instead. + assert.equal(failing.spawns.length, 1); + assert.instanceOf(error, GitCommandError); + // The reason survives... + assert.equal(error.failureKind, "authentication"); + assert.include(error.detail.toLowerCase(), "authentication"); + // ...while nothing git wrote does. + assert.notInclude(error.detail, token); + assert.notInclude(error.message, token); + assert.notInclude(String(error.cause), token); + assert.notInclude(error.detail, "Permission denied"); + assert.isAbove(error.stderrLength ?? 0, 0); + }), + ).pipe(Effect.provide(failing.layer)); +}); + +// Git writes a bare "Permission denied" for local filesystem problems, using +// the same two words ssh uses for a rejected key. Matching the phrase alone +// would answer an unwritable directory or a stale lock with advice about +// remote credentials, which is worse than saying nothing. +const localPermissionFailures = [ + { + name: "an unwritable clone target", + args: ["init", "projects"], + stderr: `error: could not create work tree dir 'projects': Permission denied\n`, + }, + { + name: "a lock file it cannot take", + args: ["commit", "-m", "wip"], + stderr: `fatal: Unable to create '/repo/.git/index.lock': Permission denied\n`, + }, + { + name: "a ref file it cannot write", + args: ["fetch", "origin"], + stderr: `error: cannot open .git/FETCH_HEAD: Permission denied\n`, + }, +] as const; + +for (const failure of localPermissionFailures) { + it.effect(`reads ${failure.name} as a command failure, not an auth failure`, () => { + const failing = makeFailingGitLayer(failure.stderr); + + return Effect.scoped( + Effect.gen(function* () { + const driver = yield* GitVcsDriver.GitVcsDriver; + const cwd = yield* makeTmpDir(); + const error = yield* driver + .execute({ + operation: "GitVcsDriver.test.localPermissionFailure", + cwd, + args: [...failure.args], + }) + .pipe(Effect.flip); + + assert.equal(failing.spawns.length, 1); + assert.instanceOf(error, GitCommandError); + assert.equal(error.failureKind, "command-failed"); + assert.notInclude(error.detail.toLowerCase(), "authentication"); + assert.notInclude(error.detail.toLowerCase(), "credentials"); + }), + ).pipe(Effect.provide(failing.layer)); + }); +} + +it.effect("prefers not-found over the access-rights footer git pairs with it", () => { + // GitHub/GitLab SSH for a missing repo writes both lines. Matching + // "access rights" first would call that authentication. + const failing = makeFailingGitLayer( + `ERROR: Repository not found.\n` + + `fatal: Could not read from remote repository.\n` + + `Please make sure you have the correct access rights\n` + + `and the repository exists.\n`, + ); + + return Effect.scoped( + Effect.gen(function* () { + const driver = yield* GitVcsDriver.GitVcsDriver; + const cwd = yield* makeTmpDir(); + const error = yield* driver + .execute({ + operation: "GitVcsDriver.test.notFoundWithAccessRights", + cwd, + args: ["clone", "git@github.com:owner/missing.git", "projects"], + }) + .pipe(Effect.flip); + + assert.equal(failing.spawns.length, 1); + assert.instanceOf(error, GitCommandError); + assert.equal(error.failureKind, "not-found"); + assert.notInclude(error.detail.toLowerCase(), "authentication"); + assert.notInclude(error.detail.toLowerCase(), "credentials"); + }), + ).pipe(Effect.provide(failing.layer)); +}); + it.layer(TestLayer)("GitVcsDriver core integration", (it) => { describe("process environment", () => { - it.effect("preserves the caller locale for general Git subprocesses", () => + it.effect("preserves the caller locale when non-zero exits are returned raw", () => Effect.gen(function* () { const cwd = yield* makeTmpDir(); + const driver = yield* GitVcsDriver.GitVcsDriver; - const locale = yield* git( + // allowNonZeroExit skips classification, so the caller's locale rides + // through with the raw stdout/stderr instead of being forced to C. + const result = yield* driver.execute({ + operation: "GitVcsDriver.test.printLocale", cwd, - ["-c", 'alias.print-locale=!printf "%s" "$LC_ALL"', "print-locale"], - { LC_ALL: "zh_CN.UTF-8" }, - ); + args: ["-c", 'alias.print-locale=!printf "%s" "$LC_ALL"', "print-locale"], + env: { LC_ALL: "zh_CN.UTF-8" }, + allowNonZeroExit: true, + timeoutMs: 10_000, + }); + + assert.equal(result.stdout.trim(), "zh_CN.UTF-8"); + }), + ); + + it.effect("forces a stable locale when a failure will be classified", () => + Effect.gen(function* () { + const cwd = yield* makeTmpDir(); + const driver = yield* GitVcsDriver.GitVcsDriver; + + const result = yield* driver.execute({ + operation: "GitVcsDriver.test.printLocaleClassified", + cwd, + args: ["-c", 'alias.print-locale=!printf "%s" "$LC_ALL"', "print-locale"], + env: { LC_ALL: "zh_CN.UTF-8" }, + timeoutMs: 10_000, + }); - assert.equal(locale, "zh_CN.UTF-8"); + assert.equal(result.stdout.trim(), "C"); }), ); }); @@ -712,6 +901,13 @@ it.layer(TestLayer)("GitVcsDriver core integration", (it) => { assert.notInclude(error.message, secret); assert.notProperty(error, "args"); assert.notProperty(error, "stderr"); + // `cause` is part of the RPC error schema, so it reaches clients just + // like the direct attributes do. Git echoes the offending argument back + // on stderr, so nothing derived from that text may land here. + assert.notInclude(String(error.cause), secret); + // The failure stays classifiable without retaining the text that + // classified it. + assert.oneOf(error.failureKind, ["authentication", "not-found", "command-failed"]); }), ); diff --git a/apps/server/src/vcs/GitVcsDriverCore.ts b/apps/server/src/vcs/GitVcsDriverCore.ts index d39817c0ee1d..02fdc9360e63 100644 --- a/apps/server/src/vcs/GitVcsDriverCore.ts +++ b/apps/server/src/vcs/GitVcsDriverCore.ts @@ -24,6 +24,7 @@ import { type ReviewDiffFileContentsInput, type ReviewDiffPreviewInput, type ReviewDiffPreviewSource, + type VcsProcessExitFailureKind, type VcsRef, } from "@t3tools/contracts"; import { dedupeRemoteBranchesWithLocalMatches, normalizeGitRemoteUrl } from "@t3tools/shared/git"; @@ -396,6 +397,97 @@ function parseDefaultBranchFromRemoteHeadRef(value: string, remoteName: string): return refName.length > 0 ? refName : null; } +/** + * Normalized category for a failing git command. + * + * Reuses the vocabulary `VcsProcess.classifyNonZeroExit` already established, + * so the reason a command failed survives as a bounded, non-secret-bearing + * value. Reading stderr to produce it is safe; keeping stderr is not, so the + * text is read here and goes no further. + */ +const classifyGitFailure = (stderr: string): VcsProcessExitFailureKind => { + const normalized = stderr.toLowerCase(); + if ( + normalized.includes("authentication failed") || + normalized.includes("could not read username") || + normalized.includes("could not read password") || + normalized.includes("invalid username or password") || + normalized.includes("authentication required") || + // ssh names the methods it tried: "Permission denied (publickey)." A bare + // "permission denied" is a local filesystem error — `git init` into an + // unwritable directory reaches this function too, and telling someone to + // check their remote credentials would bury the actual problem. + normalized.includes("permission denied (") || + // GitHub over https: "remote: Permission to owner/repo.git denied to user." + /remote:[^\n]*permission to [^\n]*denied/.test(normalized) + ) { + return "authentication"; + } + // Before the generic "access rights" footer: GitHub/GitLab SSH failures for a + // missing repo include both "Repository not found" and "correct access rights", + // and the actionable classification is not-found. + if (normalized.includes("not found") || normalized.includes("does not exist")) { + return "not-found"; + } + // "...make sure you have the correct access rights and the repository exists." + if (normalized.includes("access rights")) { + return "authentication"; + } + return "command-failed"; +}; + +/** + * Caller-facing `detail` for a failing git command. + * + * Mirrors `VcsProcessExitError.fromProcessExit`: the classification is what + * makes the failure actionable, so it is turned into a fixed sentence rather + * than left for the caller to infer from an exit code. Every branch returns a + * constant — nothing derived from git's output crosses this boundary. + */ +const detailForGitFailure = (failureKind: VcsProcessExitFailureKind, fallback: string): string => { + switch (failureKind) { + case "authentication": + return "Git authentication failed. Check the credentials available to this host for the remote."; + case "not-found": + // Deliberately covers paths too: the match is broad enough to catch + // `path 'x' does not exist in 'HEAD'`, so the sentence must not promise + // the missing thing was a repository. + return "Git could not find something the command referred to — a repository, remote, ref, or path."; + case "command-failed": + return fallback; + } +}; + +/** + * Log annotation for a failing git command. + * + * Bounded and safe by construction — a category plus counts, never the output + * itself. Git echoes back its arguments and any hook output, so `stdout` and + * `stderr` can carry credentials and are capped only by `maxOutputBytes` + * (megabytes at some call sites); a log payload has to be as safe as a direct + * error attribute. + */ +const logFailedGitCommandOutput = ( + command: { + readonly operation: string; + readonly cwd: string; + readonly args: ReadonlyArray; + }, + exitCode: number | null, + failureKind: VcsProcessExitFailureKind, + stdout: string, + stderr: string, +): Effect.Effect => + Effect.logDebug( + `GitVcsDriver.commandFailed: ${command.operation} in ${command.cwd} ` + + `exited ${exitCode ?? "null"} (${command.args.length} arguments)`, + { + failureKind, + stdoutLength: stdout.length, + stderrLength: stderr.length, + }, + ); + function isMissingGitCwdError(error: GitCommandError): boolean { if (!(error.cause instanceof PlatformError.PlatformError)) { return false; @@ -728,6 +820,10 @@ export const makeGitVcsDriverCore = Effect.fn("makeGitVcsDriverCore")(function* }), ), ); + // Classified failures match English stderr. When this path will turn a + // non-zero exit into a failureKind, force a stable locale so translated + // git text cannot miss every heuristic. allowNonZeroExit callers get + // the caller's locale back with the raw output instead. const child = yield* commandSpawner .spawn( ChildProcess.make("git", commandInput.args, { @@ -735,6 +831,7 @@ export const makeGitVcsDriverCore = Effect.fn("makeGitVcsDriverCore")(function* env: { ...process.env, ...input.env, + ...(input.allowNonZeroExit ? {} : { LC_ALL: "C" }), ...trace2Monitor.env, }, }), @@ -794,10 +891,19 @@ export const makeGitVcsDriverCore = Effect.fn("makeGitVcsDriverCore")(function* yield* trace2Monitor.flush; if (!input.allowNonZeroExit && exitCode !== 0) { + const failureKind = classifyGitFailure(stderr.text); + yield* logFailedGitCommandOutput( + commandInput, + exitCode, + failureKind, + stdout.text, + stderr.text, + ); return yield* new GitCommandError({ ...gitCommandContext(commandInput), - detail: "Git command exited with a non-zero status.", + detail: detailForGitFailure(failureKind, "Git command exited with a non-zero status."), exitCode, + failureKind, stdoutLength: stdout.text.length, stderrLength: stderr.text.length, }); @@ -860,8 +966,13 @@ export const makeGitVcsDriverCore = Effect.fn("makeGitVcsDriverCore")(function* operation, cwd, args, + // execute always allows non-zero so this wrapper can classify; force a + // stable locale whenever that classification will run (see executeRaw). + env: { + ...options.env, + ...(options.allowNonZeroExit ? {} : { LC_ALL: "C" }), + }, ...(options.stdin !== undefined ? { stdin: options.stdin } : {}), - ...(options.env !== undefined ? { env: options.env } : {}), allowNonZeroExit: true, ...(options.timeoutMs !== undefined ? { timeoutMs: options.timeoutMs } : {}), ...(options.maxOutputBytes !== undefined ? { maxOutputBytes: options.maxOutputBytes } : {}), @@ -874,14 +985,29 @@ export const makeGitVcsDriverCore = Effect.fn("makeGitVcsDriverCore")(function* if (options.allowNonZeroExit || result.exitCode === 0) { return Effect.succeed(result); } - return Effect.fail( - new GitCommandError({ - ...gitCommandContext({ operation, cwd, args }), - detail: options.fallbackErrorDetail ?? "Git command exited with a non-zero status.", - ...(result.exitCode === null ? {} : { exitCode: result.exitCode }), - stdoutLength: result.stdout.length, - stderrLength: result.stderr.length, - }), + const failureKind = classifyGitFailure(result.stderr); + return logFailedGitCommandOutput( + { operation, cwd, args }, + result.exitCode, + failureKind, + result.stdout, + result.stderr, + ).pipe( + Effect.andThen( + Effect.fail( + new GitCommandError({ + ...gitCommandContext({ operation, cwd, args }), + detail: detailForGitFailure( + failureKind, + options.fallbackErrorDetail ?? "Git command exited with a non-zero status.", + ), + ...(result.exitCode === null ? {} : { exitCode: result.exitCode }), + failureKind, + stdoutLength: result.stdout.length, + stderrLength: result.stderr.length, + }), + ), + ), ); }), ); diff --git a/packages/contracts/src/git.ts b/packages/contracts/src/git.ts index 2e0552740a6c..be7feee2f87c 100644 --- a/packages/contracts/src/git.ts +++ b/packages/contracts/src/git.ts @@ -1,7 +1,7 @@ import * as Schema from "effect/Schema"; import { NonNegativeInt, PositiveInt, ThreadId, TrimmedNonEmptyString } from "./baseSchemas.ts"; import { SourceControlProviderError, SourceControlProviderInfo } from "./sourceControl.ts"; -import { VcsDriverKind } from "./vcs.ts"; +import { VcsDriverKind, VcsProcessExitFailureKind } from "./vcs.ts"; const TrimmedNonEmptyStringSchema = TrimmedNonEmptyString; const GIT_LIST_BRANCHES_MAX_LIMIT = 200; @@ -327,6 +327,7 @@ export class GitCommandError extends Schema.TaggedErrorClass()( cwd: Schema.String, argumentCount: Schema.optional(Schema.Number), exitCode: Schema.optional(Schema.Number), + failureKind: Schema.optional(VcsProcessExitFailureKind), stdoutLength: Schema.optional(Schema.Number), stderrLength: Schema.optional(Schema.Number), outputLength: Schema.optional(Schema.Number),