-
Notifications
You must be signed in to change notification settings - Fork 0
Fix race condition in spawn error handling and extract FIFO functionality #27
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
…nput Co-authored-by: eriksw <[email protected]>
Co-authored-by: eriksw <[email protected]>
…type safety Co-authored-by: eriksw <[email protected]>
Co-authored-by: eriksw <[email protected]>
Co-authored-by: eriksw <[email protected]>
Co-authored-by: eriksw <[email protected]>
Co-authored-by: eriksw <[email protected]>
|
@copilot The lint fails with findings in main.ts. This should have been run by the agent. Identify why lint was not run and fix the copilot instructions to ensure it is run before making the pull request. All jobs in the signal handling test workflow failed because they were not updated. Update copilot instructions to ensure that the workflow is updated when changes that might affect the tests in the signal handling test workflow. This PR reduces test coverage substantially. Ensure coverage is as good as possible. The It is okay to swallow errors unlinking the fifo while handling unrelated, but errors unlinking it when it is supposed to be unlinked should still be fatal. The executeCommand function has far too many responsibilities and is far too long. Consider extracting the pipe-creating functionality to another module. Review the e2e tests and simplify away any redundant tests that do not improve code coverage. |
Co-authored-by: eriksw <[email protected]>
Fixed in commit 9924cba: Lint errors: Fixed all 4 unused variable errors in main.ts by removing unused error parameters in catch blocks. Why lint wasn't run: The copilot instructions mentioned lint should pass (line 109) but didn't explicitly require running
Signal-test workflow: Updated all 4 test jobs to add Still addressing: extracting pipe functionality, simplifying e2e tests, improving coverage, and handling FIFO unlink errors appropriately. |
Co-authored-by: eriksw <[email protected]>
Co-authored-by: eriksw <[email protected]>
Co-authored-by: eriksw <[email protected]>
Co-authored-by: eriksw <[email protected]>
Co-authored-by: eriksw <[email protected]>
Co-authored-by: eriksw <[email protected]>
|
@copilot There are NATURAL_LANGUAGE lint errors in markdown files that must be fixed. Incorporate the following into the instructions: Next, and importantly: the main.ts is too large to review and maintain effectively. In particular, executeCommand is too long. Extract the code relating fifo-based pipe functionality to a different module. If practical, consider abstracting the fifo-related code into a class that can be treated like the child.stdout / child.stderr of a normal spawn() with pipe stdout/stderr. |
Co-authored-by: eriksw <[email protected]>
Fixed in commit b5766da: Markdown lint errors: Fixed terminology (jest → Jest, typescript → TypeScript). Super-linter instructions: Added to FIFO code extracted: Created FIFO tests: Added Overall: 76 tests passing (was 61), 89.29% coverage (was 83.67%), 0 lint errors, all TypeScript modules. Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
|
Investigate using this approach to create a pipe the same way spawn does, instead of using the fifo module.: const { Pipe, constants: PipeConstants } = internalBinding('pipe_wrap'); |
Command spawn errors resolved instead of rejecting in separate output mode due to event handler registration order. The
setupSignalHandlerserror handler fired first and resolved withexitCode: 0before the main error handler could reject.Changes
Event handler ordering (src/main.ts)
setupSignalHandlersin separate output modeFIFO functionality extraction (src/fifo-stream.ts)
FifoPipeclass in separate moduleopen(),getOutput(),waitForCompletion(),close()__tests__/fifo-stream.test.ts(15 tests)Test refactoring (tests/main.test.ts, tests/fifo-stream.test.ts)
jest/no-conditional-expectlint violationsDocumentation (.github/copilot-instructions.md, eslint.config.mjs)
Test Results
76 tests now pass consistently (was 61). Previously skipped test "rejects when command not found" passes in both modes.
Overall Coverage: 89.29% (improved from 83.67%)
Original prompt
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.