fix(approval): address R4 review feedback on PR #69

- tokenize: normalize shell-quote output to drop {comment} tokens and
  convert {op:"glob"} tokens to their literal pattern strings, preventing
  crashes on non-string objects and ensuring path-based deny rules match
  glob patterns (e.g. rm -rf /tmp/*)
- github-utils: fix extractGitHubRepo regex to capture dots in repo
  names (e.g. socket.io) by using ([^/]+?) with optional .git suffix
  stripping instead of ([^/.]+)
- gh-exemption: handle -R=owner/repo combined short flag form by
  scanning raw args, since parseSegment expands multi-char short flags
  per-character and loses the value
- prescan: remove unreachable { decision: "continue" } variant from
  PrescanResult type, add explicit PrescanLineResult type for prescanLine
- check-command: replace unreachable "lines" in check with
  lines.length === 0 now that normalizeAndSplit always returns lines

323 tests pass (+7 new).

Author: limbibot

approval/__tests__/gh-exemption.test.ts

@@ -55,6 +55,10 @@ describe("extractRepoTarget", () => {
     );
     expect(extractRepoTarget(seg)).toEqual({ owner: "owner", repo: "repo" });
   });
+  test("extracts from -R=owner/repo (combined short flag form)", () => {
+    const seg = parseSegment(["gh", "issue", "list", "-R=owner/repo"], false);
+    expect(extractRepoTarget(seg)).toEqual({ owner: "owner", repo: "repo" });
+  });
   test("returns 'implicit' for gh pr create without --repo", () => {
     const seg = parseSegment(["gh", "pr", "create", "--title", "test"], false);
     expect(extractRepoTarget(seg)).toBe("implicit");

approval/__tests__/github-utils.test.ts

@@ -39,4 +39,21 @@ describe("extractGitHubRepo", () => {
   test("returns null for non-GitHub URL", () => {
     expect(extractGitHubRepo("https://gitlab.com/owner/repo.git")).toBeNull();
   });
+  test("handles repo name with dots (e.g. socket.io)", () => {
+    expect(extractGitHubRepo("https://github.com/socketio/socket.io")).toEqual({
+      owner: "socketio",
+      repo: "socket.io",
+    });
+  });
+  test("handles repo name with dots and .git suffix", () => {
+    expect(extractGitHubRepo("git@github.com:socketio/socket.io.git")).toEqual({
+      owner: "socketio",
+      repo: "socket.io",
+    });
+  });
+  test("handles SSH URL with dots in repo name", () => {
+    expect(
+      extractGitHubRepo("ssh://git@github.com/socketio/socket.io.git"),
+    ).toEqual({ owner: "socketio", repo: "socket.io" });
+  });
 });

approval/__tests__/pipeline.test.ts

@@ -10,7 +10,7 @@ import { evaluateRules } from "../rules";
 function quickEval(command: string): "deny" | "escalate" | "allow" {
   const splitResult = normalizeAndSplit(command);
   if (splitResult.decision === "deny") return "deny";
-  if (!("lines" in splitResult)) return "allow";
+  if (splitResult.lines.length === 0) return "allow";
 
   let worst: "allow" | "escalate" | "deny" = "allow";
 

approval/__tests__/tokenize.test.ts

@@ -47,6 +47,24 @@ describe("tokenize", () => {
     const strings = tokens.filter((t): t is string => typeof t === "string");
     expect(strings[0]).toBe("$SHELL");
   });
+
+  test("drops comment tokens from output", () => {
+    const tokens = tokenize("echo hello # this is a comment");
+    // Comment should be stripped — only string and operator tokens remain
+    expect(tokens).toEqual(["echo", "hello"]);
+  });
+
+  test("converts glob tokens to pattern strings", () => {
+    const tokens = tokenize("echo *.txt");
+    // Glob should become its literal pattern string
+    expect(tokens).toEqual(["echo", "*.txt"]);
+  });
+
+  test("preserves glob pattern with path for deny rule matching", () => {
+    const tokens = tokenize("rm -rf /tmp/*");
+    const strings = tokens.filter((t): t is string => typeof t === "string");
+    expect(strings).toContain("/tmp/*");
+  });
 });
 
 describe("splitSegments", () => {

approval/check-command.ts

@@ -106,7 +106,7 @@ async function evaluateCommand(raw: string): Promise<SegmentResult> {
     return { decision: "deny", reason: splitResult.reason };
   }
 
-  if (!("lines" in splitResult)) {
+  if (splitResult.lines.length === 0) {
     return { decision: "allow", reason: "empty command" };
   }
 

approval/gh-exemption.ts

@@ -39,8 +39,8 @@ export function extractRepoTarget(
     return null;
   }
 
-  // --repo=value or -R=value (from flags map)
-  const repoFlag = flags.get("--repo") ?? flags.get("-R");
+  // --repo=value (from flags map, parsed as --key=value)
+  const repoFlag = flags.get("--repo");
   if (typeof repoFlag === "string") {
     const parts = repoFlag.split("/");
     if (parts.length === 2 && parts[0] && parts[1]) {
@@ -49,6 +49,19 @@ export function extractRepoTarget(
     return null;
   }
 
+  // -R=value — parseSegment expands multi-char short flags per-character,
+  // so -R=owner/repo becomes individual char flags and the value is lost.
+  // Scan raw args for the combined form.
+  const attachedShortRepo = args.find((arg) => arg.startsWith("-R="));
+  if (attachedShortRepo) {
+    const val = attachedShortRepo.slice(3);
+    const parts = val.split("/");
+    if (parts.length === 2 && parts[0] && parts[1]) {
+      return { owner: parts[0], repo: parts[1] };
+    }
+    return null;
+  }
+
   // -R value (next arg) — short flags are expanded, so -R is in flags as true
   // but the value is the next arg after -R in the args array
   const rIdx = args.indexOf("-R");

approval/github-utils.ts

@@ -23,9 +23,9 @@ export function extractGitHubOwner(url: string): string | null {
 
 export function extractGitHubRepo(url: string): RepoTarget | null {
   const patterns = [
-    /https?:\/\/github\.com\/([^/]+)\/([^/.]+)/,
-    /git@github\.com:([^/]+)\/([^/.]+)/,
-    /ssh:\/\/git@github\.com\/([^/]+)\/([^/.]+)/,
+    /https?:\/\/github\.com\/([^/]+)\/([^/]+?)(?:\.git)?$/,
+    /git@github\.com:([^/]+)\/([^/]+?)(?:\.git)?$/,
+    /ssh:\/\/git@github\.com\/([^/]+)\/([^/]+?)(?:\.git)?$/,
   ];
 
   for (const pattern of patterns) {

approval/prescan.ts

@@ -1,8 +1,7 @@
 export type PrescanResult =
   | { decision: "deny"; reason: string }
   | { decision: "escalate"; reason: string }
-  | { decision: "continue"; lines: string[] }
-  | { decision: "continue" };
+  | { decision: "continue"; lines: string[] };
 
 const INVALID_CONTROL_CHARS = /[\x00-\x08\x0b\x0c\x0e-\x1f\x7f]/;
 
@@ -20,7 +19,9 @@ const DEBUG_TELEMETRY = /GH_DEBUG|OTEL_EXPORTER_OTLP_HEADERS/;
 
 export function normalizeAndSplit(
   raw: string,
-): Extract<PrescanResult, { lines: string[] } | { decision: "deny" }> {
+):
+  | { decision: "deny"; reason: string }
+  | { decision: "continue"; lines: string[] } {
   if (raw.includes("\0")) {
     return { decision: "deny", reason: "null byte in command" };
   }
@@ -41,9 +42,12 @@ export function normalizeAndSplit(
   return { decision: "continue", lines };
 }
 
-export function prescanLine(
-  line: string,
-): Exclude<PrescanResult, { lines: string[] }> {
+export type PrescanLineResult =
+  | { decision: "deny"; reason: string }
+  | { decision: "escalate"; reason: string }
+  | { decision: "continue" };
+
+export function prescanLine(line: string): PrescanLineResult {
   if (CREDENTIAL_REF.test(line)) {
     return { decision: "deny", reason: "credential reference in command" };
   }

approval/tokenize.ts

@@ -34,8 +34,43 @@ function preserveEnvReference(key: string): string {
   return `$${key}`;
 }
 
+/**
+ * Normalize raw shell-quote output into strict ShellToken shapes.
+ * shell-quote can emit { comment: string } for # comments and
+ * { op: "glob", pattern: string } for unquoted wildcards. Comments
+ * are dropped; globs are converted to their literal pattern string
+ * so path-based deny rules still match (e.g. /tmp/*).
+ */
+function normalizeToken(token: unknown): ShellToken | null {
+  if (typeof token === "string") return token;
+  if (!token || typeof token !== "object") return null;
+
+  // Drop shell comments — not relevant for rule evaluation
+  if ("comment" in token) return null;
+
+  // Convert glob tokens to literal pattern strings so they appear
+  // in args/positionals for deny rule matching
+  const record = token as Record<string, unknown>;
+  if (record.op === "glob" && typeof record.pattern === "string") {
+    return record.pattern;
+  }
+
+  // Known operator tokens
+  if ("op" in token && typeof (token as { op: unknown }).op === "string") {
+    return { op: (token as { op: string }).op };
+  }
+
+  return null;
+}
+
 export function tokenize(line: string): ShellToken[] {
-  return parse(line, preserveEnvReference) as ShellToken[];
+  const raw = parse(line, preserveEnvReference) as unknown[];
+  const tokens: ShellToken[] = [];
+  for (const token of raw) {
+    const normalized = normalizeToken(token);
+    if (normalized !== null) tokens.push(normalized);
+  }
+  return tokens;
 }
 
 export function splitSegments(tokens: ShellToken[]): RawSegment[] {