diff --git a/apps/server/src/vcs/GitVcsDriverCore.test.ts b/apps/server/src/vcs/GitVcsDriverCore.test.ts index 5b84236db14c0..60990e24cd219 100644 --- a/apps/server/src/vcs/GitVcsDriverCore.test.ts +++ b/apps/server/src/vcs/GitVcsDriverCore.test.ts @@ -1,5 +1,6 @@ // @effect-diagnostics nodeBuiltinImport:off - realpathSync.native resolves Windows 8.3 short names, which the Effect realPath does not. import * as NodeFS from "node:fs"; +import * as NodeOS from "node:os"; import * as NodePath from "node:path"; import * as NodeURL from "node:url"; import * as NodeCrypto from "@effect/platform-node/NodeCrypto"; @@ -631,7 +632,7 @@ it.effect.each(["bare", "unborn"] as const)( assert.equal(Result.isSuccess(result), state === "bare"); if (Result.isFailure(result)) { assert.equal(result.failure.operation, "GitVcsDriver.createWorktree.createBranch"); - assert.equal(result.failure.detail, "git branch failed"); + assert.equal(result.failure.detail, "Git could not find that branch or commit."); } assert.equal(spawnedArgs.filter((args) => args.includes("--git-common-dir")).length, 1); assert.isFalse(spawnedArgs.some((args) => args[0] === "symbolic-ref")); @@ -1781,7 +1782,7 @@ describe("rejected git refs", () => { { label: "a ref the remote refused", stderr: [ - "remote: Unable to determine if workflow can be created; workflow scope may be required.", + "remote: nothing to commit; workflow scope may be required.", `To ${remoteUrl}`, " ! [remote rejected] private-branch -> private-branch (workflow scope may be required.)", failedPush, @@ -1811,7 +1812,7 @@ describe("rejected git refs", () => { // A fetch into a diverged local branch prints the same rejected line, but // never "failed to push some refs". - it.effect("leaves a rejected fetch unclassified", () => + it.effect("leaves a rejected fetch unclassified without exposing diagnostics", () => Effect.gen(function* () { const error = yield* failWith( [`From ${remoteUrl}`, " ! [rejected] main -> side (non-fast-forward)"].join( @@ -1821,6 +1822,7 @@ describe("rejected git refs", () => { assert.equal(error.reason, undefined); assert.equal(error.detail, "Git command exited with a non-zero status."); + for (const text of leakedText) assert.notInclude(error.message, text); }), ); }); @@ -1905,18 +1907,28 @@ it.layer(layerTest)("GitVcsDriver core integration", (it) => { }), ); describe("structured errors", () => { - it.effect("does not retain git arguments or stderr in command failures", () => + it.effect.each([ + ...[ + "secret-token-value", + "prefix123=private-secret", + "tiny", + "status=private-secret", + "prefix-status-private-secret", + "Bearer token=private-secret", + "private-secret\ncontinued", + ].map((secret) => ({ secret, args: ["status", `--unknown-option=${secret}`] })), + { secret: "private-secret", args: ["status", "--private-secret"] }, + ])("does not retain git argument $secret in command failures", ({ secret, args }) => Effect.gen(function* () { const cwd = yield* makeTmpDir(); const driver = yield* GitVcsDriver.GitVcsDriver; yield* driver.initRepo({ cwd }); - const secret = "secret-token-value"; const error = yield* driver .execute({ operation: "GitVcsDriver.test.redactedFailure", cwd, - args: ["status", `--unknown-option=${secret}`], + args, }) .pipe(Effect.flip); @@ -1929,7 +1941,9 @@ it.layer(layerTest)("GitVcsDriver core integration", (it) => { }); assert.isNumber(error.exitCode); assert.isAbove(error.stderrLength ?? 0, 0); + assert.equal(error.detail, "Git command exited with a non-zero status."); assert.notInclude(error.detail, secret); + assert.notInclude(error.message, secret.split("=").at(-1)!.split("\n")[0]!); assert.notInclude(error.message, secret); assert.notProperty(error, "args"); assert.notProperty(error, "stderr"); @@ -1937,6 +1951,107 @@ it.layer(layerTest)("GitVcsDriver core integration", (it) => { }), ); + it.effect("names a missing author identity behind a failed commit", () => + Effect.gen(function* () { + const cwd = yield* makeTmpDir(); + const delegate = yield* ChildProcessSpawner.ChildProcessSpawner; + const spawner = ChildProcessSpawner.make((command) => { + if (!ChildProcess.isStandardCommand(command) || command.args[0] !== "commit") { + return delegate.spawn(command); + } + assert.equal(command.options.env?.LANGUAGE, "C"); + return delegate.spawn( + ChildProcess.make(command.command, command.args, { + ...command.options, + env: { + ...command.options.env, + GIT_CONFIG_GLOBAL: NodeOS.devNull, + GIT_CONFIG_NOSYSTEM: "1", + GIT_CONFIG_COUNT: "2", + GIT_CONFIG_KEY_0: "user.name", + GIT_CONFIG_VALUE_0: "", + GIT_CONFIG_KEY_1: "user.email", + GIT_CONFIG_VALUE_1: "", + }, + }), + ); + }); + const driver = yield* makeGitVcsDriverCore().pipe( + Effect.provideService(ChildProcessSpawner.ChildProcessSpawner, spawner), + Effect.provide(layerServerConfig), + ); + yield* driver.initRepo({ cwd }); + yield* writeTextFile(cwd, "README.md", "hello\n"); + yield* git(cwd, ["add", "README.md"]); + + const error = yield* driver.commit(cwd, "initial", "").pipe(Effect.flip); + + assert.deepInclude(error, { + _tag: "GitCommandError", + reason: "author_identity_unknown", + }); + assert.include(error.detail, "user.name"); + assert.include(error.detail, "user.email"); + }), + ); + + it.effect.each([false, true])( + "names a commit without staged changes (unstaged: %s)", + (unstaged) => + Effect.gen(function* () { + const cwd = yield* makeTmpDir(); + const driver = yield* GitVcsDriver.GitVcsDriver; + yield* initRepoWithCommit(cwd); + + if (unstaged) yield* writeTextFile(cwd, "README.md", "unstaged changes\n"); + const error = yield* driver.commit(cwd, "empty", "").pipe(Effect.flip); + + assert.deepInclude(error, { + _tag: "GitCommandError", + reason: "nothing_to_commit", + }); + assert.include(error.detail, "Nothing to commit"); + }), + ); + + it.effect.each( + ["https", "Bearer", "-sensitive-subject"].flatMap((subject) => + ["stdout", "stderr"].map((stream) => ({ subject, stream })), + ), + )( + "surfaces a rejected commit hook safely (subject: $subject, stream: $stream)", + ({ subject, stream }) => + Effect.gen(function* () { + const cwd = yield* makeTmpDir(); + const driver = yield* GitVcsDriver.GitVcsDriver; + const fs = yield* FileSystem.FileSystem; + const path = yield* Path.Path; + yield* initRepoWithCommit(cwd); + yield* writeTextFile(cwd, "README.md", "changed\n"); + yield* git(cwd, ["add", "README.md"]); + yield* writeTextFile( + cwd, + ".git/hooks/pre-commit", + `#!/bin/sh\necho 'error: arbitrary-hook-secret; lint: fix formatting in README.md; https://user:private-secret@host/repo?token=query-secret; Authorization: Bearer TOKEN123; rejected commit -sensitive-subject' ${stream === "stderr" ? ">&2" : ""}\nexit 1\n`, + ); + yield* fs.chmod(path.join(cwd, ".git/hooks/pre-commit"), 0o755); + + const error = yield* driver.commit(cwd, subject, "").pipe(Effect.flip); + + assert.notProperty(error, "reason"); + assert.equal(error.detail, "Git command exited with a non-zero status."); + if (subject.startsWith("-")) assert.notInclude(error.message, subject); + for (const secret of [ + "arbitrary-hook-secret", + "private-secret", + "query-secret", + "TOKEN123", + ]) { + assert.notInclude(error.message, secret); + } + }), + ); + // regression: #8645 name the cause of a failed git command it.effect("names the branch conflict behind a failed worktree add", () => Effect.gen(function* () { @@ -1994,54 +2109,56 @@ it.layer(layerTest)("GitVcsDriver core integration", (it) => { ); it.effect.each([ - { action: "worktree add", detail: "git worktree add failed" }, - { action: "worktree add -b", detail: "git branch failed" }, - { action: "switch", detail: "git checkout failed" }, - ])( - "reports a missing ref during $action with stable, redacted diagnostics", - ({ action, detail }) => - Effect.gen(function* () { - const cwd = yield* makeTmpDir(); - yield* initRepoWithCommit(cwd); - const path = yield* Path.Path; - const refName = "private/deleted-base"; - const locales: Array = []; - const delegate = yield* ChildProcessSpawner.ChildProcessSpawner; - const spawner = ChildProcessSpawner.make((command) => { - if ( - ChildProcess.isStandardCommand(command) && - (command.args.includes("worktree") || - command.args[0] === "branch" || - command.args[0] === "checkout") - ) { - locales.push(command.options.env?.LANGUAGE); - } - return delegate.spawn(command); - }); - const driver = yield* makeGitVcsDriverCore().pipe( - Effect.provideService(ChildProcessSpawner.ChildProcessSpawner, spawner), - Effect.provide(layerServerConfig), - ); - const error = - action === "switch" - ? yield* driver.switchRef({ cwd, refName }).pipe(Effect.flip) - : yield* driver - .createWorktree({ - cwd, - refName, - ...(action === "worktree add -b" ? { newRefName: "t3/new-thread" } : {}), - path: path.join(cwd, "worktree"), - }) - .pipe(Effect.flip); - - assert.deepInclude(error, { _tag: "GitCommandError", detail, reason: "ref_not_found" }); - assert.include(error.message, `${detail} (ref_not_found)`); - assert.notInclude(error.message, refName); - assert.notInclude(yield* encodeGitCommandError(error), refName); - assert.deepEqual(locales, ["C"]); - assert.notProperty(error, "stderr"); - assert.notProperty(error, "args"); - }), + { action: "worktree add" as const }, + { action: "worktree add -b" as const }, + { action: "switch" as const }, + ])("reports a missing ref during $action with stable, redacted diagnostics", ({ action }) => + Effect.gen(function* () { + const cwd = yield* makeTmpDir(); + yield* initRepoWithCommit(cwd); + const path = yield* Path.Path; + const refName = "private/deleted-base"; + const locales: Array = []; + const delegate = yield* ChildProcessSpawner.ChildProcessSpawner; + const spawner = ChildProcessSpawner.make((command) => { + if ( + ChildProcess.isStandardCommand(command) && + (command.args.includes("worktree") || + command.args[0] === "branch" || + command.args[0] === "checkout") + ) { + locales.push(command.options.env?.LANGUAGE); + } + return delegate.spawn(command); + }); + const driver = yield* makeGitVcsDriverCore().pipe( + Effect.provideService(ChildProcessSpawner.ChildProcessSpawner, spawner), + Effect.provide(layerServerConfig), + ); + const error = + action === "switch" + ? yield* driver.switchRef({ cwd, refName }).pipe(Effect.flip) + : yield* driver + .createWorktree({ + cwd, + refName, + ...(action === "worktree add -b" ? { newRefName: "t3/new-thread" } : {}), + path: path.join(cwd, "worktree"), + }) + .pipe(Effect.flip); + + assert.deepInclude(error, { + _tag: "GitCommandError", + detail: "Git could not find that branch or commit.", + reason: "ref_not_found", + }); + assert.include(error.message, "Git could not find that branch or commit. (ref_not_found)"); + assert.notInclude(error.message, refName); + assert.notInclude(yield* encodeGitCommandError(error), refName); + assert.deepEqual(locales, ["C"]); + assert.notProperty(error, "stderr"); + assert.notProperty(error, "args"); + }), ); it.effect("lets a slow remote branch checkout finish without blocking config writes", () => @@ -2099,6 +2216,15 @@ it.layer(layerTest)("GitVcsDriver core integration", (it) => { label: "a pathspec worded like a diagnostic", args: ["checkout", "HEAD", "--", "fatal: invalid reference: missing"], }, + ...[ + "signing failed", + "gpg failed to sign", + "no ssh public key", + "Author identity unknown", + ].map((name) => ({ + label: `a pathspec named ${name}`, + args: ["checkout", "HEAD", "--", name], + })), // The tag exists; it names a tree, which git reports in the same words. { label: "a worktree from a non-commit", diff --git a/apps/server/src/vcs/GitVcsDriverCore.ts b/apps/server/src/vcs/GitVcsDriverCore.ts index 12ce999f25e18..5df22536d606d 100644 --- a/apps/server/src/vcs/GitVcsDriverCore.ts +++ b/apps/server/src/vcs/GitVcsDriverCore.ts @@ -479,16 +479,58 @@ const GIT_FAILURE_REASON_PATTERNS: ReadonlyArray> = { + authentication_failed: + "Git could not authenticate with the remote. Check your credentials or SSH keys, then try again.", + author_identity_unknown: + "Git doesn't know your name or email. Set user.name and user.email, then try again.", + branch_already_exists: "A branch with that name already exists. Choose a different name.", + branch_checked_out_in_worktree: + "That branch is already checked out in another worktree. Switch it there, or pick a different branch.", + commit_signing_failed: + "Git couldn't sign the commit. Check your signing key setup, or disable commit signing, then try again.", + config_locked: + "Another Git process kept the repository's config locked. Wait for it to finish, then retry.", + host_key_unverified: + "The remote host's SSH key is not trusted yet. Verify it once in a terminal with ssh, then try again.", + index_locked: "Git's index is locked. Wait for other Git operations to finish, then try again.", + not_a_repository: "This folder is not a Git repository.", + nothing_to_commit: + "Nothing to commit. Stage changes first, or make sure the selected files have changes.", + path_already_exists: "That path already exists. Choose a different location or name.", push_rejected_by_remote: "The remote rejected the push. Push this branch from a terminal to see the server's message.", push_rejected_non_fast_forward: "The remote has commits this branch doesn't have. Pull or rebase, then push again.", - config_locked: - "Another Git process kept the repository's config locked. Wait for it to finish, then retry.", + ref_not_found: "Git could not find that branch or commit.", + remote_unreachable: + "Git could not reach the remote repository. Check the remote URL and your network, then try again.", + tag_would_be_clobbered: "Updating would overwrite an existing tag. Delete or move the tag first.", }; function gitFailureDetail(reason: GitCommandFailureReason | null): string { @@ -498,24 +540,21 @@ function gitFailureDetail(reason: GitCommandFailureReason | null): string { ); } -// Existing non-commit objects share the missing-ref fatal diagnostic; Git -// distinguishes them with a separate error line. -const GIT_NOT_A_COMMIT_PATTERN = /^error: .+(?:, not a commit|expected commit type)/im; - -// Trust only Git's diagnostic prefixes: local hooks can write indistinguishable text. -// Exclude `remote:` too, since it prefixes remote hook output as well as server diagnostics. -const GIT_DIAGNOSTIC_LINE_PATTERN = /^(?:fatal|error):|^!\s|^\s+!\s/; -// SSH credential refusals are unprefixed; Git's later generic error would otherwise misclassify them as unreachable. -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 { +function classifyGitFailure( + stderr: string, + stdout: string, + args: readonly string[], +): GitCommandFailureReason | null { + if (args[0] === "commit" && NOTHING_TO_COMMIT_PATTERN.test(stdout)) { + return "nothing_to_commit"; + } const diagnostics = stderr .split(/\r?\n/) .filter( (line) => GIT_DIAGNOSTIC_LINE_PATTERN.test(line) || - (!/^remote:/.test(line) && SSH_TRANSPORT_REFUSAL_PATTERN.test(line)), + (!/^remote:/.test(line) && + (SSH_TRANSPORT_REFUSAL_PATTERN.test(line) || /^Author identity unknown\s*$/i.test(line))), ) .join("\n"); if (diagnostics.length === 0) return null; @@ -1071,7 +1110,7 @@ export const makeGitVcsDriverCore = Effect.fn("makeGitVcsDriverCore")(function* yield* trace2Monitor.flush; if (!input.allowNonZeroExit && exitCode !== 0) { - const reason = classifyGitFailure(stderr.text); + const reason = classifyGitFailure(stderr.text, stdout.text, commandInput.args); return yield* new GitCommandError({ ...gitCommandContext(commandInput), ...(reason === null ? {} : { reason }), @@ -1188,12 +1227,13 @@ export const makeGitVcsDriverCore = Effect.fn("makeGitVcsDriverCore")(function* if (options.allowNonZeroExit || result.exitCode === 0) { return Effect.succeed(result); } - const reason = classifyGitFailure(result.stderr); + const reason = classifyGitFailure(result.stderr, result.stdout, args); + const classifiedDetail = reason === null ? undefined : GIT_FAILURE_REASON_DETAILS[reason]; return Effect.fail( new GitCommandError({ ...gitCommandContext({ operation, cwd, args }), ...(reason === null ? {} : { reason }), - detail: options.fallbackErrorDetail ?? gitFailureDetail(reason), + detail: classifiedDetail ?? options.fallbackErrorDetail ?? gitFailureDetail(null), ...(result.exitCode === null ? {} : { exitCode: result.exitCode }), stdoutLength: result.stdout.length, stderrLength: result.stderr.length, @@ -2679,7 +2719,7 @@ export const makeGitVcsDriverCore = Effect.fn("makeGitVcsDriverCore")(function* onStderrLine: (line: string) => options.progress?.onOutputLine?.({ stream: "stderr", text: line }) ?? Effect.void, }; - yield* executeGit("GitVcsDriver.commit.commit", cwd, args, { + yield* executeGitWithStableDiagnostics("GitVcsDriver.commit.commit", cwd, args, { maxOutputBytes: REVIEW_METADATA_MAX_OUTPUT_BYTES, ...(options?.timeoutMs !== undefined ? { timeoutMs: options.timeoutMs } : {}), ...(progress ? { progress } : {}), @@ -3029,11 +3069,11 @@ export const makeGitVcsDriverCore = Effect.fn("makeGitVcsDriverCore")(function* } } if (result.exitCode !== 0) { - const reason = classifyGitFailure(result.stderr); + const reason = classifyGitFailure(result.stderr, result.stdout, args); return yield* new GitCommandError({ ...gitCommandContext({ operation: "GitVcsDriver.pullCurrentBranch.pull", cwd, args }), ...(reason === null ? {} : { reason }), - detail: fetchFailureDetail(result.stderr) ?? "git pull failed", + detail: fetchFailureDetail(result.stderr) ?? gitFailureDetail(reason), exitCode: result.exitCode, stdoutLength: result.stdout.length, stderrLength: result.stderr.length, diff --git a/packages/contracts/src/git.test.ts b/packages/contracts/src/git.test.ts index 60d99defe6e2f..5927c68811203 100644 --- a/packages/contracts/src/git.test.ts +++ b/packages/contracts/src/git.test.ts @@ -29,6 +29,21 @@ describe("GitCommandError", () => { "Git command failed in GitVcsDriver.createWorktree (/repo): git worktree add failed", ); }); + + it("keeps a known actionable failure reason", () => { + const error = decodeGitCommandError({ + _tag: "GitCommandError", + operation: "GitVcsDriver.commit.commit", + command: "git", + cwd: "/repo", + detail: + "Nothing to commit. Stage changes first, or make sure the selected files have changes.", + reason: "nothing_to_commit", + }); + + expect(error.reason).toBe("nothing_to_commit"); + expect(error.message).toContain("(nothing_to_commit)"); + }); }); describe("GitPreparePullRequestThreadResult", () => { diff --git a/packages/contracts/src/git.ts b/packages/contracts/src/git.ts index 62297fdd91542..ef1e7f067a0a4 100644 --- a/packages/contracts/src/git.ts +++ b/packages/contracts/src/git.ts @@ -350,16 +350,21 @@ export const VcsPullResult = Schema.Struct({ }); export type VcsPullResult = typeof VcsPullResult.Type; -// Git stderr can echo credentials from argv and remote URLs. Only a fixed -// reason reaches the error and its diagnostic suffix; newer, unknown reasons -// decode as absent through GitCommandError. +// Git stderr can echo credentials from argv and remote URLs. Classified failures +// use a closed reason tag plus a fixed actionable detail. Unclassified failures +// keep a fixed generic detail. Newer unknown reasons decode as absent through +// GitCommandError. export const GitCommandFailureReason = Schema.Literals([ "authentication_failed", + "author_identity_unknown", "branch_already_exists", "branch_checked_out_in_worktree", + "commit_signing_failed", "config_locked", "host_key_unverified", + "index_locked", "not_a_repository", + "nothing_to_commit", "path_already_exists", "push_rejected_by_remote", "push_rejected_non_fast_forward",