feat: replace tree-kill with shared implementation#600
Conversation
Co-authored-by: WillBooster (Codex CLI) <agent@willbooster.com>
Summary of ChangesHello, 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 centralizes process tree killing logic within the monorepo by implementing a custom, cross-platform Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
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
|
Co-authored-by: WillBooster (Codex CLI) <agent@willbooster.com>
Co-authored-by: WillBooster (Codex CLI) <agent@willbooster.com>
Co-authored-by: WillBooster (Codex CLI) <agent@willbooster.com>
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request replaces the external tree-kill dependency with a custom, in-house treeKill implementation within the shared-lib-node package, which is then integrated into the spawnAsync utility and exposed as a new wb tree-kill command. The custom treeKill function handles process tree termination for both Windows and other operating systems, including error handling for non-existent processes. Review comments emphasized the importance of awaiting the treeKill function in signal handlers to ensure proper cleanup before process.exit() is called, and suggested a more concise approach for managing child process PIDs within the collectDescendantPids function.
Co-authored-by: WillBooster (Codex CLI) <agent@willbooster.com>
Co-authored-by: WillBooster (Codex CLI) <agent@willbooster.com>
|
/gemini review |
There was a problem hiding this comment.
Code Review
このプルリクエストは、外部のtree-killパッケージをshared-lib-node内のネイティブ実装に置き換えるものです。これにより、依存関係が削減され、プロセスの終了処理が改善されます。新しいtreeKillの実装は、WindowsとUnix系システムの両方に対応しており、堅牢なテストも追加されています。また、wb tree-killという新しいCLIコマンドも導入されています。全体的に、これは素晴らしい改善ですが、いくつかの点でさらに改善できる可能性があります。特に、テストコードの重複や、エラーハンドリングの一貫性についてコメントしました。
Co-authored-by: WillBooster (Codex CLI) <agent@willbooster.com>
|
/gemini review |
Co-authored-by: WillBooster (Codex CLI) <agent@willbooster.com>
Co-authored-by: WillBooster (Codex CLI) <agent@willbooster.com>
|
/gemini review |
Co-authored-by: WillBooster (Codex CLI) <agent@willbooster.com>
|
/gemini review |
Co-authored-by: WillBooster (Codex CLI) <agent@willbooster.com>
|
/gemini review |
Co-authored-by: WillBooster (Codex CLI) <agent@willbooster.com>
|
/gemini review |
There was a problem hiding this comment.
Code Review
このプルリクエストは、外部の tree-kill パッケージを独自の共有実装に置き換えるものです。これにより、新しい wb tree-kill コマンドが追加され、テストも拡充されています。全体的に良い変更ですが、いくつか改善点があります。主な懸念点は、新しい treeKill の実装が同期的であり、イベントループをブロックする可能性があることです。これを非同期に修正することを強く推奨します。また、コードの可読性を向上させるための小さなリファクタリングも提案しました。
Co-authored-by: WillBooster (Codex CLI) <agent@willbooster.com>
|
/gemini review |
There was a problem hiding this comment.
Code Review
このプルリクエストは、tree-kill 依存関係をネイティブ実装に置き換えるもので、外部依存を減らす素晴らしい取り組みです。実装はWindowsと非Windowsプラットフォームの両方をカバーし、包括的なテストも含まれており、よく構造化されています。また、wb tree-kill コマンドの追加も便利なユーティリティです。spawn.ts と packages/wb/src/index.ts でシャットダウン時のエラー処理や再入防止の変更が加えられており、堅牢性が向上しています。新しい treeKill.ts の実装について、コードの明確性と正確性を向上させるための提案がいくつかあります。
Co-authored-by: WillBooster (Codex CLI) <agent@willbooster.com>
|
/gemini review |
Co-authored-by: WillBooster (Codex CLI) <agent@willbooster.com>
|
/gemini review |
|
Declining the async-conversion suggestion for in this PR. We intentionally keep synchronous in this codebase to keep shutdown/signal paths deterministic and avoid reintroducing Promise-handling complexity at call sites ( cleanup and signal handling). We already bounded command execution with timeouts, and this PR’s direction is a non-async API. If we later observe real event-loop blocking in production, we can add a separate API without changing current behavior. |
|
Declining the async-conversion suggestion for treeKill in this PR. We intentionally keep treeKill synchronous in this codebase to keep shutdown/signal paths deterministic and avoid reintroducing Promise-handling complexity at call sites (spawn cleanup and wb signal handling). We already bound command execution with timeouts, and this PR direction is a non-async API. If we later observe real event-loop blocking in production, we can add a separate treeKillAsync API without changing current behavior. |
Co-authored-by: WillBooster (Codex CLI) <agent@willbooster.com>
|
/gemini review |
There was a problem hiding this comment.
Code Review
このプルリクエストは、tree-kill パッケージを shared-lib-node 内のネイティブ実装に置き換えるものです。この新しい実装は shared-lib-node と wb の両方で利用されます。また、新しい wb tree-kill コマンドと、それを検証するための堅牢なテストが追加されています。全体として、プロセスキル操作のエラーハンドリングが追加されるなど、堅牢性が向上しており、よく構造化された変更です。コードの正確性と可読性の観点から、2点改善提案があります。
Co-authored-by: WillBooster (Codex CLI) <agent@willbooster.com>
|
/gemini review |
Co-authored-by: WillBooster (Codex CLI) <agent@willbooster.com>
Summary
wb tree-kill <pid> [signal]commandVerification