diff --git a/.changeset/tidy-moons-invite.md b/.changeset/tidy-moons-invite.md new file mode 100644 index 000000000..ea5dab854 --- /dev/null +++ b/.changeset/tidy-moons-invite.md @@ -0,0 +1,5 @@ +--- +"hunkdiff": patch +--- + +Reject ambiguous `hunk session navigate` requests instead of silently ignoring the extra target. diff --git a/src/app/cli.test.ts b/src/app/cli.test.ts index dd9333691..e2a84b457 100644 --- a/src/app/cli.test.ts +++ b/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([ diff --git a/src/app/cli.ts b/src/app/cli.ts index 4c6d4483b..e02890ea1 100644 --- a/src/app/cli.ts +++ b/src/app/cli.ts @@ -1159,16 +1159,18 @@ async function parseSessionNavigateCommand(tokens: string[]): Promise { 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 }).input).toMatchObject({ + line: 12, + }); + }); + test("dispatches reload, comment-add, comment-rm, and comment-clear commands", async () => { const { state, calls } = createFakeState(); const requests: SessionDaemonRequest[] = [ diff --git a/src/session/broker/brokerServer.ts b/src/session/broker/brokerServer.ts index b2b0406cc..f8b4f7c3c 100644 --- a/src/session/broker/brokerServer.ts +++ b/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 {