fix(git-read): disambiguate revision and path arguments

extensions/git-read/src/args.ts

@@ -4,9 +4,9 @@
  * Everything here is synchronous and side-effect free so the exact argv
  * passed to the child process can be asserted in tests. Revision and path
  * inputs are validated against strict shapes before they are ever placed
- * in an argument list, and every user-controlled value goes after a `--`
- * separator so it can never be parsed as a flag. Only argv subcommands that
- * cannot write are reachable at all.
+ * in an argument list. Validated revisions go before an explicit `--`
+ * separator and paths go after it, so Git never guesses a revision is a
+ * path. Only argv subcommands that cannot write are reachable at all.
  */
 
 export const GIT_TIMEOUT_MS = 10_000;
@@ -84,10 +84,11 @@ export function buildShowArgs(params: GitShowParams): string[] {
     "--no-textconv",
     "--format=fuller",
     params.revision.trim(),
+    "--",
   ];
   if (params.path !== undefined) {
     if (!isSafeRepoPath(params.path)) throw new InvalidPathError(params.path);
-    args.push("--", params.path);
+    args.push(params.path);
   }
   return args;
 }
@@ -126,17 +127,16 @@ export function buildDiffArgs(params: GitDiffParams): string[] {
   if (params.stat) args.push("--stat");
 
   if (from !== undefined && to !== undefined) {
-    // An explicit `--` separator is unnecessary for the range form; a
-    // validated revision can never start with `-` anyway.
     args.push(`${from}...${to}`);
   } else if (from !== undefined) {
     args.push(from);
   }
   // No revisions: worktree vs index (or HEAD with --cached).
 
+  args.push("--");
   if (params.path !== undefined) {
     if (!isSafeRepoPath(params.path)) throw new InvalidPathError(params.path);
-    args.push("--", params.path);
+    args.push(params.path);
   }
   return args;
 }
@@ -163,9 +163,10 @@ export function buildLogArgs(params: GitLogParams): string[] {
     }
     args.push(params.revision.trim());
   }
+  args.push("--");
   if (params.file !== undefined) {
     if (!isSafeRepoPath(params.file)) throw new InvalidPathError(params.file);
-    args.push("--", params.file);
+    args.push(params.file);
   }
   return args;
 }

tests/extensions/git-read/args.test.ts

@@ -18,6 +18,7 @@ test("buildShowArgs validates revisions and paths", () => {
     "--no-textconv",
     "--format=fuller",
     "HEAD",
+    "--",
   ]);
   assert.deepEqual(buildShowArgs({ revision: "  abc123  " }), [
     "show",
@@ -26,6 +27,7 @@ test("buildShowArgs validates revisions and paths", () => {
     "--no-textconv",
     "--format=fuller",
     "abc123",
+    "--",
   ]);
   assert.deepEqual(buildShowArgs({ revision: "HEAD~2", path: "src/a.ts" }), [
     "show",
@@ -76,20 +78,23 @@ test("buildDiffArgs composes only read-only diff forms", () => {
     "--no-color",
     "--no-ext-diff",
     "--no-textconv",
+    "--",
   ]);
   assert.deepEqual(buildDiffArgs({ staged: true }), [
     "diff",
     "--no-color",
     "--no-ext-diff",
     "--no-textconv",
     "--cached",
+    "--",
   ]);
   assert.deepEqual(buildDiffArgs({ from: "main", to: "feat" }), [
     "diff",
     "--no-color",
     "--no-ext-diff",
     "--no-textconv",
     "main...feat",
+    "--",
   ]);
   assert.deepEqual(buildDiffArgs({ from: "HEAD", stat: true, path: "src" }), [
     "diff",
@@ -125,6 +130,7 @@ test("buildLogArgs clamps the limit and validates inputs", () => {
     "--oneline",
     "-n",
     String(GIT_LOG_DEFAULT_LIMIT),
+    "--",
   ]);
   assert.deepEqual(buildLogArgs({ limit: 10_000, revision: "main" }), [
     "log",
@@ -134,6 +140,7 @@ test("buildLogArgs clamps the limit and validates inputs", () => {
     "-n",
     "1000",
     "main",
+    "--",
   ]);
   assert.deepEqual(
     buildLogArgs({ limit: 0, file: "src/a.ts", oneline: false }),

tests/extensions/git-read/index.test.ts

@@ -46,6 +46,7 @@ function makeRepo(): string {
   writeFileSync(join(dir, "b.txt"), "new\n");
   git(dir, ["add", "."]);
   git(dir, ["commit", "--quiet", "-m", "second"]);
+  git(dir, ["branch", "a.txt", "HEAD"]);
   writeFileSync(join(dir, "a.txt"), "one\ntwo\nthree\n");
   return dir;
 }
@@ -105,6 +106,35 @@ test("git show with path limits the commit patch to that path", async () => {
   }
 });
 
+for (const [name, args, expected] of [
+  ["show", buildShowArgs({ revision: "a.txt" }), /second/],
+  ["log", buildLogArgs({ revision: "a.txt" }), /first/],
+  ["diff", buildDiffArgs({ from: "a.txt" }), /\+three/],
+] as const) {
+  test(`git ${name} accepts a revision that also names a worktree file`, async () => {
+    const exit = await Effect.runPromiseExit(runGit(args, repo));
+    assert.ok(Exit.isSuccess(exit));
+    if (Exit.isSuccess(exit)) {
+      assert.match(exit.value.output.preview, expected);
+    }
+  });
+}
+
+for (const [name, args] of [
+  ["show", buildShowArgs({ revision: "b.txt" })],
+  ["log", buildLogArgs({ revision: "b.txt" })],
+  ["diff", buildDiffArgs({ from: "b.txt" })],
+] as const) {
+  test(`git ${name} rejects a file-only name in the revision field`, async () => {
+    const exit = await Effect.runPromiseExit(runGit(args, repo));
+    assert.ok(Exit.isFailure(exit));
+    if (Exit.isFailure(exit)) {
+      const error = Cause.squash(exit.cause) as { message?: string };
+      assert.match(error.message ?? "", /bad revision.*b\.txt/i);
+    }
+  });
+}
+
 test("structured git diff and show never invoke diff.external", async () => {
   const { command, helper, marker } = markerCommand(repo, "external-diff");
   try {