From a82ecaee9f8414ebd28a77001ab74fe6f178cab4 Mon Sep 17 00:00:00 2001 From: Astrid Gealer Date: Fri, 7 Aug 2026 15:03:03 +0100 Subject: [PATCH 1/7] fix(vcs): surface git's stderr on GitCommandError MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A failing git command records only `stdoutLength` and `stderrLength` — how much git wrote, never what it wrote. The text is captured and then dropped, so the reason exists nowhere: not in the error, not in the RPC response, and not in the server log (clone failures are returned to the caller rather than logged). In practice this turns an ordinary, self-explanatory git failure into an opaque one. A clone whose remote refuses the key surfaces as "The source control operation could not be completed", with git's own "Permission denied (publickey)" discarded a few frames earlier. Add an optional `stderrTail` carrying the end of stderr, and include it in the error message. Truncated from the end, since git puts the reason on its last lines behind a long transfer log, and credential-bearing URLs are redacted because git echoes back the remote it was handed. Co-Authored-By: Claude Opus 5 (1M context) --- apps/server/src/vcs/GitVcsDriverCore.ts | 19 +++++++++++++++++++ packages/contracts/src/git.ts | 9 ++++++++- 2 files changed, 27 insertions(+), 1 deletion(-) diff --git a/apps/server/src/vcs/GitVcsDriverCore.ts b/apps/server/src/vcs/GitVcsDriverCore.ts index d39817c0ee1d..ff62c2ae2da4 100644 --- a/apps/server/src/vcs/GitVcsDriverCore.ts +++ b/apps/server/src/vcs/GitVcsDriverCore.ts @@ -396,6 +396,23 @@ function parseDefaultBranchFromRemoteHeadRef(value: string, remoteName: string): return refName.length > 0 ? refName : null; } +/** + * Tail of git's stderr for `GitCommandError.stderrTail`. + * + * Truncated from the end, because git puts the reason on its last lines and a + * long transfer log in front of it. Credential-bearing URLs are redacted: + * git echoes back the remote it was handed, which may embed a token. + */ +const GIT_STDERR_TAIL_LIMIT = 2000; + +function gitStderrTail(stderr: string): string | undefined { + const redacted = stderr.replace(/\/\/([^/@:\s]+):([^@\s]+)@/g, "//$1:***@").trim(); + if (redacted.length === 0) return undefined; + return redacted.length > GIT_STDERR_TAIL_LIMIT + ? `…${redacted.slice(-GIT_STDERR_TAIL_LIMIT)}` + : redacted; +} + function isMissingGitCwdError(error: GitCommandError): boolean { if (!(error.cause instanceof PlatformError.PlatformError)) { return false; @@ -800,6 +817,7 @@ export const makeGitVcsDriverCore = Effect.fn("makeGitVcsDriverCore")(function* exitCode, stdoutLength: stdout.text.length, stderrLength: stderr.text.length, + stderrTail: gitStderrTail(stderr.text), }); } @@ -881,6 +899,7 @@ export const makeGitVcsDriverCore = Effect.fn("makeGitVcsDriverCore")(function* ...(result.exitCode === null ? {} : { exitCode: result.exitCode }), stdoutLength: result.stdout.length, stderrLength: result.stderr.length, + stderrTail: gitStderrTail(result.stderr), }), ); }), diff --git a/packages/contracts/src/git.ts b/packages/contracts/src/git.ts index 2e0552740a6c..a5900e3a7f07 100644 --- a/packages/contracts/src/git.ts +++ b/packages/contracts/src/git.ts @@ -330,11 +330,18 @@ export class GitCommandError extends Schema.TaggedErrorClass()( stdoutLength: Schema.optional(Schema.Number), stderrLength: Schema.optional(Schema.Number), outputLength: Schema.optional(Schema.Number), + /** + * Tail of git's stderr, truncated and with credential-bearing URLs + * redacted. Without it a failed git command is undiagnosable from outside + * the process: the lengths above say how much git wrote, never what. + */ + stderrTail: Schema.optional(Schema.String), detail: Schema.String, cause: Schema.optional(Schema.Defect()), }) { override get message(): string { - return `Git command failed in ${this.operation} (${this.cwd}): ${this.detail}`; + const summary = `Git command failed in ${this.operation} (${this.cwd}): ${this.detail}`; + return this.stderrTail === undefined ? summary : `${summary}\n${this.stderrTail}`; } } From e06d24cca591bfe16b6d4694a2da93471ea70c17 Mon Sep 17 00:00:00 2001 From: Astrid Gealer Date: Fri, 7 Aug 2026 15:10:04 +0100 Subject: [PATCH 2/7] fix(vcs): log a failing git command's output at debug level Addresses review: drop `stderrTail` and the message change entirely, and log the output at the failure site instead. The first attempt put git's stderr on `GitCommandError` as a bounded, redacted attribute. That was wrong on three counts, all correctly flagged: it reintroduces raw command output into a value that crosses RPC, UI and persistence boundaries; it makes `message` unbounded rather than derived from stable structural attributes; and the redaction only covered `//user:pass@` remotes, missing single-token URLs, query-string tokens, and anything git echoes back from an argument or hook. It also regressed the existing test asserting a secret passed as a git argument never reaches `error.message`, and effectively reverted #3253, which removed stderr from this error for exactly these reasons. The error's public shape is now untouched. Both non-zero-exit sites call `Effect.logDebug` with the command context and its stdout/stderr, so the diagnostic is reachable when someone goes looking without becoming part of the serialized error. Co-Authored-By: Claude Opus 5 (1M context) --- apps/server/src/vcs/GitVcsDriverCore.ts | 67 ++++++++++++++++--------- packages/contracts/src/git.ts | 9 +--- 2 files changed, 45 insertions(+), 31 deletions(-) diff --git a/apps/server/src/vcs/GitVcsDriverCore.ts b/apps/server/src/vcs/GitVcsDriverCore.ts index ff62c2ae2da4..7b7aa3f6b9ec 100644 --- a/apps/server/src/vcs/GitVcsDriverCore.ts +++ b/apps/server/src/vcs/GitVcsDriverCore.ts @@ -397,21 +397,34 @@ function parseDefaultBranchFromRemoteHeadRef(value: string, remoteName: string): } /** - * Tail of git's stderr for `GitCommandError.stderrTail`. + * Record what a failing git command actually printed, at debug level. * - * Truncated from the end, because git puts the reason on its last lines and a - * long transfer log in front of it. Credential-bearing URLs are redacted: - * git echoes back the remote it was handed, which may embed a token. + * `GitCommandError` keeps only bounded attributes — lengths, not text — and + * deliberately so: git echoes back its arguments and any hook output, so the + * text can carry credentials, and #3253 removed it from the error for exactly + * that reason. But dropping it everywhere leaves a failing git command + * undiagnosable from outside the process: neither the error, the RPC response, + * nor the server log says why git exited non-zero. + * + * Logging it at debug keeps the diagnostic reachable when someone goes looking, + * without putting unbounded, possibly secret-bearing output into a value that + * crosses RPC, UI, and persistence boundaries. */ -const GIT_STDERR_TAIL_LIMIT = 2000; - -function gitStderrTail(stderr: string): string | undefined { - const redacted = stderr.replace(/\/\/([^/@:\s]+):([^@\s]+)@/g, "//$1:***@").trim(); - if (redacted.length === 0) return undefined; - return redacted.length > GIT_STDERR_TAIL_LIMIT - ? `…${redacted.slice(-GIT_STDERR_TAIL_LIMIT)}` - : redacted; -} +const logFailedGitCommandOutput = ( + command: { + readonly operation: string; + readonly cwd: string; + readonly args: ReadonlyArray; + }, + exitCode: number | null, + stdout: string, + stderr: string, +): Effect.Effect => + Effect.logDebug( + `GitVcsDriver.commandFailed: ${command.operation} in ${command.cwd} ` + + `exited ${exitCode ?? "null"} (${command.args.length} arguments)`, + { stdout, stderr }, + ); function isMissingGitCwdError(error: GitCommandError): boolean { if (!(error.cause instanceof PlatformError.PlatformError)) { @@ -811,13 +824,13 @@ export const makeGitVcsDriverCore = Effect.fn("makeGitVcsDriverCore")(function* yield* trace2Monitor.flush; if (!input.allowNonZeroExit && exitCode !== 0) { + yield* logFailedGitCommandOutput(commandInput, exitCode, stdout.text, stderr.text); return yield* new GitCommandError({ ...gitCommandContext(commandInput), detail: "Git command exited with a non-zero status.", exitCode, stdoutLength: stdout.text.length, stderrLength: stderr.text.length, - stderrTail: gitStderrTail(stderr.text), }); } @@ -892,15 +905,23 @@ 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, - stderrTail: gitStderrTail(result.stderr), - }), + return logFailedGitCommandOutput( + { operation, cwd, args }, + result.exitCode, + result.stdout, + result.stderr, + ).pipe( + Effect.andThen( + 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, + }), + ), + ), ); }), ); diff --git a/packages/contracts/src/git.ts b/packages/contracts/src/git.ts index a5900e3a7f07..2e0552740a6c 100644 --- a/packages/contracts/src/git.ts +++ b/packages/contracts/src/git.ts @@ -330,18 +330,11 @@ export class GitCommandError extends Schema.TaggedErrorClass()( stdoutLength: Schema.optional(Schema.Number), stderrLength: Schema.optional(Schema.Number), outputLength: Schema.optional(Schema.Number), - /** - * Tail of git's stderr, truncated and with credential-bearing URLs - * redacted. Without it a failed git command is undiagnosable from outside - * the process: the lengths above say how much git wrote, never what. - */ - stderrTail: Schema.optional(Schema.String), detail: Schema.String, cause: Schema.optional(Schema.Defect()), }) { override get message(): string { - const summary = `Git command failed in ${this.operation} (${this.cwd}): ${this.detail}`; - return this.stderrTail === undefined ? summary : `${summary}\n${this.stderrTail}`; + return `Git command failed in ${this.operation} (${this.cwd}): ${this.detail}`; } } From c172f9bafbad558382fe8d532d40b78c03c2e937 Mon Sep 17 00:00:00 2001 From: Astrid Gealer Date: Fri, 7 Aug 2026 15:13:29 +0100 Subject: [PATCH 3/7] fix(vcs): log a normalized failure category, keep the output on cause MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses the remaining review finding: a log payload has to be as safe and bounded as a direct error attribute, and the previous revision copied raw stdout/stderr into one. Git echoes back its arguments and hook output, and the buffers are capped only by `maxOutputBytes` — megabytes at some call sites — so that payload was neither safe nor bounded. The log annotation is now a normalized category plus lengths, reusing the vocabulary `VcsProcess.classifyNonZeroExit` already established (authentication / not-found / command-failed). The exact text is preserved on the error's `cause`, as the convention prescribes. `GitCommandError`'s direct attributes are unchanged, so the existing assertion that a secret passed as a git argument reaches neither `error.message` nor an `stderr` property still holds, and `isMissingGitCwdError` is unaffected — it guards on `cause instanceof PlatformError`, which a string fails exactly as the previous `undefined` did. Co-Authored-By: Claude Opus 5 (1M context) --- apps/server/src/vcs/GitVcsDriverCore.ts | 46 +++++++++++++++++++------ 1 file changed, 35 insertions(+), 11 deletions(-) diff --git a/apps/server/src/vcs/GitVcsDriverCore.ts b/apps/server/src/vcs/GitVcsDriverCore.ts index 7b7aa3f6b9ec..d631d91dc864 100644 --- a/apps/server/src/vcs/GitVcsDriverCore.ts +++ b/apps/server/src/vcs/GitVcsDriverCore.ts @@ -397,18 +397,36 @@ function parseDefaultBranchFromRemoteHeadRef(value: string, remoteName: string): } /** - * Record what a failing git command actually printed, at debug level. + * Normalized category for a failing git command, for the log annotation. * - * `GitCommandError` keeps only bounded attributes — lengths, not text — and - * deliberately so: git echoes back its arguments and any hook output, so the - * text can carry credentials, and #3253 removed it from the error for exactly - * that reason. But dropping it everywhere leaves a failing git command - * undiagnosable from outside the process: neither the error, the RPC response, - * nor the server log says why git exited non-zero. + * Mirrors the vocabulary `VcsProcess.classifyNonZeroExit` already uses, so a + * log line says *what kind* of failure it was without carrying the text that + * says so. Kept deliberately coarse: the point is a bounded, safe value. + */ +const classifyGitFailure = (stderr: string): "authentication" | "not-found" | "command-failed" => { + const normalized = stderr.toLowerCase(); + if ( + normalized.includes("permission denied") || + normalized.includes("authentication failed") || + normalized.includes("could not read username") || + normalized.includes("access rights") + ) { + return "authentication"; + } + if (normalized.includes("not found") || normalized.includes("does not exist")) { + return "not-found"; + } + return "command-failed"; +}; + +/** + * Log annotation for a failing git command. * - * Logging it at debug keeps the diagnostic reachable when someone goes looking, - * without putting unbounded, possibly secret-bearing output into a value that - * crosses RPC, UI, and persistence boundaries. + * 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. The exact text is preserved on the error's `cause`. */ const logFailedGitCommandOutput = ( command: { @@ -423,7 +441,11 @@ const logFailedGitCommandOutput = ( Effect.logDebug( `GitVcsDriver.commandFailed: ${command.operation} in ${command.cwd} ` + `exited ${exitCode ?? "null"} (${command.args.length} arguments)`, - { stdout, stderr }, + { + failureKind: classifyGitFailure(stderr), + stdoutLength: stdout.length, + stderrLength: stderr.length, + }, ); function isMissingGitCwdError(error: GitCommandError): boolean { @@ -831,6 +853,7 @@ export const makeGitVcsDriverCore = Effect.fn("makeGitVcsDriverCore")(function* exitCode, stdoutLength: stdout.text.length, stderrLength: stderr.text.length, + cause: stderr.text, }); } @@ -919,6 +942,7 @@ export const makeGitVcsDriverCore = Effect.fn("makeGitVcsDriverCore")(function* ...(result.exitCode === null ? {} : { exitCode: result.exitCode }), stdoutLength: result.stdout.length, stderrLength: result.stderr.length, + cause: result.stderr, }), ), ), From 7d68e295d6f84e16eb77cd0f905396b245f71a88 Mon Sep 17 00:00:00 2001 From: Astrid Gealer Date: Fri, 7 Aug 2026 15:46:11 +0100 Subject: [PATCH 4/7] fix(vcs): classify a git failure instead of carrying its stderr MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Attaching stderr to `GitCommandError.cause` put unredacted git output on a field that is part of the error's RPC schema, so it reached clients on all six `WsVcs*` methods that declare `GitCommandError`. Git echoes back the remote it was handed and any hook output, so that text can carry a token. Take the approach `VcsProcessExitError.fromProcessExit` already uses for the same problem: classify stderr into a bounded `failureKind`, turn that into a fixed caller-facing `detail`, and let the text go. An authentication failure now says so in `detail` — which was the point of the change — while nothing git wrote crosses the boundary. `GitVcsDriverCore.test.ts` now asserts `cause` is free of a secret passed as a git argument; the existing case only covered `message` and `detail`, which is why this regressed unnoticed. Co-Authored-By: Claude Opus 5 (1M context) --- apps/server/src/vcs/GitVcsDriverCore.test.ts | 73 ++++++++++++++++++++ apps/server/src/vcs/GitVcsDriverCore.ts | 58 ++++++++++++---- packages/contracts/src/git.ts | 3 +- 3 files changed, 121 insertions(+), 13 deletions(-) diff --git a/apps/server/src/vcs/GitVcsDriverCore.test.ts b/apps/server/src/vcs/GitVcsDriverCore.test.ts index 6e352f013fe5..7a6c5782e0cf 100644 --- a/apps/server/src/vcs/GitVcsDriverCore.test.ts +++ b/apps/server/src/vcs/GitVcsDriverCore.test.ts @@ -42,6 +42,21 @@ 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, + }); + const makeSuccessfulHandle = (stdout: string) => ChildProcessSpawner.makeHandle({ pid: ChildProcessSpawner.ProcessId(1), @@ -712,9 +727,67 @@ 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"]); }), ); + 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 spawner = ChildProcessSpawner.make(() => + Effect.succeed( + makeFailingHandle( + `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`, + ), + ), + ); + const layer = GitVcsDriver.layer.pipe( + Layer.provide(ServerConfigLayer), + Layer.provideMerge( + Layer.merge( + NodeServices.layer, + Layer.succeed(ChildProcessSpawner.ChildProcessSpawner, spawner), + ), + ), + ); + + 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); + + 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(layer)); + }); + it.effect("recovers a structurally identified missing cwd as a non-repository", () => Effect.gen(function* () { const parent = yield* makeTmpDir(); diff --git a/apps/server/src/vcs/GitVcsDriverCore.ts b/apps/server/src/vcs/GitVcsDriverCore.ts index d631d91dc864..ac3de9cf72f8 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"; @@ -397,13 +398,14 @@ function parseDefaultBranchFromRemoteHeadRef(value: string, remoteName: string): } /** - * Normalized category for a failing git command, for the log annotation. + * Normalized category for a failing git command. * - * Mirrors the vocabulary `VcsProcess.classifyNonZeroExit` already uses, so a - * log line says *what kind* of failure it was without carrying the text that - * says so. Kept deliberately coarse: the point is a bounded, safe value. + * 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): "authentication" | "not-found" | "command-failed" => { +const classifyGitFailure = (stderr: string): VcsProcessExitFailureKind => { const normalized = stderr.toLowerCase(); if ( normalized.includes("permission denied") || @@ -419,6 +421,25 @@ const classifyGitFailure = (stderr: string): "authentication" | "not-found" | "c 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": + return "Git could not find the requested repository, remote, or ref."; + case "command-failed": + return fallback; + } +}; + /** * Log annotation for a failing git command. * @@ -426,7 +447,7 @@ const classifyGitFailure = (stderr: string): "authentication" | "not-found" | "c * 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. The exact text is preserved on the error's `cause`. + * error attribute. */ const logFailedGitCommandOutput = ( command: { @@ -435,6 +456,7 @@ const logFailedGitCommandOutput = ( readonly args: ReadonlyArray; }, exitCode: number | null, + failureKind: VcsProcessExitFailureKind, stdout: string, stderr: string, ): Effect.Effect => @@ -442,7 +464,7 @@ const logFailedGitCommandOutput = ( `GitVcsDriver.commandFailed: ${command.operation} in ${command.cwd} ` + `exited ${exitCode ?? "null"} (${command.args.length} arguments)`, { - failureKind: classifyGitFailure(stderr), + failureKind, stdoutLength: stdout.length, stderrLength: stderr.length, }, @@ -846,14 +868,21 @@ export const makeGitVcsDriverCore = Effect.fn("makeGitVcsDriverCore")(function* yield* trace2Monitor.flush; if (!input.allowNonZeroExit && exitCode !== 0) { - yield* logFailedGitCommandOutput(commandInput, exitCode, stdout.text, stderr.text); + 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, - cause: stderr.text, }); } @@ -928,9 +957,11 @@ export const makeGitVcsDriverCore = Effect.fn("makeGitVcsDriverCore")(function* if (options.allowNonZeroExit || result.exitCode === 0) { return Effect.succeed(result); } + const failureKind = classifyGitFailure(result.stderr); return logFailedGitCommandOutput( { operation, cwd, args }, result.exitCode, + failureKind, result.stdout, result.stderr, ).pipe( @@ -938,11 +969,14 @@ export const makeGitVcsDriverCore = Effect.fn("makeGitVcsDriverCore")(function* Effect.fail( new GitCommandError({ ...gitCommandContext({ operation, cwd, args }), - detail: options.fallbackErrorDetail ?? "Git command exited with a non-zero status.", + 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, - cause: result.stderr, }), ), ), 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), From e721f82d51ff22ab8d4c3db95f67442ab82a4ec2 Mon Sep 17 00:00:00 2001 From: Astrid Gealer Date: Fri, 7 Aug 2026 15:59:11 +0100 Subject: [PATCH 5/7] fix(vcs): only read a remote auth failure as authentication MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `classifyGitFailure` matched a bare "permission denied", which git also writes for local filesystem errors — `git init` into an unwritable directory reaches it too. That was harmless while the classification only annotated a log line, but it now drives the caller-facing `detail`, so an unwritable directory would have been answered with advice about remote credentials. Match the forms that are specific to a remote instead: ssh names the methods it tried ("Permission denied (publickey)."), and GitHub over https writes "remote: Permission to owner/repo.git denied to user". Also widens the not-found sentence, which promised the missing thing was a repository while the match is broad enough to catch `path 'x' does not exist in 'HEAD'`. The two new cases were asserting against real git rather than the stub — they sit outside `it.layer(TestLayer)` now, and assert on recorded spawns so a bypassed stub fails instead of silently passing. Co-Authored-By: Claude Opus 5 (1M context) --- apps/server/src/vcs/GitVcsDriverCore.test.ts | 152 ++++++++++++------- apps/server/src/vcs/GitVcsDriverCore.ts | 19 ++- 2 files changed, 117 insertions(+), 54 deletions(-) diff --git a/apps/server/src/vcs/GitVcsDriverCore.test.ts b/apps/server/src/vcs/GitVcsDriverCore.test.ts index 7a6c5782e0cf..c78e8a87f603 100644 --- a/apps/server/src/vcs/GitVcsDriverCore.test.ts +++ b/apps/server/src/vcs/GitVcsDriverCore.test.ts @@ -57,6 +57,38 @@ const makeFailingHandle = (stderr: string, exitCode = 128) => 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), @@ -650,6 +682,75 @@ 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)); +}); + +it.effect("does not read a local permission error as an authentication failure", () => { + // `git init` into an unwritable directory says "Permission denied" too. + // Matching that bare phrase would answer a filesystem problem with advice + // about remote credentials, which is worse than saying nothing. + const failing = makeFailingGitLayer( + `error: could not create work tree dir 'projects': Permission denied\n`, + ); + + 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: ["init", "projects"], + }) + .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.layer(TestLayer)("GitVcsDriver core integration", (it) => { describe("process environment", () => { it.effect("preserves the caller locale for general Git subprocesses", () => @@ -737,57 +838,6 @@ it.layer(TestLayer)("GitVcsDriver core integration", (it) => { }), ); - 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 spawner = ChildProcessSpawner.make(() => - Effect.succeed( - makeFailingHandle( - `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`, - ), - ), - ); - const layer = GitVcsDriver.layer.pipe( - Layer.provide(ServerConfigLayer), - Layer.provideMerge( - Layer.merge( - NodeServices.layer, - Layer.succeed(ChildProcessSpawner.ChildProcessSpawner, spawner), - ), - ), - ); - - 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); - - 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(layer)); - }); - it.effect("recovers a structurally identified missing cwd as a non-repository", () => Effect.gen(function* () { const parent = yield* makeTmpDir(); diff --git a/apps/server/src/vcs/GitVcsDriverCore.ts b/apps/server/src/vcs/GitVcsDriverCore.ts index ac3de9cf72f8..44c2212e81ae 100644 --- a/apps/server/src/vcs/GitVcsDriverCore.ts +++ b/apps/server/src/vcs/GitVcsDriverCore.ts @@ -408,10 +408,20 @@ function parseDefaultBranchFromRemoteHeadRef(value: string, remoteName: string): const classifyGitFailure = (stderr: string): VcsProcessExitFailureKind => { const normalized = stderr.toLowerCase(); if ( - normalized.includes("permission denied") || normalized.includes("authentication failed") || normalized.includes("could not read username") || - normalized.includes("access rights") + normalized.includes("could not read password") || + normalized.includes("invalid username or password") || + normalized.includes("authentication required") || + // "...make sure you have the correct access rights and the repository exists." + normalized.includes("access rights") || + // 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"; } @@ -434,7 +444,10 @@ const detailForGitFailure = (failureKind: VcsProcessExitFailureKind, fallback: s case "authentication": return "Git authentication failed. Check the credentials available to this host for the remote."; case "not-found": - return "Git could not find the requested repository, remote, or ref."; + // 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; } From b68ec7f05e305ceab89f2ed71feba9041d184f78 Mon Sep 17 00:00:00 2001 From: Astrid Gealer Date: Fri, 7 Aug 2026 16:02:02 +0100 Subject: [PATCH 6/7] test(vcs): cover the local permission phrasings git actually writes Bugbot named `.git/index.lock` and `FETCH_HEAD` specifically. Both already classify as command-failed, but only the clone-target phrasing was covered, so nothing pinned the other two. Co-Authored-By: Claude Opus 5 (1M context) --- apps/server/src/vcs/GitVcsDriverCore.test.ts | 73 ++++++++++++-------- 1 file changed, 46 insertions(+), 27 deletions(-) diff --git a/apps/server/src/vcs/GitVcsDriverCore.test.ts b/apps/server/src/vcs/GitVcsDriverCore.test.ts index c78e8a87f603..a5368dcd3b12 100644 --- a/apps/server/src/vcs/GitVcsDriverCore.test.ts +++ b/apps/server/src/vcs/GitVcsDriverCore.test.ts @@ -722,34 +722,53 @@ it.effect("classifies an authentication failure without retaining the remote it ).pipe(Effect.provide(failing.layer)); }); -it.effect("does not read a local permission error as an authentication failure", () => { - // `git init` into an unwritable directory says "Permission denied" too. - // Matching that bare phrase would answer a filesystem problem with advice - // about remote credentials, which is worse than saying nothing. - const failing = makeFailingGitLayer( - `error: could not create work tree dir 'projects': Permission denied\n`, - ); - - 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: ["init", "projects"], - }) - .pipe(Effect.flip); +// 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)); -}); + 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.layer(TestLayer)("GitVcsDriver core integration", (it) => { describe("process environment", () => { From dc881848a2410be17ed954a4160c74a702ca42d5 Mon Sep 17 00:00:00 2001 From: Astrid Gealer Date: Mon, 10 Aug 2026 07:47:31 +0100 Subject: [PATCH 7/7] fix(vcs): classify not-found before access-rights, stabilize locale GitHub/GitLab SSH failures for a missing repo include both "not found" and the "access rights" footer; matching the footer first mislabeled them as auth. Also force LC_ALL=C on paths that classify failures so translated stderr cannot miss the English heuristics. Co-authored-by: Cursor --- apps/server/src/vcs/GitVcsDriverCore.test.ts | 66 ++++++++++++++++++-- apps/server/src/vcs/GitVcsDriverCore.ts | 21 ++++++- 2 files changed, 78 insertions(+), 9 deletions(-) diff --git a/apps/server/src/vcs/GitVcsDriverCore.test.ts b/apps/server/src/vcs/GitVcsDriverCore.test.ts index a5368dcd3b12..a383da3d9fae 100644 --- a/apps/server/src/vcs/GitVcsDriverCore.test.ts +++ b/apps/server/src/vcs/GitVcsDriverCore.test.ts @@ -770,19 +770,73 @@ for (const failure of localPermissionFailures) { }); } +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"); }), ); }); diff --git a/apps/server/src/vcs/GitVcsDriverCore.ts b/apps/server/src/vcs/GitVcsDriverCore.ts index 44c2212e81ae..02fdc9360e63 100644 --- a/apps/server/src/vcs/GitVcsDriverCore.ts +++ b/apps/server/src/vcs/GitVcsDriverCore.ts @@ -413,8 +413,6 @@ const classifyGitFailure = (stderr: string): VcsProcessExitFailureKind => { normalized.includes("could not read password") || normalized.includes("invalid username or password") || normalized.includes("authentication required") || - // "...make sure you have the correct access rights and the repository exists." - normalized.includes("access rights") || // 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 @@ -425,9 +423,16 @@ const classifyGitFailure = (stderr: string): VcsProcessExitFailureKind => { ) { 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"; }; @@ -815,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, { @@ -822,6 +831,7 @@ export const makeGitVcsDriverCore = Effect.fn("makeGitVcsDriverCore")(function* env: { ...process.env, ...input.env, + ...(input.allowNonZeroExit ? {} : { LC_ALL: "C" }), ...trace2Monitor.env, }, }), @@ -956,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 } : {}),