Fix for reding finished process outputs #210
Conversation
…ut after it finished stopped working. Added tests and fixed issue
WalkthroughAdded logic in readProcessOutput to return final output from completed sessions via terminalManager.getNewOutput(pid) when no active session exists; updated error message accordingly. Introduced end-to-end tests verifying reading output from completed and immediately finishing processes. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant ProcessTools as readProcessOutput
participant TerminalMgr as terminalManager
Caller->>ProcessTools: readProcessOutput(pid, timeout)
ProcessTools->>TerminalMgr: getActiveSession(pid)
alt Active session exists
TerminalMgr-->>ProcessTools: session
ProcessTools-->>Caller: stream/output per active session
else No active session
ProcessTools->>TerminalMgr: getNewOutput(pid)
alt Completed output exists
TerminalMgr-->>ProcessTools: completedOutput
ProcessTools-->>Caller: completedOutput (isError=false)
else No completed output
ProcessTools-->>Caller: error "No session found for PID X"
end
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
src/tools/improved-process-tools.ts (1)
126-127: Unify error wording across endpointsThis endpoint now returns "No session found for PID X" while forceTerminate still says "No active session found for PID X". Consider using a consistent message across tools to avoid confusion.
As a follow-up, you can update forceTerminate to return "No session found for PID ..." when appropriate (outside this change scope). Do you want me to propose a patch for that file as well?
test/test-read-completed-process-proper.js (3)
20-24: Assert start_process succeeded before parsing PIDIf startProcess failed, parsing PID from the message can throw or mislead.
Apply this diff:
- // Extract PID + // Ensure the process started successfully and extract PID + assert(!startResult.isError, `start_process failed: ${startResult.content?.[0]?.text || 'unknown error'}`); const pidMatch = startResult.content[0].text.match(/Process started with PID (\d+)/); assert(pidMatch, 'Should get PID from start_process'); const pid = parseInt(pidMatch[1]);
54-58: Assert start_process succeeded before parsing PIDMirror the robustness added in the first test.
Apply this diff:
- // Extract PID + // Ensure the process started successfully and extract PID + assert(!startResult.isError, `start_process failed: ${startResult.content?.[0]?.text || 'unknown error'}`); const pidMatch = startResult.content[0].text.match(/Process started with PID (\d+)/); assert(pidMatch, 'Should get PID from start_process'); const pid = parseInt(pidMatch[1]);
62-70: Optionally assert presence of completion metadataterminalManager.getNewOutput for completed sessions returns exit code and runtime info. Verifying it here adds confidence that the “completed session” pathway was used.
You can add:
const readResult = await readProcessOutput({ pid, timeout_ms: 1000 }); assert(!readResult.isError, 'Should be able to read from immediately completed process'); assert(readResult.content[0].text.includes('IMMEDIATE OUTPUT'), 'Should contain immediate output from completed process'); + assert(readResult.content[0].text.match(/Process completed with exit code \d+/), + 'Should include completion info (exit code) from terminalManager');
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these settings in your CodeRabbit configuration.
📒 Files selected for processing (2)
src/tools/improved-process-tools.ts(1 hunks)test/test-read-completed-process-proper.js(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
test/test-read-completed-process-proper.js (1)
src/tools/improved-process-tools.ts (2)
startProcess(15-94)readProcessOutput(100-278)
src/tools/improved-process-tools.ts (1)
src/terminal-manager.ts (1)
getNewOutput(190-208)
🔇 Additional comments (1)
src/tools/improved-process-tools.ts (1)
113-122: Good fix: now returns output for completed sessionsEarly-returning completed output via terminalManager.getNewOutput(pid) when no active session exists addresses the core bug and aligns with the behavior of terminal-manager’s completedSessions. This should unblock users who try to read after the process exits.
| @@ -0,0 +1,97 @@ | |||
| import assert from 'assert'; | |||
| import { startProcess, readProcessOutput } from '../dist/tools/improved-process-tools.js'; | |||
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Avoid importing from dist unless CI builds first
The tests import from dist/tools/improved-process-tools.js. If CI doesn’t build before running tests, this will fail. Either:
- ensure the test runner builds first (e.g., npm test runs npm run build), or
- import from src and run tests with ts-node, or
- switch the test to require the compiled artifact only when present.
Run this script to verify that the dist artifact exists and that tests build before running:
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Checking for compiled artifact:"
fd improved-process-tools.js dist/tools | sed 's/^/ - /' || true
echo
echo "package.json scripts:"
if [ -f package.json ]; then
echo " - build: $(jq -r '.scripts.build // "N/A"' package.json)"
echo " - test: $(jq -r '.scripts.test // "N/A"' package.json)"
else
echo "No package.json found"
fi
echo
echo "Does test script run build first?"
jq -r '.scripts.test // ""' package.json | rg -q 'build' && echo " - Yes" || echo " - No"Length of output: 905
Tests import from dist but compiled artifact is missing and tests don't run build — fix required
The test at test/test-read-completed-process-proper.js imports dist/tools/improved-process-tools.js, but dist/tools is not present and package.json's test script does not run the build first.
Files/locations to address:
- test/test-read-completed-process-proper.js — line 2:
import ... from '../dist/tools/improved-process-tools.js' - package.json
- scripts.build: tsc && shx cp setup-claude-server.js uninstall-claude-server.js dist/ && shx chmod +x dist/*.js
- scripts.test: node test/run-all-tests.js (does NOT run build)
Recommended fixes (pick one):
- Update package.json test script to build first, e.g.:
"test": "npm run build && node test/run-all-tests.js"
- OR change tests to import from src (e.g., ../src/...) and run tests via ts-node or a test runner that supports TS.
- OR make the test require the dist artifact only when present (guard the import/require).
🤖 Prompt for AI Agents
In test/test-read-completed-process-proper.js around line 2 and package.json,
the test imports from ../dist/tools/improved-process-tools.js but the compiled
dist artifact isn't built by the current test script; update package.json's
"test" script to run the build first by changing it to something like "npm run
build && node test/run-all-tests.js" so dist is produced before tests run
(alternatively adjust tests to import from src or guard the import if you
prefer, but the immediate fix is to prepend the build step in package.json).
| const startResult = await startProcess({ | ||
| command: 'sleep 1 && echo "SUCCESS MESSAGE"', | ||
| timeout_ms: 500 // Returns before the echo happens | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Make the delayed command cross-platform
Using sleep will fail on Windows shells. Prefer a cross-platform Node one-liner that works under both cmd.exe and POSIX shells.
Apply this diff:
- const startResult = await startProcess({
- command: 'sleep 1 && echo "SUCCESS MESSAGE"',
- timeout_ms: 500 // Returns before the echo happens
- });
+ const startResult = await startProcess({
+ // Cross-platform delay + output using Node
+ command: 'node -e "setTimeout(() => console.log(\'SUCCESS MESSAGE\'), 1000)"',
+ timeout_ms: 500 // Returns before the output happens
+ });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const startResult = await startProcess({ | |
| command: 'sleep 1 && echo "SUCCESS MESSAGE"', | |
| timeout_ms: 500 // Returns before the echo happens | |
| }); | |
| const startResult = await startProcess({ | |
| // Cross-platform delay + output using Node | |
| command: 'node -e "setTimeout(() => console.log(\'SUCCESS MESSAGE\'), 1000)"', | |
| timeout_ms: 500 // Returns before the output happens | |
| }); |
🤖 Prompt for AI Agents
In test/test-read-completed-process-proper.js around lines 15 to 18, the test
uses the shell command "sleep 1 && echo 'SUCCESS MESSAGE'" which fails on
Windows; replace that command with a cross-platform Node one‑liner that uses
node -e and setTimeout to emit "SUCCESS MESSAGE" after ~1000ms (so the existing
timeout_ms: 500 still causes the process to return before the message). Update
the command string to the Node one-liner and keep the timeout_ms unchanged.
| const startResult = await startProcess({ | ||
| command: 'echo "IMMEDIATE OUTPUT"', | ||
| timeout_ms: 2000 | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Make the immediate command cross-platform
echo quoting/availability differs across shells; the Node one-liner is portable.
Apply this diff:
- const startResult = await startProcess({
- command: 'echo "IMMEDIATE OUTPUT"',
- timeout_ms: 2000
- });
+ const startResult = await startProcess({
+ command: 'node -e "console.log(\'IMMEDIATE OUTPUT\')"',
+ timeout_ms: 2000
+ });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const startResult = await startProcess({ | |
| command: 'echo "IMMEDIATE OUTPUT"', | |
| timeout_ms: 2000 | |
| }); | |
| const startResult = await startProcess({ | |
| command: 'node -e "console.log(\'IMMEDIATE OUTPUT\')"', | |
| timeout_ms: 2000 | |
| }); |
🤖 Prompt for AI Agents
In test/test-read-completed-process-proper.js around lines 49 to 52, the test
uses an OS/shell-dependent echo command; replace it with a portable Node
one-liner using the current Node binary so it works across platforms (e.g. set
command to `${process.execPath} -e "process.stdout.write('IMMEDIATE OUTPUT')"`),
ensuring proper quoting/escaping in the JS string.
Noticed that if process finished early it fails to read its final outputs
That functionality is there just not used correctly in read output.
Added tests and fixed