Move validateExclusiveModes before getBody for fail-fast validation (#1663)

* Initial plan

* feat: refactor option validation to list-based approach and add delete+hide_and_recreate check

Co-authored-by: marocchino <128431+marocchino@users.noreply.github.com>

* feat: add only_update to exclusive modes validation list

Co-authored-by: marocchino <128431+marocchino@users.noreply.github.com>

* refactor: extract validation logic into src/validate.ts

Co-authored-by: marocchino <128431+marocchino@users.noreply.github.com>

* refactor: move validateExclusiveModes before getBody in run()

Co-authored-by: marocchino <128431+marocchino@users.noreply.github.com>

* test: assert getBody is not called when validateExclusiveModes fails

Co-authored-by: marocchino <128431+marocchino@users.noreply.github.com>

---------

Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: marocchino <128431+marocchino@users.noreply.github.com>

__tests__/main.test.ts

@@ -115,8 +115,9 @@ describe("run", () => {
     mockConfig.recreate = true
     const {core} = await runMain()
     expect(core.setFailed).toHaveBeenCalledWith(
-      "delete and recreate cannot be both set to true",
+      "delete and recreate cannot be set to true simultaneously",
     )
+    expect(mockConfig.getBody).not.toHaveBeenCalled()
   })
 
   test("fails when deleteOldComment and onlyCreateComment are both true", async () => {
@@ -124,8 +125,9 @@ describe("run", () => {
     mockConfig.onlyCreateComment = true
     const {core} = await runMain()
     expect(core.setFailed).toHaveBeenCalledWith(
-      "delete and only_create cannot be both set to true",
+      "delete and only_create cannot be set to true simultaneously",
     )
+    expect(mockConfig.getBody).not.toHaveBeenCalled()
   })
 
   test("fails when deleteOldComment and hideOldComment are both true", async () => {
@@ -133,8 +135,9 @@ describe("run", () => {
     mockConfig.hideOldComment = true
     const {core} = await runMain()
     expect(core.setFailed).toHaveBeenCalledWith(
-      "delete and hide cannot be both set to true",
+      "delete and hide cannot be set to true simultaneously",
     )
+    expect(mockConfig.getBody).not.toHaveBeenCalled()
   })
 
   test("fails when onlyCreateComment and onlyUpdateComment are both true", async () => {
@@ -142,8 +145,9 @@ describe("run", () => {
     mockConfig.onlyUpdateComment = true
     const {core} = await runMain()
     expect(core.setFailed).toHaveBeenCalledWith(
-      "only_create and only_update cannot be both set to true",
+      "only_create and only_update cannot be set to true simultaneously",
     )
+    expect(mockConfig.getBody).not.toHaveBeenCalled()
   })
 
   test("fails when hideOldComment and hideAndRecreate are both true", async () => {
@@ -151,8 +155,19 @@ describe("run", () => {
     mockConfig.hideAndRecreate = true
     const {core} = await runMain()
     expect(core.setFailed).toHaveBeenCalledWith(
-      "hide and hide_and_recreate cannot be both set to true",
+      "hide and hide_and_recreate cannot be set to true simultaneously",
     )
+    expect(mockConfig.getBody).not.toHaveBeenCalled()
+  })
+
+  test("fails when deleteOldComment and hideAndRecreate are both true", async () => {
+    mockConfig.deleteOldComment = true
+    mockConfig.hideAndRecreate = true
+    const {core} = await runMain()
+    expect(core.setFailed).toHaveBeenCalledWith(
+      "delete and hide_and_recreate cannot be set to true simultaneously",
+    )
+    expect(mockConfig.getBody).not.toHaveBeenCalled()
   })
 
   test("deletes previous comment when deleteOldComment is true and previous comment exists", async () => {

__tests__/validate.test.ts

@@ -0,0 +1,85 @@
+import {describe, expect, test} from "vitest"
+import {validateBody, validateExclusiveModes} from "../src/validate"
+
+describe("validateBody", () => {
+  test("throws when body is empty and neither delete nor hide is set", () => {
+    expect(() => validateBody("", false, false)).toThrow(
+      "Either message or path input is required",
+    )
+  })
+
+  test("does not throw when body is provided", () => {
+    expect(() => validateBody("some body", false, false)).not.toThrow()
+  })
+
+  test("does not throw when body is empty but deleteOldComment is true", () => {
+    expect(() => validateBody("", true, false)).not.toThrow()
+  })
+
+  test("does not throw when body is empty but hideOldComment is true", () => {
+    expect(() => validateBody("", false, true)).not.toThrow()
+  })
+})
+
+describe("validateExclusiveModes", () => {
+  test("does not throw when no modes are enabled", () => {
+    expect(() => validateExclusiveModes(false, false, false, false, false, false)).not.toThrow()
+  })
+
+  test("does not throw when exactly one mode is enabled", () => {
+    expect(() => validateExclusiveModes(true, false, false, false, false, false)).not.toThrow()
+    expect(() => validateExclusiveModes(false, true, false, false, false, false)).not.toThrow()
+    expect(() => validateExclusiveModes(false, false, true, false, false, false)).not.toThrow()
+    expect(() => validateExclusiveModes(false, false, false, true, false, false)).not.toThrow()
+    expect(() => validateExclusiveModes(false, false, false, false, true, false)).not.toThrow()
+    expect(() => validateExclusiveModes(false, false, false, false, false, true)).not.toThrow()
+  })
+
+  test("throws when delete and recreate are both true", () => {
+    expect(() => validateExclusiveModes(true, true, false, false, false, false)).toThrow(
+      "delete and recreate cannot be set to true simultaneously",
+    )
+  })
+
+  test("throws when delete and only_create are both true", () => {
+    expect(() => validateExclusiveModes(true, false, true, false, false, false)).toThrow(
+      "delete and only_create cannot be set to true simultaneously",
+    )
+  })
+
+  test("throws when delete and only_update are both true", () => {
+    expect(() => validateExclusiveModes(true, false, false, true, false, false)).toThrow(
+      "delete and only_update cannot be set to true simultaneously",
+    )
+  })
+
+  test("throws when delete and hide are both true", () => {
+    expect(() => validateExclusiveModes(true, false, false, false, true, false)).toThrow(
+      "delete and hide cannot be set to true simultaneously",
+    )
+  })
+
+  test("throws when delete and hide_and_recreate are both true", () => {
+    expect(() => validateExclusiveModes(true, false, false, false, false, true)).toThrow(
+      "delete and hide_and_recreate cannot be set to true simultaneously",
+    )
+  })
+
+  test("throws when only_create and only_update are both true", () => {
+    expect(() => validateExclusiveModes(false, false, true, true, false, false)).toThrow(
+      "only_create and only_update cannot be set to true simultaneously",
+    )
+  })
+
+  test("throws when hide and hide_and_recreate are both true", () => {
+    expect(() => validateExclusiveModes(false, false, false, false, true, true)).toThrow(
+      "hide and hide_and_recreate cannot be set to true simultaneously",
+    )
+  })
+
+  test("uses Oxford comma when three or more modes are enabled", () => {
+    expect(() => validateExclusiveModes(true, true, true, false, false, false)).toThrow(
+      "delete, recreate, and only_create cannot be set to true simultaneously",
+    )
+  })
+})

src/main.ts

@@ -27,6 +27,7 @@ import {
   repo,
   skipUnchanged,
 } from "./config"
+import {validateBody, validateExclusiveModes} from "./validate"
 
 async function run(): Promise<undefined> {
   if (Number.isNaN(pullRequestNumber) || pullRequestNumber < 1) {
@@ -35,6 +36,15 @@ async function run(): Promise<undefined> {
   }
 
   try {
+    validateExclusiveModes(
+      deleteOldComment,
+      recreate,
+      onlyCreateComment,
+      onlyUpdateComment,
+      hideOldComment,
+      hideAndRecreate,
+    )
+
     const body = await getBody()
 
     if (!body && ignoreEmpty) {
@@ -42,29 +52,7 @@ async function run(): Promise<undefined> {
       return
     }
 
-    if (!deleteOldComment && !hideOldComment && !body) {
+    validateBody(body, deleteOldComment, hideOldComment)
-      throw new Error("Either message or path input is required")
-    }
-
-    if (deleteOldComment && recreate) {
-      throw new Error("delete and recreate cannot be both set to true")
-    }
-
-    if (deleteOldComment && onlyCreateComment) {
-      throw new Error("delete and only_create cannot be both set to true")
-    }
-
-    if (deleteOldComment && hideOldComment) {
-      throw new Error("delete and hide cannot be both set to true")
-    }
-
-    if (onlyCreateComment && onlyUpdateComment) {
-      throw new Error("only_create and only_update cannot be both set to true")
-    }
-
-    if (hideOldComment && hideAndRecreate) {
-      throw new Error("hide and hide_and_recreate cannot be both set to true")
-    }
 
     const octokit = github.getOctokit(githubToken)
     const previous = await findPreviousComment(octokit, repo, pullRequestNumber, header)

src/validate.ts

@@ -0,0 +1,35 @@
+export function validateBody(
+  body: string,
+  deleteOldComment: boolean,
+  hideOldComment: boolean,
+): void {
+  if (!deleteOldComment && !hideOldComment && !body) {
+    throw new Error("Either message or path input is required")
+  }
+}
+
+export function validateExclusiveModes(
+  deleteOldComment: boolean,
+  recreate: boolean,
+  onlyCreateComment: boolean,
+  onlyUpdateComment: boolean,
+  hideOldComment: boolean,
+  hideAndRecreate: boolean,
+): void {
+  const exclusiveModes: [string, boolean][] = [
+    ["delete", deleteOldComment],
+    ["recreate", recreate],
+    ["only_create", onlyCreateComment],
+    ["only_update", onlyUpdateComment],
+    ["hide", hideOldComment],
+    ["hide_and_recreate", hideAndRecreate],
+  ]
+  const enabledModes = exclusiveModes.filter(([, flag]) => flag).map(([name]) => name)
+  if (enabledModes.length > 1) {
+    const last = enabledModes[enabledModes.length - 1]
+    const rest = enabledModes.slice(0, -1)
+    const joined =
+      enabledModes.length === 2 ? `${rest[0]} and ${last}` : `${rest.join(", ")}, and ${last}`
+    throw new Error(`${joined} cannot be set to true simultaneously`)
+  }
+}