fix(vcs): surface git's stderr on GitCommandError

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<ReadonlyArray<string>> = [];
+  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"]);
       }),
     );