#234: Fix code review findings — 5 HIGH, 8 MEDIUM, 6 LOW severity

.gitignore

@@ -21,6 +21,9 @@ coverage/
 docs/codingbuddy/
 .omc/
 
+# Git worktrees
+.worktrees/
+
 # TypeScript build cache
 *.tsbuildinfo
 

eslint.config.mjs

@@ -6,7 +6,7 @@ import prettier from "eslint-config-prettier";
 
 export default tseslint.config(
   {
-    ignores: ["dist/**", "node_modules/**", "*.config.{js,mjs,ts}"],
+    ignores: ["dist/**", "node_modules/**", "coverage/**", "*.config.{js,mjs,ts}"],
   },
   eslint.configs.recommended,
   ...tseslint.configs.recommendedTypeChecked,

src/config.test.ts

@@ -664,7 +664,6 @@ describe("config", () => {
             "FIRST_TIME_CONTRIBUTOR",
             "FIRST_TIMER",
             "MANNEQUIN",
-            "NONE",
           ],
         };
         const inputs: ActionInputs = {
@@ -685,7 +684,6 @@ describe("config", () => {
           "FIRST_TIME_CONTRIBUTOR",
           "FIRST_TIMER",
           "MANNEQUIN",
-          "NONE",
         ]);
       });
 
@@ -760,7 +758,22 @@ describe("config", () => {
         };
 
         expect(() => mergeConfig(fileConfig, inputs)).toThrow(
-          "Must be one of: OWNER, MEMBER, COLLABORATOR, CONTRIBUTOR, FIRST_TIME_CONTRIBUTOR, FIRST_TIMER, MANNEQUIN, NONE",
+          "Must be one of: OWNER, MEMBER, COLLABORATOR, CONTRIBUTOR, FIRST_TIME_CONTRIBUTOR, FIRST_TIMER, MANNEQUIN",
+        );
+      });
+
+      it("should reject NONE in authorized_approvers", () => {
+        const fileConfig = { authorized_approvers: ["OWNER", "NONE"] };
+        const inputs: ActionInputs = {
+          mode: "plan",
+          anthropic_api_key: "test-key",
+          github_token: "test-token",
+          config_path: ".leonidas.yml",
+          system_prompt_path: ".github/leonidas.md",
+        };
+
+        expect(() => mergeConfig(fileConfig, inputs)).toThrow(
+          'Invalid authorized_approvers value: "NONE" is not allowed',
         );
       });
     });

src/config.ts

@@ -1,5 +1,6 @@
 import * as core from "@actions/core";
 import * as fs from "fs";
+import * as path from "path";
 import * as yaml from "js-yaml";
 import { LeonidasConfig, ActionInputs } from "./types";
 import { resolveLanguage } from "./i18n";
@@ -159,9 +160,13 @@ export function mergeConfig(
     "FIRST_TIME_CONTRIBUTOR",
     "FIRST_TIMER",
     "MANNEQUIN",
-    "NONE",
   ];
   for (const approver of merged.authorized_approvers) {
+    if (approver === "NONE") {
+      throw new Error(
+        'Invalid authorized_approvers value: "NONE" is not allowed as it represents unauthenticated users. Use valid associations like OWNER, MEMBER, or COLLABORATOR instead.',
+      );
+    }
     if (!validAssociations.includes(approver)) {
       throw new Error(
         `Invalid authorized_approvers value: "${approver}". Must be one of: ${validAssociations.join(", ")}`,
@@ -190,7 +195,7 @@ export function loadRules(rulesPath: string): Record<string, string> {
 
     const rules: Record<string, string> = {};
     for (const file of mdFiles) {
-      const filePath = `${rulesPath}/${file}`;
+      const filePath = path.join(rulesPath, file);
       const ruleName = file.replace(/\.md$/, "");
       try {
         const content = fs.readFileSync(filePath, "utf-8");

src/github.test.ts

@@ -1,7 +1,12 @@
 import { describe, it, expect, vi, beforeEach, afterEach } from "vitest";
 import * as github from "@actions/github";
 import * as core from "@actions/core";
-import { parseSubIssueMetadata, isDecomposedPlan, createGitHubClient } from "./github";
+import {
+  parseSubIssueMetadata,
+  isDecomposedPlan,
+  createGitHubClient,
+  clearOctokitCache,
+} from "./github";
 import { PLAN_HEADER, PLAN_MARKER, DECOMPOSED_MARKER } from "./templates/plan_comment";
 
 vi.mock("@actions/core");
@@ -17,6 +22,7 @@ describe("github", () => {
 
   beforeEach(() => {
     vi.clearAllMocks();
+    clearOctokitCache();
 
     mockOctokit = {
       paginate: vi.fn(),
@@ -51,10 +57,10 @@ describe("github", () => {
   describe("findPlanComment", () => {
     it("should return the latest plan comment when multiple exist", async () => {
       const comments = [
-        { id: 1, body: "Regular comment" },
-        { id: 2, body: `${PLAN_HEADER}\n\nFirst plan` },
-        { id: 3, body: "Another regular comment" },
-        { id: 4, body: `${PLAN_HEADER}\n\nLatest plan` },
+        { id: 1, body: "Regular comment", user: { login: "github-actions[bot]" } },
+        { id: 2, body: `${PLAN_HEADER}\n\nFirst plan`, user: { login: "github-actions[bot]" } },
+        { id: 3, body: "Another regular comment", user: { login: "github-actions[bot]" } },
+        { id: 4, body: `${PLAN_HEADER}\n\nLatest plan`, user: { login: "github-actions[bot]" } },
       ];
 
       mockOctokit.paginate.mockResolvedValue(comments);
@@ -96,8 +102,12 @@ describe("github", () => {
 
     it("should handle comments with undefined body", async () => {
       const comments = [
-        { id: 1, body: undefined },
-        { id: 2, body: `${PLAN_HEADER}\n\nPlan with content` },
+        { id: 1, body: undefined, user: { login: "github-actions[bot]" } },
+        {
+          id: 2,
+          body: `${PLAN_HEADER}\n\nPlan with content`,
+          user: { login: "github-actions[bot]" },
+        },
       ];
 
       mockOctokit.paginate.mockResolvedValue(comments);
@@ -118,7 +128,13 @@ describe("github", () => {
     });
 
     it("should find plan comment in the middle of body text", async () => {
-      const comments = [{ id: 1, body: `Some text before\n${PLAN_HEADER}\nSome text after` }];
+      const comments = [
+        {
+          id: 1,
+          body: `Some text before\n${PLAN_HEADER}\nSome text after`,
+          user: { login: "github-actions[bot]" },
+        },
+      ];
 
       mockOctokit.paginate.mockResolvedValue(comments);
 
@@ -130,8 +146,12 @@ describe("github", () => {
 
     it("should find plan comment by PLAN_MARKER", async () => {
       const comments = [
-        { id: 1, body: "Regular comment" },
-        { id: 2, body: `${PLAN_MARKER}\n## Plan\n\nContent here` },
+        { id: 1, body: "Regular comment", user: { login: "github-actions[bot]" } },
+        {
+          id: 2,
+          body: `${PLAN_MARKER}\n## Plan\n\nContent here`,
+          user: { login: "github-actions[bot]" },
+        },
       ];
 
       mockOctokit.paginate.mockResolvedValue(comments);
@@ -144,8 +164,12 @@ describe("github", () => {
 
     it("should prefer PLAN_MARKER over PLAN_HEADER when both exist", async () => {
       const comments = [
-        { id: 1, body: `${PLAN_HEADER}\n\nOld style plan` },
-        { id: 2, body: `${PLAN_MARKER}\n## Plan\n\nNew style plan` },
+        { id: 1, body: `${PLAN_HEADER}\n\nOld style plan`, user: { login: "github-actions[bot]" } },
+        {
+          id: 2,
+          body: `${PLAN_MARKER}\n## Plan\n\nNew style plan`,
+          user: { login: "github-actions[bot]" },
+        },
       ];
 
       mockOctokit.paginate.mockResolvedValue(comments);
@@ -158,9 +182,17 @@ describe("github", () => {
 
     it("should return latest PLAN_MARKER comment when multiple exist", async () => {
       const comments = [
-        { id: 1, body: `${PLAN_MARKER}\n## Plan\n\nFirst plan` },
-        { id: 2, body: "Regular comment" },
-        { id: 3, body: `${PLAN_MARKER}\n## Plan\n\nLatest plan` },
+        {
+          id: 1,
+          body: `${PLAN_MARKER}\n## Plan\n\nFirst plan`,
+          user: { login: "github-actions[bot]" },
+        },
+        { id: 2, body: "Regular comment", user: { login: "someone" } },
+        {
+          id: 3,
+          body: `${PLAN_MARKER}\n## Plan\n\nLatest plan`,
+          user: { login: "github-actions[bot]" },
+        },
       ];
 
       mockOctokit.paginate.mockResolvedValue(comments);
@@ -173,8 +205,12 @@ describe("github", () => {
 
     it("should fallback to PLAN_HEADER when no PLAN_MARKER found", async () => {
       const comments = [
-        { id: 1, body: "Regular comment" },
-        { id: 2, body: `${PLAN_HEADER}\n\nPlan without marker` },
+        { id: 1, body: "Regular comment", user: { login: "someone" } },
+        {
+          id: 2,
+          body: `${PLAN_HEADER}\n\nPlan without marker`,
+          user: { login: "github-actions[bot]" },
+        },
       ];
 
       mockOctokit.paginate.mockResolvedValue(comments);
@@ -184,6 +220,28 @@ describe("github", () => {
 
       expect(result).toBe(`${PLAN_HEADER}\n\nPlan without marker`);
     });
+
+    it("should return null when plan comment is from untrusted user", async () => {
+      const comments = [
+        {
+          id: 1,
+          body: `${PLAN_MARKER}\n## Malicious Plan\n\nFake plan from attacker`,
+          user: { login: "malicious-user" },
+        },
+        {
+          id: 2,
+          body: `${PLAN_HEADER}\n\nAnother fake plan`,
+          user: { login: "untrusted-actor" },
+        },
+      ];
+
+      mockOctokit.paginate.mockResolvedValue(comments);
+
+      const client = createGitHubClient({ token: mockToken, owner: mockOwner, repo: mockRepo });
+      const result = await client.findPlanComment(mockIssueNumber);
+
+      expect(result).toBeNull();
+    });
   });
 
   describe("postComment", () => {
@@ -926,6 +984,39 @@ Plan content`;
     });
   });
 
+  describe("createGitHubClient - caching", () => {
+    it("should cache Octokit instances by token", () => {
+      // This test needs isolated mock state
+      vi.clearAllMocks();
+
+      const client1 = createGitHubClient({ token: mockToken, owner: mockOwner, repo: mockRepo });
+      const client2 = createGitHubClient({ token: mockToken, owner: mockOwner, repo: mockRepo });
+
+      // Should only call getOctokit once for the same token
+      expect(github.getOctokit).toHaveBeenCalledWith(mockToken);
+      expect(github.getOctokit).toHaveBeenCalledTimes(1);
+
+      // Both clients should have all expected methods
+      expect(client1).toHaveProperty("findPlanComment");
+      expect(client2).toHaveProperty("findPlanComment");
+    });
+
+    it("should create separate Octokit instances for different tokens", () => {
+      vi.clearAllMocks();
+
+      const token1 = "ghp_token_1";
+      const token2 = "ghp_token_2";
+
+      createGitHubClient({ token: token1, owner: mockOwner, repo: mockRepo });
+      createGitHubClient({ token: token2, owner: mockOwner, repo: mockRepo });
+
+      // Should call getOctokit twice for different tokens
+      expect(github.getOctokit).toHaveBeenCalledWith(token1);
+      expect(github.getOctokit).toHaveBeenCalledWith(token2);
+      expect(github.getOctokit).toHaveBeenCalledTimes(2);
+    });
+  });
+
   describe("createGitHubClient", () => {
     it("should create a client that shares a single Octokit instance", () => {
       const client = createGitHubClient({ token: mockToken, owner: mockOwner, repo: mockRepo });
@@ -948,8 +1039,8 @@ Plan content`;
       const client = createGitHubClient({ token: mockToken, owner: mockOwner, repo: mockRepo });
 
       const comments = [
-        { id: 1, body: "Regular comment" },
-        { id: 2, body: `${PLAN_HEADER}\n\nPlan content` },
+        { id: 1, body: "Regular comment", user: { login: "someone" } },
+        { id: 2, body: `${PLAN_HEADER}\n\nPlan content`, user: { login: "github-actions[bot]" } },
       ];
       mockOctokit.paginate.mockResolvedValue(comments);
 

src/github.ts

@@ -8,6 +8,14 @@ import { SubIssueMetadata, GitHubRepo } from "./types";
 // Other bot identifiers may be added for GitHub App installations
 const TRUSTED_BOT_AUTHORS = new Set(["github-actions[bot]"]);
 
+// Cache for Octokit instances keyed by token
+const octokitCache = new Map<string, ReturnType<typeof github.getOctokit>>();
+
+// For testing: clear the Octokit cache
+export function clearOctokitCache(): void {
+  octokitCache.clear();
+}
+
 export interface LinkSubIssuesResult {
   linked: number;
   failed: number;
@@ -16,7 +24,11 @@ export interface LinkSubIssuesResult {
 export type GitHubClient = ReturnType<typeof createGitHubClient>;
 
 export function createGitHubClient(repo: GitHubRepo) {
-  const octokit = github.getOctokit(repo.token);
+  let octokit = octokitCache.get(repo.token);
+  if (!octokit) {
+    octokit = github.getOctokit(repo.token);
+    octokitCache.set(repo.token, octokit);
+  }
   const { owner, repo: repoName } = repo;
 
   return {
@@ -38,13 +50,6 @@ export function createGitHubClient(repo: GitHubRepo) {
         planComments = trustedComments.filter((comment) => comment.body?.includes(PLAN_HEADER));
       }
 
-      if (planComments.length === 0) {
-        planComments = comments.filter((comment) => comment.body?.includes(PLAN_MARKER));
-      }
-      if (planComments.length === 0) {
-        planComments = comments.filter((comment) => comment.body?.includes(PLAN_HEADER));
-      }
-
       if (planComments.length === 0) {
         return null;
       }

src/main.test.ts

@@ -2,7 +2,7 @@ import { describe, it, expect, vi, beforeEach, afterEach } from "vitest";
 import * as core from "@actions/core";
 import * as fs from "fs";
 import * as os from "os";
-import { run } from "./main";
+import { run, readInputs } from "./main";
 
 vi.mock("@actions/core");
 vi.mock("fs");
@@ -110,7 +110,7 @@ describe("main", () => {
       expect(fs.writeFileSync).toHaveBeenCalledWith(
         expect.stringContaining("leonidas-prompt-"),
         "plan prompt content",
-        "utf-8",
+        { encoding: "utf-8", mode: 0o600 },
       );
     });
 
@@ -166,7 +166,7 @@ describe("main", () => {
       expect(fs.writeFileSync).toHaveBeenCalledWith(
         expect.stringMatching(/^\/runner\/tmp\/leonidas-prompt-\d+\.md$/),
         "prompt content",
-        "utf-8",
+        { encoding: "utf-8", mode: 0o600 },
       );
 
       delete process.env.RUNNER_TEMP;
@@ -391,6 +391,26 @@ describe("main", () => {
     });
   });
 
+  describe("readInputs() - secrets handling", () => {
+    it("should register API key and GitHub token as secrets", () => {
+      vi.mocked(core.getInput).mockImplementation((name: string) => {
+        const inputs: Record<string, string> = {
+          mode: "plan",
+          anthropic_api_key: "test-api-key",
+          github_token: "test-github-token",
+        };
+        return inputs[name] || "";
+      });
+      vi.mocked(core.setSecret).mockImplementation(() => {});
+
+      readInputs();
+
+      expect(core.setSecret).toHaveBeenCalledWith("test-api-key");
+      expect(core.setSecret).toHaveBeenCalledWith("test-github-token");
+      expect(core.setSecret).toHaveBeenCalledTimes(2);
+    });
+  });
+
   describe("run() - error handling", () => {
     it("should catch and report errors via setFailed", async () => {
       vi.mocked(core.getInput).mockImplementation(() => {