fix: flush process output before completion#455
Open
Showiix wants to merge 3 commits into
Open
Conversation
Contributor
|
CodeAnt AI is reviewing your PR. Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughSwitches terminal-manager's lifecycle listener from the child process Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant TerminalManager
participant ChildProcess
participant Storage
Client->>TerminalManager: executeCommand(cmd, ...)
TerminalManager->>ChildProcess: spawn process
ChildProcess-->>TerminalManager: stdout/stderr (stream chunks)
ChildProcess-->>TerminalManager: exit (process exits)
note right of TerminalManager: do not finalize session yet
ChildProcess-->>TerminalManager: close (stdio streams closed)
TerminalManager->>Storage: persist completed session (output, exitCode)
TerminalManager->>Client: mark session complete / isBlocked = false
Client->>TerminalManager: readOutputPaginated(...) -> returns persisted output
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Review rate limit: 3/8 reviews remaining, refill in 31 minutes and 53 seconds.Comment |
codeant-ai
Bot
added
the
size:M
![codeant-ai[bot]](https://avatars.githubusercontent.com/in/646884?s=60&v=4){kind=small}
| await fs.writeFile( | ||
| SCRIPT_PATH, | ||
| [ | ||
| "import fs from 'fs';", |
Contributor
There was a problem hiding this comment.
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
Contributor
|
CodeAnt AI finished reviewing your PR. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/test-process-output-close.js`:
- Around line 14-31: The setup() function can leak the temporary directory if
fs.writeFile throws; modify setup() so that after creating testDir (via
fs.mkdtemp(TEST_DIR_PREFIX)) you guard the subsequent file operations with a
try/catch or try/finally that calls fs.rm(testDir, { recursive: true, force:
true }) on error, or alternatively make setup() return a disposer/teardown
callback that always removes testDir on failure; ensure you reference the
created scriptPath and call fs.rm in the error path so partially created
fixtures are cleaned up (leave teardown(testDir) as the successful teardown
path).
- Around line 69-73: The entrypoint check using import.meta.url ===
`file://${process.argv[1]}` can fail for relative process.argv[1]; update the
check to normalize process.argv[1] with pathToFileURL before comparison (like in
test-widget-state-runtime.js and test-markdown-preview.js). Locate the
conditional that wraps runTests() (the import.meta.url comparison) and replace
the raw file://${process.argv[1]} comparison with
pathToFileURL(process.argv[1]).toString() so import.meta.url reliably matches
whether argv[1] is relative or absolute, leaving the runTests() invocation
unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 67690712-27e5-4fb5-bc4d-2be149702379
📒 Files selected for processing (1)
test/test-process-output-close.js
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
User description
Summary
Fixes #395
Test
CodeAnt-AI Description
Keep fast-exiting process output until stdio finishes
What Changed
Impact
✅ Fewer missing error messages✅ More complete process output✅ Clearer fast-failure results🔄 Retrigger CodeAnt AI Review
Details
💡 Usage Guide
Checking Your Pull Request
Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.
Talking to CodeAnt AI
Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:
This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.
Example
Preserve Org Learnings with CodeAnt
You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:
This helps CodeAnt AI learn and adapt to your team's coding style and standards.
Example
Retrigger review
Ask CodeAnt AI to review the PR again, by typing:
Check Your Repository Health
To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.
Summary by CodeRabbit
Bug Fixes
Tests