[jsweep] Clean add_reaction.cjs

.github/workflows/jsweep.md

@@ -90,7 +90,7 @@ fi
 - Exclude test files (`*.test.cjs`)
 - Exclude files already listed in `cleaned_files` in the loaded state
 - **Priority 1**: Pick files with `@ts-nocheck` or `// @ts-nocheck` comments (these need type checking enabled)
-- **Priority 2**: If no uncleaned files with `@ts-nocheck` remain, pick the **one file** with the earliest modification timestamp that hasn't been cleaned
+- **Priority 2**: If no uncleaned files with `@ts-nocheck` remain, pick the **one file** whose most recent git commit is oldest, using a deterministic `git log -1 --format='%ct' -- <file>` query over the candidate set sorted by path (do not use filesystem modification timestamps)
 
 If no uncleaned files remain, start over with the oldest cleaned file (reset `cleaned_files` to only the one just chosen).
 

actions/setup/js/add_reaction.cjs

@@ -37,61 +37,102 @@ async function main() {
 
   // Determine the API endpoint based on the event type
   const eventName = context.eventName;
-  const owner = context.repo.owner;
-  const repo = context.repo.repo;
+  const { owner, repo } = context.repo;
 
-  /** @type {string | undefined} */
-  let reactionEndpoint;
+  /** @type {string | null} */
+  const reactionEndpoint = resolveRestEndpoint(eventName, owner, repo);
 
+  if (reactionEndpoint === null) {
+    // GraphQL paths are handled separately; REST validation failures already called setFailed.
+    if (!isRestReactionEvent(eventName)) {
+      await handleGraphQLOrUnknownEvent(eventName, owner, repo, reaction);
+    }
+    return;
+  }
+
+  core.info(`Adding reaction to: ${reactionEndpoint}`);
+  try {
+    await addReaction(reactionEndpoint, reaction);
+  } catch (error) {
+    handleReactionError(error);
+  }
+}
+
+/**
+ * Resolve the REST API endpoint for non-discussion events.
+ * Returns null for discussion/discussion_comment/unsupported events (handled separately).
+ * @param {string} eventName
+ * @param {string} owner
+ * @param {string} repo
+ * @returns {string | null}
+ */
+function resolveRestEndpoint(eventName, owner, repo) {
   switch (eventName) {
     case "issues": {
       const issueNumber = context.payload?.issue?.number;
       if (!issueNumber) {
         core.setFailed(`${ERR_NOT_FOUND}: Issue number not found in event payload`);
-        return;
+        return null;
       }
-      reactionEndpoint = `/repos/${owner}/${repo}/issues/${issueNumber}/reactions`;
-      break;
+      return `/repos/${owner}/${repo}/issues/${issueNumber}/reactions`;
     }
 
     case "issue_comment": {
       const commentId = context.payload?.comment?.id;
       if (!commentId) {
         core.setFailed(`${ERR_VALIDATION}: Comment ID not found in event payload`);
-        return;
+        return null;
       }
-      reactionEndpoint = `/repos/${owner}/${repo}/issues/comments/${commentId}/reactions`;
-      break;
+      return `/repos/${owner}/${repo}/issues/comments/${commentId}/reactions`;
     }
 
     case "pull_request": {
       const prNumber = context.payload?.pull_request?.number;
       if (!prNumber) {
         core.setFailed(`${ERR_NOT_FOUND}: Pull request number not found in event payload`);
-        return;
+        return null;
       }
       // PRs are "issues" for the reactions endpoint
-      reactionEndpoint = `/repos/${owner}/${repo}/issues/${prNumber}/reactions`;
-      break;
+      return `/repos/${owner}/${repo}/issues/${prNumber}/reactions`;
     }
 
     case "pull_request_review_comment": {
       const reviewCommentId = context.payload?.comment?.id;
       if (!reviewCommentId) {
         core.setFailed(`${ERR_VALIDATION}: Review comment ID not found in event payload`);
-        return;
+        return null;
       }
-      reactionEndpoint = `/repos/${owner}/${repo}/pulls/comments/${reviewCommentId}/reactions`;
-      break;
+      return `/repos/${owner}/${repo}/pulls/comments/${reviewCommentId}/reactions`;
     }
 
+    default:
+      return null;
+  }
+}
+
+/**
+ * @param {string} eventName
+ * @returns {boolean}
+ */
+function isRestReactionEvent(eventName) {
+  return ["issues", "issue_comment", "pull_request", "pull_request_review_comment"].includes(eventName);
+}
+
+/**
+ * Handle GraphQL-based reactions (discussion, discussion_comment) and unsupported event types.
+ * @param {string} eventName
+ * @param {string} owner
+ * @param {string} repo
+ * @param {string} reaction
+ */
+async function handleGraphQLOrUnknownEvent(eventName, owner, repo, reaction) {
+  switch (eventName) {
     case "discussion": {
       const discussionNumber = context.payload?.discussion?.number;
       if (!discussionNumber) {
         core.setFailed(`${ERR_NOT_FOUND}: Discussion number not found in event payload`);
         return;
       }
-      // Discussions use GraphQL API - get the node ID
       try {
         const discussionNodeId = await getDiscussionNodeId(owner, repo, discussionNumber);
         await addDiscussionReaction(discussionNodeId, reaction);
@@ -117,17 +158,6 @@ async function main() {
 
     default:
       core.setFailed(`${ERR_VALIDATION}: Unsupported event type: ${eventName}`);
-      return;
-  }
-
-  // Add reaction using REST API (for non-discussion events)
-  // reactionEndpoint is always defined here - all other cases return early
-  if (!reactionEndpoint) return;
-  core.info(`Adding reaction to: ${reactionEndpoint}`);
-  try {
-    await addReaction(reactionEndpoint, reaction);
-  } catch (error) {
-    handleReactionError(error);
   }
 }
 
@@ -220,4 +250,4 @@ async function getDiscussionNodeId(owner, repo, discussionNumber) {
   return repository.discussion.id;
 }
 
-module.exports = { main, addReaction, addDiscussionReaction, getDiscussionNodeId, handleReactionError, REACTION_MAP };
+module.exports = { main, addReaction, addDiscussionReaction, getDiscussionNodeId, handleReactionError, resolveRestEndpoint, handleGraphQLOrUnknownEvent, REACTION_MAP };

actions/setup/js/add_reaction.test.cjs

@@ -165,6 +165,7 @@ describe("add_reaction", () => {
       await runScript();
 
       expect(mockCore.setFailed).toHaveBeenCalledWith(`${ERR_NOT_FOUND}: Issue number not found in event payload`);
+      expect(mockCore.setFailed).toHaveBeenCalledTimes(1);
       expect(mockGithub.request).not.toHaveBeenCalled();
     });
   });
@@ -650,4 +651,112 @@ describe("add_reaction", () => {
       expect(mockCore.setFailed).toHaveBeenCalledWith(expect.stringContaining(`${ERR_API}: Failed to add reaction`));
     });
   });
+
+  describe("resolveRestEndpoint (direct)", () => {
+    it("should return issues endpoint", async () => {
+      global.context = { eventName: "issues", repo: { owner: "o", repo: "r" }, payload: { issue: { number: 1 } } };
+      const { resolveRestEndpoint } = await importHelpers();
+      expect(resolveRestEndpoint("issues", "o", "r")).toBe("/repos/o/r/issues/1/reactions");
+    });
+
+    it("should return null and setFailed when issue number is missing", async () => {
+      global.context = { eventName: "issues", repo: { owner: "o", repo: "r" }, payload: {} };
+      const { resolveRestEndpoint } = await importHelpers();
+      expect(resolveRestEndpoint("issues", "o", "r")).toBeNull();
+      expect(mockCore.setFailed).toHaveBeenCalledWith(expect.stringContaining("Issue number not found"));
+    });
+
+    it("should return issue_comment endpoint", async () => {
+      global.context = { eventName: "issue_comment", repo: { owner: "o", repo: "r" }, payload: { comment: { id: 42 } } };
+      const { resolveRestEndpoint } = await importHelpers();
+      expect(resolveRestEndpoint("issue_comment", "o", "r")).toBe("/repos/o/r/issues/comments/42/reactions");
+    });
+
+    it("should return null and setFailed when comment id is missing", async () => {
+      global.context = { eventName: "issue_comment", repo: { owner: "o", repo: "r" }, payload: {} };
+      const { resolveRestEndpoint } = await importHelpers();
+      expect(resolveRestEndpoint("issue_comment", "o", "r")).toBeNull();
+      expect(mockCore.setFailed).toHaveBeenCalledWith(expect.stringContaining("Comment ID not found"));
+    });
+
+    it("should return pull_request endpoint using issues path", async () => {
+      global.context = { eventName: "pull_request", repo: { owner: "o", repo: "r" }, payload: { pull_request: { number: 7 } } };
+      const { resolveRestEndpoint } = await importHelpers();
+      expect(resolveRestEndpoint("pull_request", "o", "r")).toBe("/repos/o/r/issues/7/reactions");
+    });
+
+    it("should return null and setFailed when PR number is missing", async () => {
+      global.context = { eventName: "pull_request", repo: { owner: "o", repo: "r" }, payload: {} };
+      const { resolveRestEndpoint } = await importHelpers();
+      expect(resolveRestEndpoint("pull_request", "o", "r")).toBeNull();
+      expect(mockCore.setFailed).toHaveBeenCalledWith(expect.stringContaining("Pull request number not found"));
+    });
+
+    it("should return pull_request_review_comment endpoint", async () => {
+      global.context = { eventName: "pull_request_review_comment", repo: { owner: "o", repo: "r" }, payload: { comment: { id: 55 } } };
+      const { resolveRestEndpoint } = await importHelpers();
+      expect(resolveRestEndpoint("pull_request_review_comment", "o", "r")).toBe("/repos/o/r/pulls/comments/55/reactions");
+    });
+
+    it("should return null for discussion events (handled via GraphQL)", async () => {
+      global.context = { eventName: "discussion", repo: { owner: "o", repo: "r" }, payload: { discussion: { number: 3 } } };
+      const { resolveRestEndpoint } = await importHelpers();
+      expect(resolveRestEndpoint("discussion", "o", "r")).toBeNull();
+      expect(mockCore.setFailed).not.toHaveBeenCalled();
+    });
+
+    it("should return null for unknown events", async () => {
+      global.context = { eventName: "push", repo: { owner: "o", repo: "r" }, payload: {} };
+      const { resolveRestEndpoint } = await importHelpers();
+      expect(resolveRestEndpoint("push", "o", "r")).toBeNull();
+      expect(mockCore.setFailed).not.toHaveBeenCalled();
+    });
+  });
+
+  describe("handleGraphQLOrUnknownEvent (direct)", () => {
+    beforeEach(() => {
+      mockGithub.graphql.mockImplementation(query => {
+        if (query.includes("query")) {
+          return Promise.resolve({ repository: { discussion: { id: "D_abc" } } });
+        }
+        return Promise.resolve({ addReaction: { reaction: { id: "R_xyz", content: "EYES" } } });
+      });
+    });
+
+    it("should handle discussion event with GraphQL", async () => {
+      global.context = { eventName: "discussion", repo: { owner: "o", repo: "r" }, payload: { discussion: { number: 5 } } };
+      const { handleGraphQLOrUnknownEvent } = await importHelpers();
+      await handleGraphQLOrUnknownEvent("discussion", "o", "r", "eyes");
+      expect(mockGithub.graphql).toHaveBeenCalledTimes(2);
+      expect(mockCore.setOutput).toHaveBeenCalledWith("reaction-id", "R_xyz");
+    });
+
+    it("should setFailed when discussion number missing", async () => {
+      global.context = { eventName: "discussion", repo: { owner: "o", repo: "r" }, payload: {} };
+      const { handleGraphQLOrUnknownEvent } = await importHelpers();
+      await handleGraphQLOrUnknownEvent("discussion", "o", "r", "eyes");
+      expect(mockCore.setFailed).toHaveBeenCalledWith(expect.stringContaining("Discussion number not found"));
+    });
+
+    it("should handle discussion_comment event with GraphQL", async () => {
+      global.context = { eventName: "discussion_comment", repo: { owner: "o", repo: "r" }, payload: { comment: { node_id: "DC_abc" } } };
+      const { handleGraphQLOrUnknownEvent } = await importHelpers();
+      await handleGraphQLOrUnknownEvent("discussion_comment", "o", "r", "heart");
+      expect(mockGithub.graphql).toHaveBeenCalledWith(expect.stringContaining("mutation"), expect.objectContaining({ content: "HEART" }));
+    });
+
+    it("should setFailed when discussion_comment node_id missing", async () => {
+      global.context = { eventName: "discussion_comment", repo: { owner: "o", repo: "r" }, payload: {} };
+      const { handleGraphQLOrUnknownEvent } = await importHelpers();
+      await handleGraphQLOrUnknownEvent("discussion_comment", "o", "r", "eyes");
+      expect(mockCore.setFailed).toHaveBeenCalledWith(expect.stringContaining("Discussion comment node ID not found"));
+    });
+
+    it("should setFailed for unknown event types", async () => {
+      global.context = { eventName: "push", repo: { owner: "o", repo: "r" }, payload: {} };
+      const { handleGraphQLOrUnknownEvent } = await importHelpers();
+      await handleGraphQLOrUnknownEvent("push", "o", "r", "eyes");
+      expect(mockCore.setFailed).toHaveBeenCalledWith(expect.stringContaining("Unsupported event type: push"));
+    });
+  });
 });

pkg/workflow/jsweep_workflow_test.go

@@ -122,7 +122,23 @@ func TestJSweepWorkflowConfiguration(t *testing.T) {
 		}
 	})
 
-	// Test 9: Verify the workflow has instructions to remove @ts-nocheck
+	// Test 9: Verify the fallback selection is deterministic
+	t.Run("UsesDeterministicFallbackSelection", func(t *testing.T) {
+		if !strings.Contains(mdContent, "git log -1 --format='%ct'") {
+			t.Error("jsweep workflow should use a deterministic git query for fallback file selection")
+		}
+		if !strings.Contains(mdContent, "most recent git commit is oldest") {
+			t.Error("jsweep workflow should tie the git query to choosing the oldest cleanup candidate")
+		}
+		if !strings.Contains(mdContent, "sorted by path") {
+			t.Error("jsweep workflow should sort candidate files by path before fallback selection")
+		}
+		if strings.Contains(mdContent, "earliest modification timestamp") {
+			t.Error("jsweep workflow should not use filesystem modification timestamps for fallback selection")
+		}
+	})
+
+	// Test 10: Verify the workflow has instructions to remove @ts-nocheck
 	t.Run("RemovesTsNocheck", func(t *testing.T) {
 		if !strings.Contains(mdContent, "Remove `@ts-nocheck`") {
 			t.Error("jsweep workflow should have instructions to remove @ts-nocheck")
@@ -135,7 +151,7 @@ func TestJSweepWorkflowConfiguration(t *testing.T) {
 		}
 	})
 
-	// Test 10: Verify the workflow has a valid lock file
+	// Test 11: Verify the workflow has a valid lock file
 	t.Run("HasValidLockFile", func(t *testing.T) {
 		lockPath := filepath.Join("..", "..", ".github", "workflows", "jsweep.lock.yml")
 		_, err := os.Stat(lockPath)
@@ -144,7 +160,7 @@ func TestJSweepWorkflowConfiguration(t *testing.T) {
 		}
 	})
 
-	// Test 11: Verify the workflow has explicit done conditions to prevent runaway loops
+	// Test 12: Verify the workflow has explicit done conditions to prevent runaway loops
 	t.Run("HasDoneConditions", func(t *testing.T) {
 		if !strings.Contains(mdContent, "Done Conditions") {
 			t.Error("jsweep workflow should have a 'Done Conditions' section to prevent runaway iteration")
@@ -157,14 +173,14 @@ func TestJSweepWorkflowConfiguration(t *testing.T) {
 		}
 	})
 
-	// Test 12: Verify the one-file-per-run constraint includes stop instruction
+	// Test 13: Verify the one-file-per-run constraint includes stop instruction
 	t.Run("OneFilePerRunStopsAfterPR", func(t *testing.T) {
 		if !strings.Contains(mdContent, "after calling `create_pull_request`, STOP immediately") {
 			t.Error("jsweep workflow one-file-per-run constraint should include explicit stop instruction after PR creation")
 		}
 	})
 
-	// Test 13: Verify the workflow uses lean GitHub tooling and no Serena import
+	// Test 14: Verify the workflow uses lean GitHub tooling and no Serena import
 	t.Run("UsesReposToolsetWithoutSerenaImport", func(t *testing.T) {
 		if !strings.Contains(mdContent, "toolsets: [repos]") {
 			t.Error("jsweep workflow should scope github tools to [repos]")
@@ -174,7 +190,7 @@ func TestJSweepWorkflowConfiguration(t *testing.T) {
 		}
 	})
 
-	// Test 14: Verify workflow permissions follow least privilege
+	// Test 15: Verify workflow permissions follow least privilege
 	t.Run("LeastPrivilegePermissions", func(t *testing.T) {
 		if !strings.Contains(mdContent, "contents: read") || !strings.Contains(mdContent, "actions: read") {
 			t.Error("jsweep workflow should keep contents: read and actions: read permissions")
@@ -184,7 +200,7 @@ func TestJSweepWorkflowConfiguration(t *testing.T) {
 		}
 	})
 
-	// Test 15: Verify validation instructions are batched into one command
+	// Test 16: Verify validation instructions are batched into one command
 	t.Run("BatchedValidationCommand", func(t *testing.T) {
 		if !strings.Contains(mdContent, expectedJSweepBatchedValidationCommand) {
 			t.Error("jsweep workflow should batch validation commands into a single chained command")