Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0

### Fixed

- **Windows jscpd output bug (#270)** - Fixed `runDuplicateDetection` creating a mangled filename on Windows when `--output NUL` was passed to jscpd via `execFileSync` (no shell). Replaced platform-specific null device with a temp directory via `os.tmpdir()` that is cleaned up in a `finally` block. Added 5 regression tests for temp directory lifecycle.

- **task-discoverer**: Exclude issues that already have an open PR from discovery results (GitHub source only). Detection uses branch name suffix, PR body closing keywords (`closes/fixes/resolves #N`), and PR title `(#N)` convention. Fixes #236.

- **`/debate` 240s timeout enforcement** — All tool invocations in the debate workflow now enforce a hard 240-second timeout. Round 1 proposer timeouts abort the debate; round 1 challenger timeouts proceed with an uncontested position; round 2+ timeouts synthesize from completed rounds. Added "all rounds timeout" error path (`[ERROR] Debate failed: all tool invocations timed out.`). Timeout handling is consistent across the Claude Code command, OpenCode adapter, Codex adapter, and the `debate-orchestrator` agent. Restored missing "Round 2+: Challenger Follow-up" template in the OpenCode adapter SKILL.md. Fixes issue #233.
Expand Down
154 changes: 154 additions & 0 deletions __tests__/cli-enhancers.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -511,6 +511,160 @@ describe('cli-enhancers', () => {
const result = runDuplicateDetection('/nonexistent/path', { minLines: 3 });
expect(result === null || Array.isArray(result)).toBe(true);
});

describe('temp directory for jscpd output (Windows NUL fix)', () => {
// The module destructures execFileSync from child_process at require time,
// so we must use jest.isolateModules to re-require with mocked child_process.
function loadWithMocks(execFileSyncMock) {
let mod;
jest.isolateModules(() => {
jest.doMock('child_process', () => ({
...jest.requireActual('child_process'),
execFileSync: execFileSyncMock,
execSync: jest.requireActual('child_process').execSync
}));
mod = require('../lib/patterns/cli-enhancers');
});
return mod;
}

it('should use temp directory for jscpd output instead of NUL', () => {
const mkdtempSpy = jest.spyOn(fs, 'mkdtempSync');
const rmSpy = jest.spyOn(fs, 'rmSync').mockImplementation(() => {});
Copy link

Copilot AI Feb 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These tests spy on fs.rmSync but also mock it to a no-op. Since runDuplicateDetection() uses a real fs.mkdtempSync(), this can leave real temp directories behind on disk (cleanup runs but is stubbed). Prefer spying without overriding implementation (call-through), or ensure the created temp dir is actually removed in test teardown.

Suggested change
const rmSpy = jest.spyOn(fs, 'rmSync').mockImplementation(() => {});
const rmSpy = jest.spyOn(fs, 'rmSync');

Copilot uses AI. Check for mistakes.

const mockExecFileSync = jest.fn((cmd, args) => {
if (cmd === 'jscpd' && args && args.includes('--version')) {
return 'jscpd v1.0.0';
}
if (cmd === 'jscpd' && args && args.includes('--reporters')) {
return JSON.stringify({ duplicates: [] });
}
return '';
});

const mod = loadWithMocks(mockExecFileSync);
mod.runDuplicateDetection('/some/repo');

// Verify mkdtempSync was called with os.tmpdir() prefix
expect(mkdtempSpy).toHaveBeenCalled();
const mkdtempArg = mkdtempSpy.mock.calls[0][0];
expect(mkdtempArg.startsWith(os.tmpdir())).toBe(true);

// Verify --output arg uses temp dir, not NUL or /dev/null
const jscpdCall = mockExecFileSync.mock.calls.find(
call => call[0] === 'jscpd' && call[1] && call[1].includes('--output')
);
expect(jscpdCall).toBeDefined();
const outputIndex = jscpdCall[1].indexOf('--output');
const outputValue = jscpdCall[1][outputIndex + 1];
expect(outputValue).not.toBe('NUL');
expect(outputValue).not.toBe('/dev/null');

mkdtempSpy.mockRestore();
rmSpy.mockRestore();
});

it('should clean up temp directory after successful execution', () => {
const rmSpy = jest.spyOn(fs, 'rmSync').mockImplementation(() => {});

const mockExecFileSync = jest.fn((cmd, args) => {
if (cmd === 'jscpd' && args && args.includes('--version')) {
return 'jscpd v1.0.0';
}
if (cmd === 'jscpd' && args && args.includes('--reporters')) {
return JSON.stringify({ duplicates: [] });
}
return '';
});

const mod = loadWithMocks(mockExecFileSync);
mod.runDuplicateDetection('/some/repo');

// Verify rmSync was called with recursive and force options
const rmCalls = rmSpy.mock.calls.filter(
call => call[1] && call[1].recursive && call[1].force
);
expect(rmCalls.length).toBeGreaterThan(0);
// The path should be in the temp directory
expect(rmCalls[0][0].startsWith(os.tmpdir())).toBe(true);

rmSpy.mockRestore();
});

it('should clean up temp directory even on jscpd failure', () => {
const rmSpy = jest.spyOn(fs, 'rmSync').mockImplementation(() => {});

const mockExecFileSync = jest.fn((cmd, args) => {
if (cmd === 'jscpd' && args && args.includes('--version')) {
return 'jscpd v1.0.0';
}
if (cmd === 'jscpd' && args && args.includes('--reporters')) {
throw new Error('jscpd execution failed');
}
return '';
});

const mod = loadWithMocks(mockExecFileSync);
const result = mod.runDuplicateDetection('/some/repo');

// Should return null on failure
expect(result).toBeNull();
// Should still clean up temp dir via finally block
const rmCalls = rmSpy.mock.calls.filter(
call => call[1] && call[1].recursive && call[1].force
);
expect(rmCalls.length).toBeGreaterThan(0);

rmSpy.mockRestore();
});

it('should return null if temp directory creation fails', () => {
const mkdtempSpy = jest.spyOn(fs, 'mkdtempSync').mockImplementation(() => {
throw new Error('ENOSPC: no space left on device');
});

const mockExecFileSync = jest.fn((cmd, args) => {
if (cmd === 'jscpd' && args && args.includes('--version')) {
return 'jscpd v1.0.0';
}
return '';
});

const mod = loadWithMocks(mockExecFileSync);
const result = mod.runDuplicateDetection('/some/repo');

expect(result).toBeNull();
mkdtempSpy.mockRestore();
});

it('should not reference NUL or /dev/null in jscpd args', () => {
jest.spyOn(fs, 'rmSync').mockImplementation(() => {});

const mockExecFileSync = jest.fn((cmd, args) => {
if (cmd === 'jscpd' && args && args.includes('--version')) {
return 'jscpd v1.0.0';
}
if (cmd === 'jscpd' && args && args.includes('--reporters')) {
return JSON.stringify({ duplicates: [] });
}
return '';
});

const mod = loadWithMocks(mockExecFileSync);
mod.runDuplicateDetection('/some/repo');

// Verify no args contain NUL or /dev/null as the output path
for (const call of mockExecFileSync.mock.calls) {
const args = call[1] || [];
const outputIdx = args.indexOf('--output');
if (outputIdx !== -1) {
const outputVal = args[outputIdx + 1];
expect(outputVal.toLowerCase()).not.toBe('nul');
expect(outputVal).not.toBe('/dev/null');
}
}
});
});
});

describe('runDependencyAnalysis', () => {
Expand Down
13 changes: 11 additions & 2 deletions lib/patterns/cli-enhancers.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
*/

const { execSync, execFileSync } = require('child_process');
const os = require('os');
const path = require('path');
const fs = require('fs');
// Note: escapeDoubleQuotes no longer needed - using execFileSync with arg arrays
Expand Down Expand Up @@ -345,16 +346,20 @@ function runDuplicateDetection(repoPath, options = {}) {
const minLines = options.minLines || 5;
const minTokens = options.minTokens || 50;

// Use temp directory for jscpd output to avoid platform-specific null device issues.
// On Windows, NUL passed via execFileSync (no shell) is treated as a literal filename,
// creating a mangled path in the working directory.
Comment on lines +350 to +351
Copy link

Copilot AI Feb 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The explanatory comment is a bit inaccurate: execFileSync doesn’t treat NUL as a filename—jscpd interprets the --output value and ends up creating the mangled path on Windows. Consider rewording to attribute the behavior to jscpd and clarify that NUL//dev/null aren’t valid output paths for jscpd in this mode.

Suggested change
// On Windows, NUL passed via execFileSync (no shell) is treated as a literal filename,
// creating a mangled path in the working directory.
// jscpd interprets the value passed to --output itself, so using NUL or /dev/null here
// results in jscpd creating a mangled path on Windows rather than discarding the output.

Copilot uses AI. Check for mistakes.
let tempOutputDir;
try {
tempOutputDir = fs.mkdtempSync(path.join(os.tmpdir(), 'jscpd-'));
// Run jscpd with JSON output
// Use execFileSync with arg array to prevent command injection (no shell interpretation)
const outputPath = process.platform === 'win32' ? 'NUL' : '/dev/null';
const args = [
repoPath,
'--min-lines', String(minLines),
'--min-tokens', String(minTokens),
'--reporters', 'json',
'--output', outputPath,
'--output', tempOutputDir,
'--silent'
];

Expand Down Expand Up @@ -393,6 +398,10 @@ function runDuplicateDetection(repoPath, options = {}) {
} catch {
// Tool execution failed
return null;
} finally {
if (tempOutputDir) {
try { fs.rmSync(tempOutputDir, { recursive: true, force: true }); } catch {}
}
}
}

Expand Down