fix(cli): Move stdin processing to main process to resolve hang in child_process worker#878
Conversation
…ild_process worker Issue #867で報告された--stdinオプションのハング問題を修正しました。 v1.6.0でdefaultActionがchild_processで実行されるようになりましたが、 child_processではデフォルトで親プロセスのstdinが継承されないため、 worker内でprocess.stdinを読み取ろうとするとハングしていました。 Changes: - stdin処理をメインプロセスに移動し、読み取り結果をworkerに渡すように変更 - DefaultActionTask interfaceをisStdinからstdinFilePathsに変更 - child_processによるリソース分離と安定性を維持しながら、stdin機能を復元 Fixes #867
WalkthroughMoves --stdin file-path reading from the worker to the main defaultAction, validating no directories are passed with --stdin. The main action reads stdin file paths and passes them as stdinFilePaths to the worker. Worker logic now branches on stdinFilePaths presence. Tests updated to reflect the new task shape and flow. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant CLI as defaultAction (main)
participant Worker as defaultActionWorker
participant Pack as pack()
User->>CLI: repomix --stdin
note over CLI: Read file paths from stdin
CLI->>CLI: validate no directory args with --stdin
CLI->>Worker: start task { stdinFilePaths }
alt stdinFilePaths provided
Worker->>Pack: pack({ explicitPaths: stdinFilePaths })
else no stdinFilePaths
Worker->>Pack: pack({ search in provided directories })
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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. Comment |
There was a problem hiding this comment.
Pull Request Overview
This PR fixes a hang issue with the --stdin option by moving stdin processing from the child_process worker to the main process, resolving the problem where workers couldn't access stdin by default.
- Move stdin file path reading from worker to main process before worker creation
- Update
DefaultActionTaskinterface to usestdinFilePathsarray instead ofisStdinboolean - Remove stdin validation and error handling from worker since it's now handled in main process
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/cli/actions/defaultAction.ts | Added stdin processing in main process with validation and passes file paths to worker |
| src/cli/actions/workers/defaultActionWorker.ts | Updated to receive pre-processed stdin file paths instead of handling stdin directly |
| tests/cli/actions/defaultAction.test.ts | Updated test mocks and expectations for new stdin handling approach |
| tests/cli/actions/workers/defaultActionWorker.test.ts | Removed stdin-related mocks and tests, updated task objects to use new interface |
Summary of ChangesHello @yamadashy, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a critical bug where the CLI's Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #878 +/- ##
==========================================
- Coverage 88.85% 88.81% -0.05%
==========================================
Files 109 109
Lines 7575 7580 +5
Branches 1423 1425 +2
==========================================
+ Hits 6731 6732 +1
- Misses 844 848 +4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Code Review
This pull request effectively resolves the hanging issue with the --stdin option by moving the standard input processing from the child process worker to the main process. The approach of reading file paths in the main process and passing them to the worker is a clean solution that maintains the benefits of using a child process. The code changes are logical and the refactoring of tests is mostly correct. However, there's a gap in test coverage for the moved validation logic, which should be addressed.
Deploying repomix with
|
| Latest commit: |
1bb752a
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://ccb8c7f6.repomix.pages.dev |
| Branch Preview URL: | https://fix-stdin.repomix.pages.dev |
Code Review for PR #878Thank you for this fix! The solution elegantly addresses the stdin hang issue by moving stdin processing to the main process before worker creation. Here's my detailed review: ✅ Strengths
🎯 Code Quality Observations
💡 Suggestions for Improvement
✅ Security & Performance
✅ Testing
ConclusionThis is a well-crafted fix that addresses the issue without compromising the architecture. The code changes are minimal, focused, and maintain good separation of concerns. With the minor suggestions above (especially the error handling), this PR is ready to merge. Great work on the diagnosis and fix! 🎉 |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/cli/actions/workers/defaultActionWorker.test.ts (1)
224-238: Consider testing edge case: empty stdinFilePaths array.The test handles empty directories array but passes a non-empty
stdinFilePaths. Consider adding a test case that verifies behavior whenstdinFilePathsis an empty array to ensure the worker handles this edge case gracefully.Example test case:
it('should handle empty stdinFilePaths array', async () => { const task: DefaultActionTask = { directories: ['.'], cwd: '/test/project', config: mockConfig, cliOptions: mockCliOptions, stdinFilePaths: [], }; mockPack.mockResolvedValueOnce(mockPackResult); await defaultActionWorker(task); expect(mockPack).toHaveBeenCalledWith( ['/test/project'], mockConfig, expect.any(Function), {}, [] ); });
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/cli/actions/defaultAction.ts(3 hunks)src/cli/actions/workers/defaultActionWorker.ts(4 hunks)tests/cli/actions/defaultAction.test.ts(4 hunks)tests/cli/actions/workers/defaultActionWorker.test.ts(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/cli/actions/defaultAction.ts (3)
src/shared/errorHandle.ts (1)
RepomixError(6-11)src/core/file/fileStdin.ts (1)
readFilePathsFromStdin(69-116)src/cli/actions/workers/defaultActionWorker.ts (1)
DefaultActionTask(12-18)
tests/cli/actions/workers/defaultActionWorker.test.ts (1)
src/cli/actions/workers/defaultActionWorker.ts (1)
DefaultActionTask(12-18)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: Build and run (windows-latest, 22.x)
- GitHub Check: Build and run (windows-latest, 24.x)
- GitHub Check: Build and run (windows-latest, 20.x)
- GitHub Check: Test (windows-latest, 22.x)
- GitHub Check: Test (windows-latest, 20.x)
- GitHub Check: Build and run with Bun (windows-latest, latest)
- GitHub Check: Cloudflare Pages
🔇 Additional comments (8)
tests/cli/actions/defaultAction.test.ts (2)
90-93: LGTM: Mock setup aligns with new stdin handling flow.The mock correctly returns both
filePathsandemptyDirPaths, matching theStdinFileResulttype structure fromreadFilePathsFromStdin.
180-198: Verify directory validation for stdin mode.The test verifies that
stdinFilePathsis passed to the worker task, but it doesn't verify the directory validation logic introduced in the main action (lines 58-62 ofsrc/cli/actions/defaultAction.ts). Consider adding a test case that verifies the error thrown when multiple directories or non-"." directories are provided with--stdin.Example test case to add:
it('should throw error when stdin mode is used with invalid directory arguments', async () => { const options: CliOptions = { stdin: true, }; await expect(runDefaultAction(['src', 'tests'], process.cwd(), options)).rejects.toThrow( 'When using --stdin, do not specify directory arguments' ); });src/cli/actions/defaultAction.ts (3)
57-62: LGTM: Directory validation prevents confusing behavior.The validation correctly prevents users from specifying directory arguments when using
--stdin, which would be ambiguous. The error message is clear and actionable.
80-87: LGTM: Task construction properly includes stdin file paths.The optional
stdinFilePathsfield is correctly added to the worker task, allowing the worker to distinguish between stdin and directory modes.
52-67: Remove empty-stdin validation suggestion
readFilePathsFromStdinalready throwsRepomixError('No valid file paths found in stdin input.')on empty input.src/cli/actions/workers/defaultActionWorker.ts (2)
58-72: LGTM: Worker correctly processes stdin file paths from main process.The branching logic is clean and the worker now delegates stdin reading to the main process, resolving the child_process stdin inheritance issue. The
pack()call correctly passesstdinFilePathsas the 5th argument.
73-80: LGTM: Directory processing path remains unchanged.The else branch maintains the original directory processing logic, ensuring backward compatibility for non-stdin usage.
tests/cli/actions/workers/defaultActionWorker.test.ts (1)
201-222: LGTM: Test correctly verifies stdin file paths are passed to pack().The test properly validates that
stdinFilePathsfrom the task is forwarded to thepack()function as the 5th argument, matching the new worker implementation.
This PR fixes the
--stdinoption hang issue reported in #867.Summary
In v1.6.0,
defaultActionwas changed to run in achild_processworker for better resource isolation. However,child_processworkers don't inherit stdin from the parent process by default, causing the worker to hang when trying to read fromprocess.stdin.Changes
DefaultActionTaskinterface fromisStdintostdinFilePathsTechnical Details
The root cause was that
readFilePathsFromStdin()was called inside the child_process worker, but stdin was not inherited. The fix moves stdin reading to the main process and passes the file paths to the worker as a parameter.This approach:
Checklist
npm run testnpm run lintFixes #867