fix(session): reject ambiguous navigate targets

.changeset/tidy-moons-invite.md

@@ -0,0 +1,5 @@
+---
+"hunkdiff": patch
+---
+
+Reject ambiguous `hunk session navigate` requests instead of silently ignoring the extra target.

src/app/cli.test.ts

@@ -1432,6 +1432,32 @@ describe("parseCli", () => {
     }
   });
 
+  test("rejects session navigate when a comment direction is combined with an absolute target", async () => {
+    const conflictingOptions = [
+      ["--file", "README.md"],
+      ["--hunk", "1"],
+      ["--old-line", "10"],
+      ["--new-line", "10"],
+      ["--file", "README.md", "--hunk", "2"],
+    ];
+
+    for (const direction of ["--next-comment", "--prev-comment"]) {
+      for (const conflictingOption of conflictingOptions) {
+        await expect(
+          parseCli([
+            "bun",
+            "hunk",
+            "session",
+            "navigate",
+            "session-1",
+            direction,
+            ...conflictingOption,
+          ]),
+        ).rejects.toThrow("Specify exactly one navigation selector");
+      }
+    }
+  });
+
   test("rejects session navigate with both --next-comment and --prev-comment", async () => {
     await expect(
       parseCli([

src/app/cli.ts

@@ -1159,16 +1159,18 @@ async function parseSessionNavigateCommand(tokens: string[]): Promise<ParsedCliI
 
   await parseStandaloneCommand(command, tokens);
 
-  // A comment id is resolved by the daemon and must not silently discard another selector.
+  // A comment id or a direction is resolved by the daemon and must not silently discard another
+  // selector, so every selector combination is rejected rather than ranked.
+  const hasAbsoluteTarget =
+    parsedOptions.file !== undefined ||
+    parsedOptions.hunk !== undefined ||
+    parsedOptions.oldLine !== undefined ||
+    parsedOptions.newLine !== undefined;
+  const hasCommentDirection =
+    parsedOptions.nextComment === true || parsedOptions.prevComment === true;
   const commentHasConflictingSelector =
-    parsedOptions.comment !== undefined &&
-    (parsedOptions.file !== undefined ||
-      parsedOptions.hunk !== undefined ||
-      parsedOptions.oldLine !== undefined ||
-      parsedOptions.newLine !== undefined ||
-      parsedOptions.nextComment === true ||
-      parsedOptions.prevComment === true);
-  if (commentHasConflictingSelector) {
+    parsedOptions.comment !== undefined && (hasAbsoluteTarget || hasCommentDirection);
+  if (commentHasConflictingSelector || (hasCommentDirection && hasAbsoluteTarget)) {
     throw new Error(
       "Specify exactly one navigation selector: --comment, --next-comment / --prev-comment, or --file with a navigation target.",
     );

src/session/broker/brokerServer.helpers.test.ts

@@ -278,6 +278,7 @@ describe("handleSessionApiRequest", () => {
       apiRequest({
         action: "navigate",
         selector: { sessionId: "s-1" },
+        filePath: "a.ts",
         hunkNumber: 2,
       } as SessionDaemonRequest),
     );
@@ -385,6 +386,66 @@ describe("handleSessionApiRequest", () => {
     });
   });
 
+  test("rejects a comment direction combined with another navigation target", async () => {
+    const { state } = createFakeState();
+    const response = await handleSessionApiRequest(
+      state,
+      apiRequest({
+        action: "navigate",
+        selector: { sessionId: "s-1" },
+        commentDirection: "next",
+        filePath: "a.ts",
+        hunkNumber: 2,
+      } as SessionDaemonRequest),
+    );
+
+    expect(response.status).toBe(400);
+    expect(await response.json()).toMatchObject({
+      error: expect.stringContaining("commentDirection cannot be combined"),
+    });
+  });
+
+  test("rejects a hunk or line target without a file path", async () => {
+    const { state } = createFakeState();
+    for (const target of [{ hunkNumber: 2 }, { side: "new" as const, line: 12 }]) {
+      const response = await handleSessionApiRequest(
+        state,
+        apiRequest({
+          action: "navigate",
+          selector: { sessionId: "s-1" },
+          ...target,
+        } as SessionDaemonRequest),
+      );
+
+      expect(response.status).toBe(400);
+      expect(await response.json()).toMatchObject({
+        error: expect.stringContaining("requires filePath"),
+      });
+    }
+  });
+
+  test("prefers exact line coordinates when a hunk number is also supplied", async () => {
+    const { state, calls } = createFakeState();
+    const response = await handleSessionApiRequest(
+      state,
+      apiRequest({
+        action: "navigate",
+        selector: { sessionId: "s-1" },
+        filePath: "a.ts",
+        hunkNumber: 2,
+        side: "new",
+        line: 12,
+      } as SessionDaemonRequest),
+    );
+
+    expect(response.status).toBe(200);
+    const dispatch = calls.find((c) => c.method === "dispatchCommand");
+    expect(dispatch).toBeDefined();
+    expect((dispatch!.args[0] as { input: Record<string, unknown> }).input).toMatchObject({
+      line: 12,
+    });
+  });
+
   test("dispatches reload, comment-add, comment-rm, and comment-clear commands", async () => {
     const { state, calls } = createFakeState();
     const requests: SessionDaemonRequest[] = [

src/session/broker/brokerServer.ts

@@ -279,6 +279,15 @@ function resolveNavigateCommandInput(
     };
   }
 
+  const hasAbsoluteTarget =
+    input.filePath !== undefined ||
+    input.hunkNumber !== undefined ||
+    input.side !== undefined ||
+    input.line !== undefined;
+  if (input.commentDirection !== undefined && hasAbsoluteTarget) {
+    throw new Error("navigate commentDirection cannot be combined with another navigation target.");
+  }
+
   if (
     !input.commentDirection &&
     input.hunkNumber === undefined &&
@@ -289,6 +298,14 @@ function resolveNavigateCommandInput(
     );
   }
 
+  // The live terminal cannot resolve a hunk or a line without the file that owns it.
+  if (
+    input.filePath === undefined &&
+    (input.hunkNumber !== undefined || input.line !== undefined)
+  ) {
+    throw new Error("navigate requires filePath for a hunk or line target.");
+  }
+
   // Exact coordinates take precedence so callers reveal the row rather than only its hunk.
   const hasExactLineTarget = input.side !== undefined && input.line !== undefined;
   return {