add new readProcessOutput2 logic for long time task #244
Conversation
for example: compile large project / download large file / etc this new logic will avoid to call readProcessOutput frequently by agent
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (8)
src/tools/improved-process-tools.ts (3)
408-411: Unreachable “📊 Output collection completed” branch.In v2 you never call resolveOnce unless early-exit or timeout, so this else path won’t occur. Either remove it or add a “quiescence” exit (e.g., N cycles with no new output).
Apply this minimal fix to remove dead branch:
- } else { - statusMessage = '\n📊 Output collection completed'; - } + }
355-375: Make poll interval configurable.Hard-coded 200ms may be suboptimal across environments. Consider a config value (e.g., processPollingIntervalMs) with sane default.
321-333: Duplication with readProcessOutput.Common scaffolding (arg parsing, session checks, final state/formatting) is duplicated. Extract a small helper to reduce drift.
test/test-read-process-output2.js (4)
36-39: Make the long-running command cross‑platform.sleep isn’t available on Windows. Use Node to simulate delay.
Apply:
- const sleepResult = await startProcess({ - command: 'sleep 3 && echo "Sleep finished"', - timeout_ms: 1000 - }); + const sleepResult = await startProcess({ + command: 'node -e "setTimeout(() => console.log(\\"Sleep finished\\"), 3000)"', + timeout_ms: 1000 + });
95-103: Reduce flakiness: assert on state text, not elapsed time.Timing thresholds can be noisy. Prefer checking for the “🔄” waiting-for-input marker in responses.
Apply:
- if (elapsed4 < 2000) { - console.log('✅ readProcessOutput2 detected REPL prompt correctly'); - } else if (elapsed3 < 2000) { - console.log('✅ readProcessOutput detected REPL prompt correctly'); - } else { - console.log('❌ Neither function detected REPL prompt correctly'); - throw new Error('Neither function detected REPL prompt correctly'); - } + const hasWaiting2 = /🔄/.test(pythonOutput2.content[0].text); + const hasWaiting1 = /🔄/.test(pythonOutput1.content[0].text); + if (!(hasWaiting1 || hasWaiting2)) { + throw new Error('Neither function reported waiting-for-input (🔄) state'); + }
22-24: Validate extracted PID before proceeding.If PID parsing fails, subsequent calls use null and mask issues.
Apply:
- const echoPid = extractPid(echoResult.content[0].text); + const echoPid = extractPid(echoResult.content[0].text); + assert.ok(Number.isInteger(echoPid), 'Failed to parse echo PID'); @@ - const sleepPid = extractPid(sleepResult.content[0].text); + const sleepPid = extractPid(sleepResult.content[0].text); + assert.ok(Number.isInteger(sleepPid), 'Failed to parse sleep PID'); @@ - const pythonPid = extractPid(pythonResult.content[0].text); + const pythonPid = extractPid(pythonResult.content[0].text); + assert.ok(Number.isInteger(pythonPid), 'Failed to parse Python PID');Also applies to: 45-47, 74-76
1-1: Remove unused import or use it.assert is imported; with the above checks it’s now used. If you skip those checks, drop the import.
src/handlers/terminal-handlers.ts (1)
36-42: Handler wiring looks correct; consider avoiding double-parse.You parse with Zod here and readProcessOutput2 safe-parses again. Passing args through (and handling errors in the tool) would reduce duplication.
Apply:
-export async function handleReadProcessOutput2(args: unknown): Promise<ServerResult> { - const parsed = ReadProcessOutputArgsSchema.parse(args); - return readProcessOutput2(parsed); -} +export async function handleReadProcessOutput2(args: unknown): Promise<ServerResult> { + return readProcessOutput2(args); +}Ensure handlers/index.js re-exports handleReadProcessOutput2.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/handlers/terminal-handlers.ts(2 hunks)src/server.ts(2 hunks)src/tools/improved-process-tools.ts(1 hunks)test/test-read-process-output2.js(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
src/handlers/terminal-handlers.ts (3)
src/types.ts (1)
ServerResult(51-55)src/tools/schemas.ts (1)
ReadProcessOutputArgsSchema(27-30)src/tools/improved-process-tools.ts (1)
readProcessOutput2(285-421)
src/server.ts (1)
src/tools/schemas.ts (1)
ReadProcessOutputArgsSchema(27-30)
test/test-read-process-output2.js (1)
src/tools/improved-process-tools.ts (3)
startProcess(15-94)readProcessOutput(100-278)readProcessOutput2(285-421)
src/tools/improved-process-tools.ts (4)
src/types.ts (1)
ServerResult(51-55)src/tools/schemas.ts (1)
ReadProcessOutputArgsSchema(27-30)src/terminal-manager.ts (1)
terminalManager(261-261)src/utils/process-detection.ts (3)
ProcessState(6-12)analyzeProcessState(54-130)formatProcessStateMessage(172-180)
🔇 Additional comments (5)
src/tools/improved-process-tools.ts (2)
285-314: Good addition: v2 logic matches the PR goal.Early-exit only on waiting/finished; otherwise collect until timeout. This should reduce frequent reads for long tasks.
354-384: Ensure timers are always cleared; current cleanup looks correct.resolveOnce() guards double-resolve and clears both interval and timeout. No leak concerns here.
If TerminalManager can destroy sessions mid-collection, confirm getNewOutput(pid) behaves safely (returns "" and doesn’t throw).
test/test-read-process-output2.js (1)
120-128: Precondition: ensure dist is built before running this script.If CI runs tests before build, this will fail to import. Add a pretest script (npm) or a guard at runtime that throws a clear error when imports are undefined.
src/server.ts (2)
585-614: Tool registration LGTM.Description clearly differentiates v2 semantics; schema is correct.
975-977: Approve — handler export and routing verified. Call routing LGTM; handleReadProcessOutput2 is exported from src/handlers/terminal-handlers.ts, re-exported in src/handlers/index.ts, and server imports handlers and correctly delegates in the switch.
| // Test 3: Python REPL - should both detect waiting for input immediately | ||
| console.log('\n📝 Test 3: Python REPL (should detect waiting for input)'); | ||
| const pythonResult = await startProcess({ | ||
| command: 'python3 -i', | ||
| timeout_ms: 3000 | ||
| }); | ||
|
|
||
| if (pythonResult.isError) { | ||
| console.log('Python not available, skipping REPL test'); | ||
| } else { | ||
| const pythonPid = extractPid(pythonResult.content[0].text); | ||
| console.log(`Started Python REPL with PID: ${pythonPid}`); | ||
|
|
There was a problem hiding this comment.
Terminate the Python REPL to avoid leaking processes.
Add cleanup via forceTerminate when REPL is started.
Apply:
-import { startProcess, readProcessOutput, readProcessOutput2 } from '../dist/tools/improved-process-tools.js';
+import { startProcess, readProcessOutput, readProcessOutput2, forceTerminate } from '../dist/tools/improved-process-tools.js';
@@
- } else {
+ } else {
const pythonPid = extractPid(pythonResult.content[0].text);
console.log(`Started Python REPL with PID: ${pythonPid}`);
@@
- // Check if either one detected the prompt correctly
+ // Check if either one detected the prompt correctly
if (elapsed4 < 2000) {
console.log('✅ readProcessOutput2 detected REPL prompt correctly');
} else if (elapsed3 < 2000) {
console.log('✅ readProcessOutput detected REPL prompt correctly');
} else {
console.log('❌ Neither function detected REPL prompt correctly');
throw new Error('Neither function detected REPL prompt correctly');
}
+ // Cleanup REPL
+ try {
+ await forceTerminate({ pid: pythonPid });
+ } catch (_) {
+ // best-effort
+ }
}Also applies to: 80-104
|
Hm, I am looking at it and thinking that I would probably merge it with original process. You mentioned that your main problem is that read output returns right away and you prefer for it to wait? |
|
For example, consider a compilation task that is initiated and expected to take approximately 30 minutes. This task periodically outputs new logs every few seconds. In such a scenario, the read_process_outputfunction returns immediately upon receiving each new log entry instead of waiting for the full timeout period. It only triggers the timeout mechanism when no new log content arrives consecutively. As a result, the Agent ends up invoking read_process_outputfrequently—even if the timeout is configured to a very large value—because once partial logs are obtained, the function does not need to wait out the entire timeout duration. These frequent invocations of the LLM lead to significant consumption of input tokens, particularly when the context involved is extensive. |
for example: compile large project / download large file / etc this new logic will avoid to call readProcessOutput frequently by agent
Summary by CodeRabbit
New Features
Tests