fix(core): Harden file processors from review feedback

intent(file-processors): address logic/test findings from Fable, Claude, Gemini, and CodeRabbit reviews on PR #1720
decision(windows-exec): keep execFile with a shell + windowsVerbatimArguments rather than switching to exec (Gemini's suggestion) — the issue explicitly requires execFile/spawn over exec for injection safety, and verbatim args fix the cmd.exe quoting bug directly
decision(timeout-kill): SIGKILL the direct shell child on timeout so a SIGTERM-trapping tool cannot hang the pack and pin a semaphore slot; full descendant/process-tree teardown is left as a documented v1 limitation
improve(diagnostics): surface the command's stderr (capped) in the failure message, and distinguish timeout/stdout-overflow from a plain non-zero exit
improve(empty-output): warn when a processor exits 0 but blanks a previously non-empty file (a common footgun), while still accepting empty output
constraint(gate-tests): add the security-critical regression guards — repomixConfigFileSchema strips enableFileProcessors, mergeConfigs only honors it from the CLI config, remoteAction gates it on --remote-trust-config, and the packager threads transformed content into the security scan and metrics
improve(tests): tighten the concurrency assertion (peak > 1, not just > 0) and cover cross-root semaphore sharing, empty-stdout, stderr propagation, real exit/timeout, and multi-{file} substitution

Co-authored-by: Samsen879 <Samsen879@users.noreply.github.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

src/core/file/fileProcessorRun.ts

@@ -87,11 +87,39 @@ export const runProcessorCommand = async (
     encoding: 'utf8',
     // Inherit the parent environment so PATH (needed by npx and friends) is available.
     env: process.env,
+    // On timeout, SIGKILL the shell rather than the default SIGTERM so a tool that
+    // ignores/traps SIGTERM cannot hang the whole pack (and pin a semaphore slot).
+    killSignal: 'SIGKILL',
+    // When we spawn cmd.exe ourselves, Node/libuv would otherwise re-quote each arg
+    // and backslash-escape the double quotes in `resolvedCommand`, which cmd.exe does
+    // not understand. Verbatim mode passes the command line through unchanged. Ignored
+    // on POSIX.
+    windowsVerbatimArguments: isWindows,
   });
 
   return stdout;
 };
 
+/**
+ * Turn a raw exec failure into a human-readable cause, distinguishing a timeout
+ * and a stdout-overflow from an ordinary non-zero exit so the error message is
+ * actionable.
+ */
+const describeProcessorError = (error: unknown, timeout: number): string => {
+  const err = error as (NodeJS.ErrnoException & { killed?: boolean; stderr?: string }) | undefined;
+  if (err?.killed && err.code === 'ERR_CHILD_PROCESS_STDIO_MAXBUFFER') {
+    return `output exceeded the ${FILE_PROCESSOR_MAX_BUFFER}-byte limit`;
+  }
+  // execFile kills the process (default signal, or SIGKILL here) when `timeout` elapses.
+  if (err?.killed) {
+    return `timed out after ${timeout}ms`;
+  }
+  const base = error instanceof Error ? error.message : String(error);
+  // Surface the command's stderr (capped) so a non-zero exit is diagnosable.
+  const stderr = typeof err?.stderr === 'string' ? err.stderr.trim() : '';
+  return stderr ? `${base}\n  Stderr: ${stderr.slice(0, 500)}` : base;
+};
+
 /**
  * Find the first processor whose glob matches the file. Globs are matched the
  * same way as include/ignore (minimatch `{ dot: true }` against the posix path),
@@ -209,34 +237,53 @@ export const applyFileProcessors = async (
 
     await acquireProcessorSlot();
     try {
-      const content = await deps.runProcessorCommand({
-        command: processor.command,
-        content: rawFile.content,
-        tempFilePath,
-        timeout,
-        cwd: rootDir,
-      });
+      let content: string;
+      try {
+        content = await deps.runProcessorCommand({
+          command: processor.command,
+          content: rawFile.content,
+          tempFilePath,
+          timeout,
+          cwd: rootDir,
+        });
+      } catch (error) {
+        const message = describeProcessorError(error, timeout);
+        if (onError === 'skip') {
+          logger.warn(
+            `File processor for "${rawFile.path}" failed, using original content (onError: "skip"): ${message}`,
+          );
+          completed++;
+          progressCallback(
+            `Processing file with command... (${completed}/${matchedCount}) ${pc.dim(rawFile.path)} (skipped)`,
+          );
+          return rawFile;
+        }
+        throw new RepomixError(
+          `File processor failed for "${rawFile.path}".\n` +
+            `  Pattern: ${processor.pattern}\n` +
+            `  Command: ${processor.command}\n` +
+            `  Error: ${message}\n` +
+            `  Set "onError": "skip" on this processor to fall back to the original content instead.`,
+        );
+      }
 
+      // A processor that exits 0 but writes nothing to stdout (e.g. a tool that edits
+      // the temp file in place, or only writes to stderr) would silently blank the
+      // file. Empty output is still accepted, but warn when it replaces non-empty
+      // content so the footgun is visible.
+      if (content === '' && rawFile.content !== '') {
+        logger.warn(
+          `File processor for "${rawFile.path}" produced empty output; the file will be packed as empty. ` +
+            `Check that the command writes the transformed content to stdout.`,
+        );
+      }
+
+      // Progress is reported outside the run try/catch so a throwing progressCallback
+      // is not misread as a processor failure (which, under onError: "skip", would
+      // discard the successfully transformed content).
       completed++;
       progressCallback(`Processing file with command... (${completed}/${matchedCount}) ${pc.dim(rawFile.path)}`);
-
       return { ...rawFile, content };
-    } catch (error) {
-      const message = error instanceof Error ? error.message : String(error);
-      if (onError === 'skip') {
-        logger.warn(
-          `File processor for "${rawFile.path}" failed, using original content (onError: "skip"): ${message}`,
-        );
-        completed++;
-        return rawFile;
-      }
-      throw new RepomixError(
-        `File processor failed for "${rawFile.path}".\n` +
-          `  Pattern: ${processor.pattern}\n` +
-          `  Command: ${processor.command}\n` +
-          `  Error: ${message}\n` +
-          `  Set "onError": "skip" on this processor to fall back to the original content instead.`,
-      );
     } finally {
       releaseProcessorSlot();
     }

tests/cli/actions/remoteAction.test.ts

@@ -294,6 +294,62 @@ describe('remoteAction functions', () => {
       );
     });
 
+    test('should keep file processors disabled for remote runs without --remote-trust-config', async () => {
+      const runDefaultActionMock = vi.fn(async () => createMockDefaultActionResult());
+
+      vi.mocked(fs.copyFile).mockResolvedValue(undefined);
+      // enableFileProcessors: true simulates the real CLI entry point injection; the
+      // remote gate must still force it off because the config comes from a clone.
+      await runRemoteAction(
+        'yamadashy/repomix',
+        { enableFileProcessors: true },
+        {
+          isGitInstalled: vi.fn().mockResolvedValue(false),
+          execGitShallowClone: vi.fn(),
+          getRemoteRefs: async () => Promise.resolve(['main']),
+          runDefaultAction: runDefaultActionMock,
+          downloadGitHubArchive: vi.fn().mockResolvedValue(undefined),
+          isGitHubRepository: vi.fn().mockReturnValue(true),
+          parseGitHubRepoInfo: vi.fn().mockReturnValue({ owner: 'yamadashy', repo: 'repomix' }),
+          isArchiveDownloadSupported: vi.fn().mockReturnValue(true),
+        },
+      );
+
+      expect(runDefaultActionMock).toHaveBeenCalledWith(
+        expect.any(Array),
+        expect.any(String),
+        expect.objectContaining({ enableFileProcessors: false }),
+      );
+    });
+
+    test('should enable file processors for remote runs when --remote-trust-config is passed', async () => {
+      const runDefaultActionMock = vi.fn(async () => createMockDefaultActionResult());
+
+      vi.mocked(fs.copyFile).mockResolvedValue(undefined);
+      await runRemoteAction(
+        'https://gitlab.com/owner/repo.git',
+        { enableFileProcessors: true, remoteTrustConfig: true },
+        {
+          isGitInstalled: async () => Promise.resolve(true),
+          execGitShallowClone: vi.fn(async (_url: string, directory: string) => {
+            await fs.writeFile(path.join(directory, 'README.md'), 'Hello');
+          }),
+          getRemoteRefs: async () => Promise.resolve(['main']),
+          runDefaultAction: runDefaultActionMock,
+          downloadGitHubArchive: vi.fn(),
+          isGitHubRepository: vi.fn().mockReturnValue(false),
+          parseGitHubRepoInfo: vi.fn().mockReturnValue(null),
+          isArchiveDownloadSupported: vi.fn().mockReturnValue(false),
+        },
+      );
+
+      expect(runDefaultActionMock).toHaveBeenCalledWith(
+        expect.any(Array),
+        expect.any(String),
+        expect.objectContaining({ enableFileProcessors: true }),
+      );
+    });
+
     test('should set skipLocalConfig to false when REPOMIX_REMOTE_TRUST_CONFIG env var is true', async () => {
       const originalEnv = process.env.REPOMIX_REMOTE_TRUST_CONFIG;
       process.env.REPOMIX_REMOTE_TRUST_CONFIG = 'true';

tests/config/configLoad.test.ts

@@ -394,6 +394,38 @@ describe('configLoad', () => {
       ]);
     });
 
+    test('should preserve input.processors from the file config', () => {
+      const fileConfig: RepomixConfigFile = {
+        input: {
+          processors: [{ pattern: '**/*.json', command: 'toon {file}', timeout: 30000, onError: 'skip' }],
+        },
+      };
+
+      const merged = mergeConfigs(process.cwd(), fileConfig, {});
+
+      expect(merged.input.processors).toEqual([
+        { pattern: '**/*.json', command: 'toon {file}', timeout: 30000, onError: 'skip' },
+      ]);
+    });
+
+    test('should apply enableFileProcessors from the CLI config', () => {
+      const merged = mergeConfigs(process.cwd(), {}, { enableFileProcessors: true });
+      expect(merged.enableFileProcessors).toBe(true);
+    });
+
+    test('should ignore enableFileProcessors coming from the file config (gate is CLI-only)', () => {
+      // A malicious repo config must not be able to self-authorize command execution.
+      // mergeConfigs only reads the gate from the CLI config, never the file config.
+      const fileConfig = {
+        enableFileProcessors: true,
+        input: { processors: [{ pattern: '**/*', command: 'evil {file}' }] },
+      } as unknown as RepomixConfigFile;
+
+      const merged = mergeConfigs(process.cwd(), fileConfig, {});
+
+      expect(merged.enableFileProcessors).toBeUndefined();
+    });
+
     test('should not mutate defaultConfig', () => {
       const originalFilePath = defaultConfig.output.filePath;
       const fileConfig: RepomixConfigFile = {

tests/config/configSchema.test.ts

@@ -381,6 +381,27 @@ describe('configSchema', () => {
       };
       expect(v.parse(repomixConfigFileSchema, partialConfig)).toEqual(partialConfig);
     });
+
+    it('should accept input.processors', () => {
+      const config = {
+        input: {
+          processors: [{ pattern: '**/*.json', command: 'toon {file}', timeout: 30000, onError: 'skip' }],
+        },
+      };
+      expect(v.parse(repomixConfigFileSchema, config)).toEqual(config);
+    });
+
+    it('should strip enableFileProcessors so a config file cannot open the gate', () => {
+      // The gate is a CLI-only field; a file config must never be able to enable
+      // arbitrary command execution for MCP/website/library callers.
+      const config = {
+        enableFileProcessors: true,
+        input: { processors: [{ pattern: '**/*', command: 'evil {file}' }] },
+      };
+      const parsed = v.parse(repomixConfigFileSchema, config) as Record<string, unknown>;
+      expect(parsed.enableFileProcessors).toBeUndefined();
+      expect(parsed.input).toEqual(config.input);
+    });
   });
 
   describe('repomixConfigCliSchema', () => {

tests/core/file/fileProcessorRun.test.ts

@@ -44,6 +44,23 @@ describe('fileProcessorRun', () => {
       expect(runProcessorCommandMock).not.toHaveBeenCalled();
     });
 
+    it('returns files unchanged when the gate key is absent (library/MCP default)', async () => {
+      // createMockConfig omits enableFileProcessors entirely — the real default for
+      // library pack()/runCli() callers and MCP, which never inject the gate.
+      const config = createMockConfig({
+        input: { processors: [{ pattern: '**/*.json', command: 'cat {file}' }] },
+      });
+      expect(config.enableFileProcessors).toBeUndefined();
+      const runProcessorCommandMock = vi.fn();
+
+      const result = await applyFileProcessors(rawFiles, '/root', config, () => {}, {
+        runProcessorCommand: runProcessorCommandMock,
+      });
+
+      expect(result).toBe(rawFiles);
+      expect(runProcessorCommandMock).not.toHaveBeenCalled();
+    });
+
     it('returns files unchanged when enabled but no processors configured', async () => {
       const config = createMockConfig({ enableFileProcessors: true });
       const runProcessorCommandMock = vi.fn();
@@ -112,6 +129,8 @@ describe('fileProcessorRun', () => {
       });
 
       expect(runProcessorCommandMock).toHaveBeenCalledWith(expect.objectContaining({ command: 'first {file}' }));
+      // Exactly one processor runs per file — no chaining.
+      expect(runProcessorCommandMock).toHaveBeenCalledTimes(1);
     });
 
     it('passes a unique temp file path preserving the extension', async () => {
@@ -246,10 +265,18 @@ describe('fileProcessorRun', () => {
         () => {},
         { runProcessorCommand: runProcessorCommandMock },
       );
+      // Track whether the overall promise rejects before we release "b". A regression
+      // to fail-fast (Promise.all + immediate cleanup) would settle it here — the whole
+      // point this test guards against.
+      let rejectedEarly = false;
+      resultPromise.catch(() => {
+        rejectedEarly = true;
+      });
 
-      // Let the microtask queue flush so "a" has rejected; the overall promise
+      // Let the microtask/timer queue flush so "a" has rejected; the overall promise
       // must still be pending because "b" has not settled yet.
       await new Promise((resolve) => setTimeout(resolve, 10));
+      expect(rejectedEarly).toBe(false);
       expect(bSettled.done).toBe(false);
 
       releaseB('transformed-b');
@@ -277,12 +304,78 @@ describe('fileProcessorRun', () => {
       const files = Array.from({ length: 40 }, (_, i) => ({ path: `f${i}.json`, content: '{}' }));
       await applyFileProcessors(files, '/root', config, () => {}, { runProcessorCommand: runProcessorCommandMock });
 
-      // The module-level semaphore caps concurrency at min(8, cpus); it must never
-      // exceed the hard ceiling of 8 no matter how many files match.
-      expect(peak).toBeGreaterThan(0);
+      // The module-level semaphore caps concurrency at min(8, cpus); it must run
+      // several in parallel (not serialize) but never exceed the hard ceiling of 8.
+      expect(peak).toBeGreaterThan(1);
       expect(peak).toBeLessThanOrEqual(8);
       expect(runProcessorCommandMock).toHaveBeenCalledTimes(40);
     });
+
+    it('shares the concurrency cap across concurrent per-root calls', async () => {
+      // The module-level semaphore must bound the combined concurrency of two
+      // applyFileProcessors calls (the multi-root case), not 8-per-call.
+      const config = createMockConfig({
+        enableFileProcessors: true,
+        input: { processors: [{ pattern: '**/*.json', command: 'toon {file}' }] },
+      });
+
+      let active = 0;
+      let peak = 0;
+      const runProcessorCommandMock = vi.fn(async () => {
+        active++;
+        peak = Math.max(peak, active);
+        await new Promise((resolve) => setTimeout(resolve, 5));
+        active--;
+        return 'out';
+      });
+
+      const makeFiles = (prefix: string) =>
+        Array.from({ length: 20 }, (_, i) => ({ path: `${prefix}/f${i}.json`, content: '{}' }));
+
+      await Promise.all([
+        applyFileProcessors(makeFiles('a'), '/root-a', config, () => {}, {
+          runProcessorCommand: runProcessorCommandMock,
+        }),
+        applyFileProcessors(makeFiles('b'), '/root-b', config, () => {}, {
+          runProcessorCommand: runProcessorCommandMock,
+        }),
+      ]);
+
+      expect(peak).toBeGreaterThan(1);
+      expect(peak).toBeLessThanOrEqual(8);
+      expect(runProcessorCommandMock).toHaveBeenCalledTimes(40);
+    });
+
+    it('accepts empty stdout as valid content but warns when it blanks a non-empty file', async () => {
+      const config = createMockConfig({
+        enableFileProcessors: true,
+        input: { processors: [{ pattern: '**/*.json', command: 'toon {file}' }] },
+      });
+      const runProcessorCommandMock = vi.fn().mockResolvedValue('');
+
+      const result = await applyFileProcessors([{ path: 'a.json', content: '{"a":1}' }], '/root', config, () => {}, {
+        runProcessorCommand: runProcessorCommandMock,
+      });
+
+      expect(result).toEqual([{ path: 'a.json', content: '' }]);
+      expect(logger.warn).toHaveBeenCalledWith(expect.stringContaining('produced empty output'));
+    });
+
+    it('includes the command stderr in the failure message', async () => {
+      const config = createMockConfig({
+        enableFileProcessors: true,
+        input: { processors: [{ pattern: '**/*.json', command: 'toon {file}' }] },
+      });
+      const runProcessorCommandMock = vi
+        .fn()
+        .mockRejectedValue(Object.assign(new Error('Command failed'), { stderr: 'boom detail from tool' }));
+
+      await expect(
+        applyFileProcessors([{ path: 'a.json', content: '{}' }], '/root', config, () => {}, {
+          runProcessorCommand: runProcessorCommandMock,
+        }),
+      ).rejects.toThrow(/boom detail from tool/);
+    });
   });
 
   describe('runProcessorCommand', () => {
@@ -319,28 +412,87 @@ describe('fileProcessorRun', () => {
       }
     });
 
-    it('runs a real command end-to-end (cat echoes the temp file back)', async () => {
-      // Skip on Windows where `cat` is not guaranteed to exist.
-      if (process.platform === 'win32') {
-        return;
-      }
+    it('substitutes every {file} occurrence', async () => {
       const tempDir = await fs.mkdtemp(path.join(os.tmpdir(), 'repomix-proc-test-'));
-      const tempFilePath = path.join(tempDir, '0-a.txt');
+      const tempFilePath = path.join(tempDir, '0-a.json');
+      const execFileAsyncMock = vi.fn().mockResolvedValue({ stdout: 'out', stderr: '' });
+      const deps = { execFileAsync: execFileAsyncMock } as unknown as Parameters<typeof runProcessorCommand>[1];
 
       try {
-        const result = await runProcessorCommand({
-          command: 'cat {file}',
-          content: 'hello world',
-          tempFilePath,
-          timeout: 10000,
-          cwd: tempDir,
-        });
-
-        expect(result).toBe('hello world');
+        await runProcessorCommand(
+          { command: 'diff {file} {file}', content: 'x', tempFilePath, timeout: 5000, cwd: tempDir },
+          deps,
+        );
+        const shellArgs = execFileAsyncMock.mock.calls[0][1] as string[];
+        const commandArg = shellArgs[shellArgs.length - 1];
+        expect(commandArg).not.toContain('{file}');
+        // Both placeholders replaced with the temp path.
+        expect(commandArg.match(/0-a\.json/g)).toHaveLength(2);
       } finally {
         await fs.rm(tempDir, { recursive: true, force: true });
       }
     });
+
+    // Real-shell tests exercise the execFile wiring the mocked tests can't (exit codes,
+    // timeouts, and shell quoting). Skipped on Windows where the POSIX tools differ.
+    const describePosix = process.platform === 'win32' ? describe.skip : describe;
+    describePosix('real shell', () => {
+      it('echoes the temp file back (cat), even when the temp dir path contains a space', async () => {
+        const tempDir = await fs.mkdtemp(path.join(os.tmpdir(), 'repomix-proc test-'));
+        const tempFilePath = path.join(tempDir, '0-a.txt');
+
+        try {
+          const result = await runProcessorCommand({
+            command: 'cat {file}',
+            content: 'hello world',
+            tempFilePath,
+            timeout: 10000,
+            cwd: tempDir,
+          });
+          expect(result).toBe('hello world');
+        } finally {
+          await fs.rm(tempDir, { recursive: true, force: true });
+        }
+      });
+
+      it('rejects when the real command exits non-zero', async () => {
+        const tempDir = await fs.mkdtemp(path.join(os.tmpdir(), 'repomix-proc-test-'));
+        const tempFilePath = path.join(tempDir, '0-a.txt');
+
+        try {
+          await expect(
+            runProcessorCommand({
+              command: 'cat {file} && exit 3',
+              content: 'x',
+              tempFilePath,
+              timeout: 10000,
+              cwd: tempDir,
+            }),
+          ).rejects.toThrow();
+        } finally {
+          await fs.rm(tempDir, { recursive: true, force: true });
+        }
+      });
+
+      it('rejects (killed) when the real command exceeds its timeout', async () => {
+        const tempDir = await fs.mkdtemp(path.join(os.tmpdir(), 'repomix-proc-test-'));
+        const tempFilePath = path.join(tempDir, '0-a.txt');
+
+        try {
+          await expect(
+            runProcessorCommand({
+              command: 'sleep 5 # {file}',
+              content: 'x',
+              tempFilePath,
+              timeout: 100,
+              cwd: tempDir,
+            }),
+          ).rejects.toMatchObject({ killed: true });
+        } finally {
+          await fs.rm(tempDir, { recursive: true, force: true });
+        }
+      });
+    });
   });
 
   describe('logFileProcessorStatus', () => {

tests/core/packager.test.ts

@@ -141,6 +141,65 @@ describe('packager', () => {
     expect(result.skippedFiles).toEqual([]);
   });
 
+  test('applies file processors per root and passes the transformed content to the security check and processing', async () => {
+    const collectedRawFiles = [{ path: 'a.json', content: 'raw content' }];
+    const transformedRawFiles = [{ path: 'a.json', content: 'transformed content' }];
+    const mockFilePaths = ['a.json'];
+
+    const applyFileProcessors = vi.fn().mockResolvedValue(transformedRawFiles);
+
+    const mockDeps = {
+      searchFiles: vi.fn().mockResolvedValue({ filePaths: mockFilePaths, emptyDirPaths: [] }),
+      sortPaths: vi.fn().mockImplementation((paths) => paths),
+      collectFiles: vi.fn().mockResolvedValue({ rawFiles: collectedRawFiles, skippedFiles: [] }),
+      applyFileProcessors,
+      processFiles: vi.fn().mockReturnValue([{ path: 'a.json', content: 'transformed content' }]),
+      validateFileSafety: vi.fn().mockResolvedValue({
+        safeFilePaths: mockFilePaths,
+        safeRawFiles: transformedRawFiles,
+        suspiciousFilesResults: [],
+        suspiciousGitDiffResults: [],
+        suspiciousGitLogResults: [],
+      }),
+      produceOutput: vi.fn().mockResolvedValue({ outputForMetrics: 'out' }),
+      createMetricsTaskRunner: vi.fn().mockReturnValue({
+        taskRunner: { run: vi.fn().mockResolvedValue(0), cleanup: vi.fn().mockResolvedValue(undefined) },
+        warmupPromise: Promise.resolve(),
+      }),
+      calculateMetrics: vi.fn().mockResolvedValue({
+        totalFiles: 1,
+        totalCharacters: 0,
+        totalTokens: 0,
+        fileCharCounts: {},
+        fileTokenCounts: {},
+        gitDiffTokenCount: 0,
+        gitLogTokenCount: 0,
+      }),
+    };
+
+    const config = createMockConfig();
+    const progressCallback = vi.fn();
+    await pack(['root'], config, progressCallback, mockDeps);
+
+    // Processors run per root, on the collected per-root-relative files (before path rewrite).
+    expect(applyFileProcessors).toHaveBeenCalledWith(collectedRawFiles, 'root', config, progressCallback);
+
+    // The security scan and file processing must operate on the TRANSFORMED content
+    // (what actually ships), not the original collected content.
+    expect(mockDeps.validateFileSafety).toHaveBeenCalledWith(
+      [expect.objectContaining({ path: 'a.json', content: 'transformed content' })],
+      progressCallback,
+      config,
+      undefined,
+      undefined,
+    );
+    expect(mockDeps.processFiles).toHaveBeenCalledWith(
+      [expect.objectContaining({ path: 'a.json', content: 'transformed content' })],
+      config,
+      progressCallback,
+    );
+  });
+
   describe('parallel error handling', () => {
     // The pipeline runs several stages in parallel (security check + file processing,
     // output generation + metrics). Regressions in error propagation or worker cleanup