fix: prevent terminal corruption during background bun install#2369
fix: prevent terminal corruption during background bun install#2369code-yeongyu wants to merge 2 commits intodevfrom
Conversation
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-opencode) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-opencode) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
There was a problem hiding this comment.
3 issues found across 4 files
Confidence score: 2/5
- There is a high-confidence regression risk in
src/cli/config-manager/bun-install.ts: waiting for process exit/output after timeout can hang indefinitely on Windows when orphaned child processes keep pipes open. src/cli/config-manager/bun-install.tsalso needs anoutputPromise.catch(...)path to avoid unhandled promise rejections during async stream-read failures, which can destabilize CLI execution.- Given the 8/10 and 7/10 issues are both in install flow process handling, merge risk is elevated until timeout/error handling is hardened.
- Pay close attention to
src/cli/config-manager/bun-install.tsandsrc/cli/config-manager/bun-install.test.ts- process lifecycle/error handling and environment-variable cleanup need fixes to avoid hangs and test-environment side effects.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/cli/config-manager/bun-install.test.ts">
<violation number="1" location="src/cli/config-manager/bun-install.test.ts:22">
P2: Unconditionally deleting `process.env.OPENCODE_CONFIG_DIR` can corrupt the test environment if it was previously set by a developer or another test suite.
Without restoring the original environment variable, this test pollutes the environment and can cause flaky, hard-to-debug failures in other concurrent tests that rely on `OPENCODE_CONFIG_DIR` (as seen safely handled in `install.test.ts`).</violation>
</file>
<file name="src/cli/config-manager/bun-install.ts">
<violation number="1" location="src/cli/config-manager/bun-install.ts:74">
P1: Add a `.catch()` to `outputPromise` to prevent unhandled promise rejections if stream reading fails asynchronously.</violation>
<violation number="2" location="src/cli/config-manager/bun-install.ts:95">
P1: Awaiting process exit and output on timeout can cause indefinite hangs on Windows due to orphaned child processes holding pipes open.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| }) | ||
|
|
||
| let timeoutId: ReturnType<typeof setTimeout> | ||
| const outputPromise = Promise.all([readProcessOutput(proc.stdout), readProcessOutput(proc.stderr)]).then( |
There was a problem hiding this comment.
P1: Add a .catch() to outputPromise to prevent unhandled promise rejections if stream reading fails asynchronously.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/cli/config-manager/bun-install.ts, line 74:
<comment>Add a `.catch()` to `outputPromise` to prevent unhandled promise rejections if stream reading fails asynchronously.</comment>
<file context>
@@ -15,36 +36,77 @@ export async function runBunInstall(): Promise<boolean> {
})
- let timeoutId: ReturnType<typeof setTimeout>
+ const outputPromise = Promise.all([readProcessOutput(proc.stdout), readProcessOutput(proc.stderr)]).then(
+ ([stdout, stderr]) => ({ stdout, stderr })
+ )
</file context>
| /* intentionally empty - process may have already exited */ | ||
| } | ||
|
|
||
| await proc.exited |
There was a problem hiding this comment.
P1: Awaiting process exit and output on timeout can cause indefinite hangs on Windows due to orphaned child processes holding pipes open.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/cli/config-manager/bun-install.ts, line 95:
<comment>Awaiting process exit and output on timeout can cause indefinite hangs on Windows due to orphaned child processes holding pipes open.</comment>
<file context>
@@ -15,36 +36,77 @@ export async function runBunInstall(): Promise<boolean> {
/* intentionally empty - process may have already exited */
}
+
+ await proc.exited
+ logCapturedOutputOnFailure(outputMode, await outputPromise)
+
</file context>
| } | ||
|
|
||
| describe("runBunInstallWithDetails", () => { | ||
| beforeEach(() => { |
There was a problem hiding this comment.
P2: Unconditionally deleting process.env.OPENCODE_CONFIG_DIR can corrupt the test environment if it was previously set by a developer or another test suite.
Without restoring the original environment variable, this test pollutes the environment and can cause flaky, hard-to-debug failures in other concurrent tests that rely on OPENCODE_CONFIG_DIR (as seen safely handled in install.test.ts).
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/cli/config-manager/bun-install.test.ts, line 22:
<comment>Unconditionally deleting `process.env.OPENCODE_CONFIG_DIR` can corrupt the test environment if it was previously set by a developer or another test suite.
Without restoring the original environment variable, this test pollutes the environment and can cause flaky, hard-to-debug failures in other concurrent tests that rely on `OPENCODE_CONFIG_DIR` (as seen safely handled in `install.test.ts`).</comment>
<file context>
@@ -0,0 +1,96 @@
+}
+
+describe("runBunInstallWithDetails", () => {
+ beforeEach(() => {
+ process.env.OPENCODE_CONFIG_DIR = "/test/opencode"
+ resetConfigContext()
</file context>
Summary
runBunInstallWithDetails()so background callers can pipe bun install output instead of inheriting the active terminalTesting
bun testbun test src/cli/config-manager/bun-install.test.ts src/hooks/auto-update-checker/hook/background-update-check.test.tsbun run typecheckbun run buildCloses #2238
Summary by cubic
Prevents terminal corruption during background bun install by adding a silent/piped mode and using it in the auto-update checker. Keeps interactive installs unchanged and only logs output on failure or timeout. Fixes #2238.
Written for commit 9f94c5f. Summary will update on new commits.