fix(approval): read identity from git config instead of env var

GIT_USER_NAME is set during the entrypoint boot sequence and written
into git config (--system user.name), but the env var is not available
to the approval process at runtime. This caused the ownership exemption
check to always fail (gitUserName was undefined).

Read from `git config user.name` instead, which is reliably set during
boot and available to all processes in the container.

Author: perezd

approval/__tests__/ownership.test.ts

@@ -11,19 +11,27 @@ describe("parseRemoteFromPushCommand", () => {
   });
 
   test("extracts remote from 'git push my-fork feature'", () => {
-    expect(parseRemoteFromPushCommand("git push my-fork feature")).toBe("my-fork");
+    expect(parseRemoteFromPushCommand("git push my-fork feature")).toBe(
+      "my-fork",
+    );
   });
 
   test("skips flags: 'git push --force origin main'", () => {
-    expect(parseRemoteFromPushCommand("git push --force origin main")).toBe("origin");
+    expect(parseRemoteFromPushCommand("git push --force origin main")).toBe(
+      "origin",
+    );
   });
 
   test("skips flags: 'git push -u origin feature'", () => {
-    expect(parseRemoteFromPushCommand("git push -u origin feature")).toBe("origin");
+    expect(parseRemoteFromPushCommand("git push -u origin feature")).toBe(
+      "origin",
+    );
   });
 
   test("skips compound flags: 'git push --set-upstream origin main'", () => {
-    expect(parseRemoteFromPushCommand("git push --set-upstream origin main")).toBe("origin");
+    expect(
+      parseRemoteFromPushCommand("git push --set-upstream origin main"),
+    ).toBe("origin");
   });
 
   test("returns null for bare 'git push' (can't assume origin)", () => {
@@ -35,15 +43,23 @@ describe("parseRemoteFromPushCommand", () => {
   });
 
   test("returns null when value-consuming flag -o is present", () => {
-    expect(parseRemoteFromPushCommand("git push -o ci.skip origin main")).toBeNull();
+    expect(
+      parseRemoteFromPushCommand("git push -o ci.skip origin main"),
+    ).toBeNull();
   });
 
   test("returns null when --push-option is present", () => {
-    expect(parseRemoteFromPushCommand("git push --push-option ci.skip origin main")).toBeNull();
+    expect(
+      parseRemoteFromPushCommand("git push --push-option ci.skip origin main"),
+    ).toBeNull();
   });
 
   test("returns null when --repo is present", () => {
-    expect(parseRemoteFromPushCommand("git push --repo=https://example.com origin main")).toBeNull();
+    expect(
+      parseRemoteFromPushCommand(
+        "git push --repo=https://example.com origin main",
+      ),
+    ).toBeNull();
   });
 
   test("returns null for non-git-push command", () => {
@@ -57,7 +73,9 @@ describe("parseRemoteFromPushCommand", () => {
 
 describe("extractGitHubOwner", () => {
   test("extracts owner from HTTPS URL", () => {
-    expect(extractGitHubOwner("https://github.com/alice/repo.git")).toBe("alice");
+    expect(extractGitHubOwner("https://github.com/alice/repo.git")).toBe(
+      "alice",
+    );
   });
 
   test("extracts owner from HTTPS URL without .git", () => {
@@ -86,146 +104,162 @@ describe("extractGitHubOwner", () => {
 });
 
 describe("isOwnedRemotePush", () => {
-  let originalEnv: string | undefined;
   let originalSpawn: typeof Bun.spawn;
 
   beforeEach(() => {
-    originalEnv = process.env.GIT_USER_NAME;
     originalSpawn = Bun.spawn;
   });
 
   afterEach(() => {
-    if (originalEnv === undefined) {
-      delete process.env.GIT_USER_NAME;
-    } else {
-      process.env.GIT_USER_NAME = originalEnv;
-    }
     Bun.spawn = originalSpawn;
   });
 
-  let lastSpawnArgs: string[] = [];
-
-  function mockGitRemote(url: string, exitCode = 0) {
+  let spawnCalls: string[][] = [];
+
+  /**
+   * Mock Bun.spawn to handle two sequential calls:
+   *   1. git config user.name → returns userName
+   *   2. git remote get-url --push <remote> → returns remoteUrl
+   *
+   * configExitCode / remoteExitCode let individual tests simulate failures.
+   */
+  function mockGitSpawn(
+    userName: string,
+    remoteUrl: string,
+    { configExitCode = 0, remoteExitCode = 0 } = {},
+  ) {
+    spawnCalls = [];
+    let callIndex = 0;
     // @ts-expect-error — partial mock of Bun.spawn for testing
     Bun.spawn = (args: string[]) => {
-      lastSpawnArgs = args;
+      spawnCalls.push(args);
+      const idx = callIndex++;
+      const isConfigCall = idx === 0;
+      const output = isConfigCall ? userName : remoteUrl;
+      const exit = isConfigCall ? configExitCode : remoteExitCode;
       return {
         stdout: new ReadableStream({
           start(controller) {
-            controller.enqueue(new TextEncoder().encode(url + "\n"));
+            controller.enqueue(new TextEncoder().encode(output + "\n"));
             controller.close();
           },
         }),
         stderr: new ReadableStream({
-          start(controller) { controller.close(); },
+          start(controller) {
+            controller.close();
+          },
         }),
-        exited: Promise.resolve(exitCode),
+        exited: Promise.resolve(exit),
       };
     };
   }
 
   test("allows push to owned remote", async () => {
-    process.env.GIT_USER_NAME = "alice";
-    mockGitRemote("https://github.com/alice/repo.git");
+    mockGitSpawn("alice", "https://github.com/alice/repo.git");
     expect(await isOwnedRemotePush("git push origin main")).toBe(true);
-    // Verify spawn was called with --push and correct remote
-    expect(lastSpawnArgs).toEqual(["git", "remote", "get-url", "--push", "origin"]);
+    // Verify both spawn calls
+    expect(spawnCalls[0]).toEqual(["git", "config", "user.name"]);
+    expect(spawnCalls[1]).toEqual([
+      "git",
+      "remote",
+      "get-url",
+      "--push",
+      "origin",
+    ]);
   });
 
   test("denies push to non-owned remote", async () => {
-    process.env.GIT_USER_NAME = "alice";
-    mockGitRemote("https://github.com/upstream-org/repo.git");
+    mockGitSpawn("alice", "https://github.com/upstream-org/repo.git");
     expect(await isOwnedRemotePush("git push origin main")).toBe(false);
   });
 
   test("allows push to owned non-origin remote", async () => {
-    process.env.GIT_USER_NAME = "alice";
-    mockGitRemote("https://github.com/alice/repo.git");
+    mockGitSpawn("alice", "https://github.com/alice/repo.git");
     expect(await isOwnedRemotePush("git push my-fork feature")).toBe(true);
-    // Verify spawn was called with the parsed remote name, not hardcoded "origin"
-    expect(lastSpawnArgs).toEqual(["git", "remote", "get-url", "--push", "my-fork"]);
+    expect(spawnCalls[1]).toEqual([
+      "git",
+      "remote",
+      "get-url",
+      "--push",
+      "my-fork",
+    ]);
   });
 
   test("case-insensitive username match", async () => {
-    process.env.GIT_USER_NAME = "Alice";
-    mockGitRemote("https://github.com/alice/repo.git");
+    mockGitSpawn("Alice", "https://github.com/alice/repo.git");
     expect(await isOwnedRemotePush("git push origin main")).toBe(true);
   });
 
   test("allows force push to owned remote", async () => {
-    process.env.GIT_USER_NAME = "alice";
-    mockGitRemote("https://github.com/alice/repo.git");
+    mockGitSpawn("alice", "https://github.com/alice/repo.git");
     expect(await isOwnedRemotePush("git push --force origin main")).toBe(true);
   });
 
   test("blocks --delete even on owned remote", async () => {
-    process.env.GIT_USER_NAME = "alice";
-    mockGitRemote("https://github.com/alice/repo.git");
-    expect(await isOwnedRemotePush("git push --delete origin feature")).toBe(false);
+    mockGitSpawn("alice", "https://github.com/alice/repo.git");
+    expect(await isOwnedRemotePush("git push --delete origin feature")).toBe(
+      false,
+    );
   });
 
   test("blocks -d even on owned remote", async () => {
-    process.env.GIT_USER_NAME = "alice";
-    mockGitRemote("https://github.com/alice/repo.git");
+    mockGitSpawn("alice", "https://github.com/alice/repo.git");
     expect(await isOwnedRemotePush("git push -d origin feature")).toBe(false);
   });
 
-  test("returns false when GIT_USER_NAME is unset", async () => {
-    delete process.env.GIT_USER_NAME;
-    mockGitRemote("https://github.com/alice/repo.git");
+  test("returns false when git config user.name fails", async () => {
+    mockGitSpawn("", "https://github.com/alice/repo.git", {
+      configExitCode: 1,
+    });
+    expect(await isOwnedRemotePush("git push origin main")).toBe(false);
+  });
+
+  test("returns false when git config user.name is empty", async () => {
+    mockGitSpawn("", "https://github.com/alice/repo.git");
     expect(await isOwnedRemotePush("git push origin main")).toBe(false);
   });
 
-  test("returns false when git command fails", async () => {
-    process.env.GIT_USER_NAME = "alice";
-    mockGitRemote("", 128);
+  test("returns false when git remote get-url fails", async () => {
+    mockGitSpawn("alice", "", { remoteExitCode: 128 });
     expect(await isOwnedRemotePush("git push origin main")).toBe(false);
   });
 
   test("returns false for non-GitHub remote", async () => {
-    process.env.GIT_USER_NAME = "alice";
-    mockGitRemote("https://gitlab.com/alice/repo.git");
+    mockGitSpawn("alice", "https://gitlab.com/alice/repo.git");
     expect(await isOwnedRemotePush("git push origin main")).toBe(false);
   });
 
   test("returns false for bare 'git push' (can't assume origin)", async () => {
-    process.env.GIT_USER_NAME = "alice";
-    mockGitRemote("https://github.com/alice/repo.git");
+    mockGitSpawn("alice", "https://github.com/alice/repo.git");
     expect(await isOwnedRemotePush("git push")).toBe(false);
   });
 
   test("returns false when value-consuming flags are present", async () => {
-    process.env.GIT_USER_NAME = "alice";
-    mockGitRemote("https://github.com/alice/repo.git");
-    expect(await isOwnedRemotePush("git push -o ci.skip origin main")).toBe(false);
+    mockGitSpawn("alice", "https://github.com/alice/repo.git");
+    expect(await isOwnedRemotePush("git push -o ci.skip origin main")).toBe(
+      false,
+    );
   });
 
   test("returns false for non-push commands", async () => {
-    process.env.GIT_USER_NAME = "alice";
-    mockGitRemote("https://github.com/alice/repo.git");
+    mockGitSpawn("alice", "https://github.com/alice/repo.git");
     expect(await isOwnedRemotePush("git status")).toBe(false);
   });
 
   test("handles SSH remote URL", async () => {
-    process.env.GIT_USER_NAME = "alice";
-    mockGitRemote("git@github.com:alice/repo.git");
+    mockGitSpawn("alice", "git@github.com:alice/repo.git");
     expect(await isOwnedRemotePush("git push origin main")).toBe(true);
   });
 
   test("skips -u flag and finds remote", async () => {
-    process.env.GIT_USER_NAME = "alice";
-    mockGitRemote("https://github.com/alice/repo.git");
+    mockGitSpawn("alice", "https://github.com/alice/repo.git");
     expect(await isOwnedRemotePush("git push -u origin feature")).toBe(true);
   });
 
   // SECURITY: pushurl vs url divergence — ownership check must use pushurl
   // because that is the URL git actually pushes to.
   test("uses push URL for ownership check (pushurl security invariant)", async () => {
-    process.env.GIT_USER_NAME = "alice";
-    // Simulate a repo where pushurl differs from url.
-    // The mock returns what --push would return (the pushurl).
-    // If pushurl points to a non-owned remote, the check must fail.
-    mockGitRemote("https://github.com/victim-org/repo.git");
+    mockGitSpawn("alice", "https://github.com/victim-org/repo.git");
     expect(await isOwnedRemotePush("git push origin main")).toBe(false);
   });
 });

approval/check-command.ts

@@ -33,7 +33,8 @@ const GIT_PUSH_RE = /^git\s+push\b/;
 
 // Flags that consume the next token as a value (e.g., -o <value>, --repo <value>).
 // If present, we can't reliably parse the remote name, so we refuse the exemption.
-const VALUE_CONSUMING_FLAGS = /^(-o|--push-option|--repo|--receive-pack|--exec|--signed)$/;
+const VALUE_CONSUMING_FLAGS =
+  /^(-o|--push-option|--repo|--receive-pack|--exec|--signed)$/;
 
 /**
  * Parse the target remote name from a git push command string.
@@ -92,7 +93,7 @@ const HAS_DELETE_FLAG = /\s--delete\b|\s-[a-zA-Z]*d/;
  * Check if a git push command targets a remote owned by GIT_USER_NAME.
  * Returns true if the push should be exempted from block rules.
  *
- * Fail-safe: returns false on any error (missing env var, git failure,
+ * Fail-safe: returns false on any error (missing git config, git failure,
  * unparseable URL, non-GitHub host, --delete flag present).
  */
 export async function isOwnedRemotePush(command: string): Promise<boolean> {
@@ -102,10 +103,26 @@ export async function isOwnedRemotePush(command: string): Promise<boolean> {
   // --delete pushes are never exempted, even to owned remotes
   if (HAS_DELETE_FLAG.test(command)) return false;
 
-  const gitUserName = process.env.GIT_USER_NAME;
-  if (!gitUserName) return false;
-
   try {
+    // Read identity from git config (set during entrypoint from GIT_USER_NAME).
+    // We don't use process.env.GIT_USER_NAME because the env var may not be
+    // available to the approval process at runtime.
+    const configProc = Bun.spawn(["git", "config", "user.name"], {
+      stdout: "pipe",
+      stderr: "pipe",
+    });
+
+    const [configStdout, , configExitCode] = await Promise.all([
+      new Response(configProc.stdout).text(),
+      new Response(configProc.stderr).text(),
+      configProc.exited,
+    ]);
+
+    if (configExitCode !== 0) return false;
+
+    const gitUserName = configStdout.trim();
+    if (!gitUserName) return false;
+
     // SECURITY: --push returns the URL git actually uses for push operations.
     // If pushurl is configured, --push returns pushurl (not url).
     // This ensures we check ownership against the same URL git will push to,