Skip to content
Open
Show file tree
Hide file tree
Changes from 2 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
7 changes: 4 additions & 3 deletions src/terminal-manager.ts
Original file line number Diff line number Diff line change
Expand Up @@ -336,9 +336,10 @@ export class TerminalManager {
});
}, timeoutMs);

childProcess.on('exit', (code: any) => {
childProcess.on('close', (code: any) => {
if (childProcess.pid) {
// Store completed session before removing active session
// Store completed session only after stdio closes, so fast-exiting
// processes do not lose stdout/stderr that arrives after exit.
this.completedSessions.set(childProcess.pid, {
pid: childProcess.pid,
outputLines: [...session.outputLines], // Copy line buffer
Expand Down Expand Up @@ -626,4 +627,4 @@ export class TerminalManager {
}
}

export const terminalManager = new TerminalManager();
export const terminalManager = new TerminalManager();
73 changes: 73 additions & 0 deletions test/test-process-output-close.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,73 @@
import assert from 'assert';
import fs from 'fs/promises';
import os from 'os';
import path from 'path';

import { terminalManager } from '../dist/terminal-manager.js';

const TEST_DIR_PREFIX = path.join(os.tmpdir(), 'desktop-commander-process-output-close-');

function quoteForShell(value) {
return `"${value.replace(/"/g, '\\"')}"`;
}

async function setup() {
const testDir = await fs.mkdtemp(TEST_DIR_PREFIX);
const scriptPath = path.join(testDir, 'fast-stderr.mjs');
await fs.writeFile(
scriptPath,
[
"import fs from 'fs';",

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggestion: The temporary script is written with ESM syntax but saved as .js in a temp directory that is outside this package scope, so Node will treat it as CommonJS and fail with a syntax error before running the stderr writes. This makes the regression test fail for the wrong reason. Save it as .mjs or use CommonJS syntax in the generated script. [possible bug]

Severity Level: Major ⚠️
-`test-process-output-close.js` fails due to child SyntaxError.
- ⚠️ Stderr flush regression for fast-exiting processes not validated.
- ⚠️ `npm test` / CI pipeline will report failing test suite.
Steps of Reproduction ✅
1. Run the test suite (e.g. `npm test` which calls `node test/run-all-tests.js` per
`package.json:24,43` and `test/run-all-tests.js:90-99`). The runner discovers
`test-process-output-close.js` via its `files.filter(file => file.startsWith('test') &&
file.endsWith('.js'))` logic at `test/run-all-tests.js:104-108` and spawns it with `node
./test-process-output-close.js` from `test/run-all-tests.js:57-63`.

2. Inside `test/test-process-output-close.js`, the default export `runTests()` at lines
50-62 is executed (because the file is treated as an ES module due to the project
`package.json` having `"type": "module"` at line 10). `runTests()` calls `setup()` at
lines 15-27, which writes `fast-stderr.js` into the OS temp directory (`TEST_DIR` at line
8, `SCRIPT_PATH` at line 9) with the first line of the file being `import fs from 'fs';`
per the string array at lines 20-25.

3. `testFastExitStderrIsFlushed()` at lines 33-48 constructs `command =
\`${quoteForShell(process.execPath)} ${quoteForShell(SCRIPT_PATH)}\`` at line 34 and
passes it to `terminalManager.executeCommand(command, 2000)` at line 35, causing a child
Node process to run `/tmp/.../fast-stderr.js` directly (outside the project tree, so there
is no nearby `package.json` with `"type": "module"`).

4. The child Node process treats `/tmp/.../fast-stderr.js` as a CommonJS `.js` file
(because there is no `package.json` in its ancestor directories declaring `"type":
"module"`), encounters the first line `import fs from 'fs';` (originating from
`test/test-process-output-close.js:20`) as invalid syntax, and exits with a SyntaxError
before executing any of the `fs.writeSync` calls at lines 21-23. As a result, when
`testFastExitStderrIsFlushed()` reads output via
`terminalManager.readOutputPaginated(result.pid, 0, 100)` at line 39 and then asserts that
`text` includes `FAST_STDERR_START` and `FAST_STDERR_END` at lines 44-45, those markers
are missing (only Node's syntax error is present), so the test fails for the wrong reason
and the intended stderr-flush regression is not actually exercised. This behavior is
deterministic whenever the test is run.

Fix in Cursor | Fix in VSCode Claude

(Use Cmd/Ctrl + Click for best experience)

Prompt for AI Agent 🤖
This is a comment left during a code review.

**Path:** test/test-process-output-close.js
**Line:** 20:20
**Comment:**
	*Possible Bug: The temporary script is written with ESM syntax but saved as `.js` in a temp directory that is outside this package scope, so Node will treat it as CommonJS and fail with a syntax error before running the stderr writes. This makes the regression test fail for the wrong reason. Save it as `.mjs` or use CommonJS syntax in the generated script.

Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
👍 | 👎

"fs.writeSync(2, 'FAST_STDERR_START\\n');",
"fs.writeSync(2, 'x'.repeat(256 * 1024));",
"fs.writeSync(2, '\\nFAST_STDERR_END\\n');",
'process.exit(1);',
].join('\n'),
);
return { testDir, scriptPath };
}

async function teardown(testDir) {
await fs.rm(testDir, { recursive: true, force: true });
Comment thread
coderabbitai[bot] marked this conversation as resolved.
}

async function testFastExitStderrIsFlushed(scriptPath) {
const command = `${quoteForShell(process.execPath)} ${quoteForShell(scriptPath)}`;
const result = await terminalManager.executeCommand(command, 2000);

assert.strictEqual(result.isBlocked, false, 'Fast-failing process should be marked complete');

const output = terminalManager.readOutputPaginated(result.pid, 0, 100);
assert(output, 'Completed process output should remain readable');

const text = output.lines.join('\n');
assert.strictEqual(output.exitCode, 1, 'Process exit code should be preserved');
assert(text.includes('FAST_STDERR_START'), 'stderr start marker should be captured');
assert(text.includes('FAST_STDERR_END'), 'stderr end marker should be captured after process exit');

console.log('✓ fast-exiting process stderr is flushed before completion');
}

export default async function runTests() {
let testDir;
try {
const fixture = await setup();
testDir = fixture.testDir;
await testFastExitStderrIsFlushed(fixture.scriptPath);
console.log('\n✅ process output close tests passed!');
return true;
} catch (error) {
console.error('❌ process output close test failed:', error instanceof Error ? error.message : String(error));
return false;
} finally {
if (testDir) {
await teardown(testDir);
}
}
}

if (import.meta.url === `file://${process.argv[1]}`) {
runTests().then((ok) => {
process.exit(ok ? 0 : 1);
});
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Outdated