fix: address PR review comments for isomorphic-git migration

- Fix conditional execution check using fileURLToPath to properly compare file:// URL with file path
- Make loadConfig parameter handling consistent by using nullable configPath
- Separate filesystem concerns: add pluginFs parameter for reading plugin files from real filesystem
- Update tests to use realFs for plugin files instead of trying to create them in memfs
- Remove test setup code that attempted to create plugin files in memfs
- Rename test suite from "CLI integration (help commands)" to "module exports"
- Add .gitignore to exclude node_modules and package-lock.json

Co-authored-by: retsohuang <785920+retsohuang@users.noreply.github.com>

Author: Copilot

plugins/code-review-tools/scripts/.gitignore

@@ -0,0 +1,2 @@
+node_modules/
+package-lock.json

plugins/code-review-tools/scripts/src/cli.ts

@@ -7,6 +7,7 @@ import {
   type PromiseFsClient,
 } from "isomorphic-git"
 import * as nodeFs from "node:fs"
+import { fileURLToPath } from "node:url"
 import { join } from "node:path"
 import {
   DEFAULT_CONFIG,
@@ -52,15 +53,16 @@ function error(message: string): ErrorOutput {
 }
 
 function loadConfig(
-  configPath: string = ".claude/code-review-tools/config.json",
+  configPath: string | undefined = undefined,
   fs: FsLike = defaultFs,
 ): Output<ReviewConfig> {
+  const actualConfigPath = configPath ?? ".claude/code-review-tools/config.json"
   try {
-    if (!fs.existsSync(configPath)) {
+    if (!fs.existsSync(actualConfigPath)) {
       return success(DEFAULT_CONFIG)
     }
 
-    const userConfig = JSON.parse(fs.readFileSync(configPath, "utf-8"))
+    const userConfig = JSON.parse(fs.readFileSync(actualConfigPath, "utf-8"))
     const config = parseConfig(userConfig)
 
     return success(config)
@@ -150,6 +152,7 @@ function buildRules(
   config: ReviewConfig,
   pluginRoot: string,
   fs: FsLike = defaultFs,
+  pluginFs: FsLike = defaultFs,
 ): Output<BuildRulesResult> {
   try {
     const rulesSections: string[] = []
@@ -176,8 +179,8 @@ function buildRules(
     for (const rule of builtInRules) {
       if (config.builtInRules?.[rule.key]) {
         const rulePath = join(pluginRoot, "rules", rule.file)
-        if (fs.existsSync(rulePath)) {
-          const content = fs.readFileSync(rulePath, "utf-8")
+        if (pluginFs.existsSync(rulePath)) {
+          const content = pluginFs.readFileSync(rulePath, "utf-8")
           rulesSections.push(`# ${rule.name} Rules\n\n${content}\n\n---\n`)
           enabledCount++
         }
@@ -228,23 +231,27 @@ function loadTemplate(
   pluginRoot: string,
   defaultTemplateName: string,
   fs: FsLike = defaultFs,
+  pluginFs: FsLike = defaultFs,
 ): string {
   let templatePath: string
+  let usePluginFs = false
 
   if (customTemplate) {
     templatePath = `.claude/code-review-tools/templates/${customTemplate}`
   } else {
     templatePath = `${pluginRoot}/templates/${defaultTemplateName}`
+    usePluginFs = true
   }
 
-  if (fs.existsSync(templatePath)) {
-    return fs.readFileSync(templatePath, "utf-8")
+  const fsToUse = usePluginFs ? pluginFs : fs
+  if (fsToUse.existsSync(templatePath)) {
+    return fsToUse.readFileSync(templatePath, "utf-8")
   }
 
   const defaultPath = `${pluginRoot}/templates/${defaultTemplateName}`
-  if (fs.existsSync(defaultPath)) {
+  if (pluginFs.existsSync(defaultPath)) {
     console.error(`WARNING: Template not found: ${templatePath}, using default`)
-    return fs.readFileSync(defaultPath, "utf-8")
+    return pluginFs.readFileSync(defaultPath, "utf-8")
   }
 
   throw new Error(`Template not found: ${defaultPath}`)
@@ -255,6 +262,7 @@ async function prepareReview(
   pluginRoot: string,
   dir: string = DEFAULT_DIR,
   fs: FsLike = defaultFs,
+  pluginFs: FsLike = defaultFs,
 ): Promise<Output<PrepareReviewResult>> {
   try {
     const configResult = loadConfig(undefined, fs)
@@ -268,7 +276,7 @@ async function prepareReview(
       return error(`Failed to collect commits: ${commitsResult.error}`)
     }
 
-    const rulesResult = buildRules(config, pluginRoot, fs)
+    const rulesResult = buildRules(config, pluginRoot, fs, pluginFs)
     if (!rulesResult.success) {
       return error(`Failed to build rules: ${rulesResult.error}`)
     }
@@ -278,12 +286,14 @@ async function prepareReview(
       pluginRoot,
       "report-template.md",
       fs,
+      pluginFs,
     )
     const summaryTemplate = loadTemplate(
       config.reports?.summaryTemplate,
       pluginRoot,
       "summary-template.md",
       fs,
+      pluginFs,
     )
 
     return success({
@@ -405,7 +415,7 @@ async function main(): Promise<void> {
   }
 }
 
-if (import.meta.url.endsWith(process.argv[1])) {
+if (fileURLToPath(import.meta.url) === process.argv[1]) {
   main().catch((err) => {
     console.error("Fatal error:", (err as Error).message)
     process.exit(1)

plugins/code-review-tools/scripts/tests/cli.test.ts

@@ -1,6 +1,7 @@
 import { afterEach, beforeEach, describe, expect, test } from "bun:test"
 import { add, commit, init, setConfig } from "isomorphic-git"
 import { Volume, createFsFromVolume } from "memfs"
+import * as nodeFs from "node:fs"
 import { join } from "node:path"
 import {
   collectCommits,
@@ -11,6 +12,15 @@ import {
 
 const PLUGIN_ROOT = join(__dirname, "../../")
 
+// Real filesystem for reading plugin files
+const realFs: FsLike = {
+  promises: nodeFs.promises,
+  existsSync: nodeFs.existsSync,
+  readFileSync: nodeFs.readFileSync,
+  writeFileSync: nodeFs.writeFileSync,
+  mkdirSync: nodeFs.mkdirSync,
+} as FsLike
+
 let fs: FsLike
 let vol: Volume
 let testCommits: string[] = []
@@ -67,34 +77,6 @@ beforeEach(async () => {
   })
 
   testCommits = [commit3, commit2, commit1]
-
-  const pluginRulesDir = join(PLUGIN_ROOT, "rules")
-  const pluginTemplatesDir = join(PLUGIN_ROOT, "templates")
-
-  fs.mkdirSync(pluginRulesDir, { recursive: true })
-  fs.mkdirSync(pluginTemplatesDir, { recursive: true })
-
-  fs.writeFileSync(
-    join(pluginRulesDir, "component-extraction-rules.md"),
-    "# Component Extraction Rules\nVerify component extraction.",
-  )
-  fs.writeFileSync(
-    join(pluginRulesDir, "component-reuse-rules.md"),
-    "# Component Reuse Rules\nCheck for reuse opportunities.",
-  )
-  fs.writeFileSync(
-    join(pluginRulesDir, "ai-slop-rules.md"),
-    "# AI Slop Rules\nDetect AI patterns.",
-  )
-
-  fs.writeFileSync(
-    join(pluginTemplatesDir, "report-template.md"),
-    "# Report Template\n{commitRange}",
-  )
-  fs.writeFileSync(
-    join(pluginTemplatesDir, "summary-template.md"),
-    "# Summary Template\n{commits}",
-  )
 })
 
 afterEach(() => {
@@ -223,7 +205,13 @@ describe("prepareReview", () => {
   test("prepares review data successfully with default config", async () => {
     const startCommit = testCommits[1].slice(0, 7)
 
-    const result = await prepareReview(startCommit, PLUGIN_ROOT, TEST_DIR, fs)
+    const result = await prepareReview(
+      startCommit,
+      PLUGIN_ROOT,
+      TEST_DIR,
+      fs,
+      realFs,
+    )
 
     expect(result.success).toBe(true)
     if (result.success) {
@@ -267,7 +255,13 @@ describe("prepareReview", () => {
 
     const startCommit = testCommits[1].slice(0, 7)
 
-    const result = await prepareReview(startCommit, PLUGIN_ROOT, TEST_DIR, fs)
+    const result = await prepareReview(
+      startCommit,
+      PLUGIN_ROOT,
+      TEST_DIR,
+      fs,
+      realFs,
+    )
 
     expect(result.success).toBe(true)
     if (result.success) {
@@ -282,7 +276,13 @@ describe("prepareReview", () => {
   test("returns correct commit list", async () => {
     const startCommit = testCommits[2].slice(0, 7)
 
-    const result = await prepareReview(startCommit, PLUGIN_ROOT, TEST_DIR, fs)
+    const result = await prepareReview(
+      startCommit,
+      PLUGIN_ROOT,
+      TEST_DIR,
+      fs,
+      realFs,
+    )
 
     expect(result.success).toBe(true)
     if (result.success) {
@@ -292,7 +292,13 @@ describe("prepareReview", () => {
   })
 
   test("returns error for invalid commit", async () => {
-    const result = await prepareReview("invalid", PLUGIN_ROOT, TEST_DIR, fs)
+    const result = await prepareReview(
+      "invalid",
+      PLUGIN_ROOT,
+      TEST_DIR,
+      fs,
+      realFs,
+    )
 
     expect(result.success).toBe(false)
     if (!result.success) {
@@ -301,7 +307,7 @@ describe("prepareReview", () => {
   })
 })
 
-describe("CLI integration (help commands)", () => {
+describe("module exports", () => {
   test("CLI exports are available", () => {
     expect(prepareReview).toBeDefined()
     expect(collectCommits).toBeDefined()