feat: run add-dependency installs in a PTY#3167
Conversation
|
@BugBot run |
|
@BugBot run |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![dyad-assistant[bot]](https://avatars.githubusercontent.com/in/3027874?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
🔍 Dyadbot Code Review SummaryVerdict: 🤔 NOT SURE — several MEDIUM issues worth addressing, no clear blockers Reviewed by 3 independent agents: Correctness Expert, Code Health Expert, UX Wizard. Issues Summary
Why "NOT SURE" rather than "Ready to merge"The packaging changes ( What's holding back a green light is that the stderr → stdout merge ripples through code that wasn't updated in this PR. Most importantly:
None of these are crash-or-data-loss issues, but together they make me uncertain the add-dependency error UX still works the way the tests claim it does. Strongly recommend a manual or e2e run of 🟢 Low Priority Notes (6 items)
🚫 Dropped False Positives / Out-of-scope (1 item)
Generated by Dyadbot multi-agent code review |
github-actions
bot
added
the
needs-human:review-issue
🎭 Playwright Test Results❌ Some tests failed
Summary: 813 passed, 13 failed, 11 flaky, 262 skipped Failed Tests🍎 macOS
🪟 Windows
📋 Re-run Failing Tests (macOS)Copy and paste to re-run all failing spec files locally: npm run e2e \
e2e-tests/queued_message.spec.ts \
e2e-tests/setup_flow.spec.ts \
e2e-tests/visual_editing.spec.ts
|
- route Windows package-manager shims through cmd.exe inside the PTY - add signal-aware exits plus clearer timeout handling and output retention - tighten add-dependency probe timeouts and improve combined-output summaries Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
@BugBot run |
🤖 Claude Code Review SummaryPR Confidence: 5/5All trusted review threads are resolved, the review-fix patch is pushed, and the branch passes formatting, lint, type-checks, unit tests, packaging, and the targeted socket-firewall E2E spec. Unresolved ThreadsNo unresolved threads Resolved Threads
Product Principle SuggestionsNo suggestions 🤖 Generated by Claude Code |
🔍 Dyadbot Code Review SummaryVerdict: ⛔ NO — HIGH-severity UX regression on the timeout path Reviewed by 3 independent agents: Correctness Expert, Code Health Expert, UX Wizard. This is a follow-up review after the latest round of fixes. The previous multi-agent review's five MEDIUM findings have been addressed well — the Issues Summary
Why ":no_entry: NO" rather than "NOT SURE"The HIGH-severity finding is the load-bearing one: on a timeout where the PTY printed anything before hanging (which is essentially every real-world timeout — The two other MEDIUM findings are both follow-on concerns from design choices in the current fix:
Still worth noting (not inline)🟢 Low priority / code health notes (6 items)
🚫 Dropped / out-of-scope (2 items)
Generated by Dyadbot multi-agent code review |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
- surface PTY timeout reasons in captured output - keep Windows error display text on the original command - improve npm and pnpm failure summary selection - kill Windows PTY process trees on timeout Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
keppo-bot
bot
removed
the
needs-human:review-issue
|
@BugBot run |
🤖 Claude Code Review SummaryPR Confidence: 5/5The remaining trusted review feedback is addressed, the branch was pushed, and validation passed across lint, type-checking, unit tests, build packaging, and the real socket-firewall E2E. Unresolved ThreadsNo unresolved threads Resolved Threads
Product Principle SuggestionsNone. |
There was a problem hiding this comment.
1 issue found across 6 files (changes from recent commits).
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/ipc/utils/pty_command_runner.ts">
<violation number="1" location="src/ipc/utils/pty_command_runner.ts:168">
P2: Handle taskkill failure after spawn (non‑zero exit) so the PTY process is still killed on Windows timeouts. As written, only spawn errors trigger the fallback, leaving hung child processes if taskkill runs but fails.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| windowsHide: true, | ||
| }, | ||
| ); | ||
| taskkillProcess.once("error", () => { |
There was a problem hiding this comment.
P2: Handle taskkill failure after spawn (non‑zero exit) so the PTY process is still killed on Windows timeouts. As written, only spawn errors trigger the fallback, leaving hung child processes if taskkill runs but fails.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/ipc/utils/pty_command_runner.ts, line 168:
<comment>Handle taskkill failure after spawn (non‑zero exit) so the PTY process is still killed on Windows timeouts. As written, only spawn errors trigger the fallback, leaving hung child processes if taskkill runs but fails.</comment>
<file context>
@@ -122,6 +150,38 @@ function buildExitMessage(
+ windowsHide: true,
+ },
+ );
+ taskkillProcess.once("error", () => {
+ try {
+ ptyProcess.kill();
</file context>
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| displayedCommand: string, | ||
| timeoutMs: number, | ||
| ): string { | ||
| return `Command '${displayedCommand}' timed out after ${formatDuration(timeoutMs)}. The command may be stuck. Check your network or environment and try again.`; |
There was a problem hiding this comment.
🟡 MEDIUM | microcopy
Timeout guidance is cut off in the collapsed error card
The new timeout message is helpful but long. For pnpm add react, displaySummary becomes:
Command 'pnpm add react' timed out after 10 minutes. The command may be stuck. Check your network or environment and try again.
response_processor.ts:243 prepends Failed to add dependencies: react. , and DyadOutput.tsx:61 truncates the collapsed card title at 100 characters. For a typical install, the user sees ...timed out after 10 minutes. The comman... with the remediation hint cut off. For a longer package name like @typescript-eslint/eslint-plugin, the truncation eats the timed out phrase entirely — the user just sees a repeated command prefix and has to click to expand before they understand the command timed out.
💡 Suggestion: Front-load the timeout duration so the actionable info fits in the first ~100 characters even after the Failed to add dependencies: <pkgs>. prefix, e.g. Timed out after 10 minutes running 'pnpm add react'. Check your network or try again.
| reject( | ||
| new PtyCommandExecutionError({ | ||
| message: buildExitMessage(displayedCommand, exitCode, signal), | ||
| output, |
There was a problem hiding this comment.
🟡 MEDIUM | error-messaging
Signal-killed installs surface a progress line as the user-visible summary — the signal reason is dropped
When the PTY exits due to a signal (SIGKILL from OOM, user Ctrl+C, Electron shutting the renderer), this branch rejects with a descriptive message (Command 'pnpm add react' was terminated by signal 15) but passes the raw output through unchanged — unlike the timeout branch above, which appends its timeoutMessage to output via appendCommandMessage.
Downstream, getCommandExecutionDisplayDetails prefers any non-empty stdout, so ExecuteAddDependencyError.displayDetails becomes the raw captured PTY output. If that output is just pnpm's TTY progress frames (very common when pnpm is killed mid-resolve), getDisplaySummary walks backwards: no DISPLAY_SUMMARY_PATTERNS line matches, every line matches the ^progress: noise filter, so the function falls through to lines.at(-1) and returns a progress line. The user sees a red error card titled something like Failed to add dependencies: react. Progress: resolved 50, reused 0, downloaded 0, added 0 with no hint that the process was killed. The informative was terminated by signal N exists on error.message but never reaches displaySummary.
💡 Suggestion: Append buildExitMessage(...) to output on the failure path the same way buildTimeoutMessage is appended in the timeout branch, so both displayDetails and (transitively) displaySummary carry the real reason. Example:
const exitMessage = buildExitMessage(displayedCommand, exitCode, signal);
const finalOutput = appendCommandMessage(output, exitMessage);
settle(() => reject(new PtyCommandExecutionError({ message: exitMessage, output: finalOutput, exitCode, signal })));
🔍 Dyadbot Code Review SummaryVerdict: 🤔 NOT SURE - Potential issues Reviewed by 3 independent agents: Correctness Expert, Code Health Expert, UX Wizard. No HIGH issues. Three MEDIUM issues cluster around a single theme: the user-facing error output is still leaky in ways the new heuristics don't catch. Issues Summary
All three are filed as inline comments on the PR. 🟢 Low Priority Notes (10 items)Correctness & edge cases
Code health
🚫 Dropped / Not Flagged (1 item)
Generated by Dyadbot multi-agent code review |
Co-authored-by: devin-ai-integration[bot] <158243242+devin-ai-integration[bot]@users.noreply.github.com>
|
@BugBot run |
Co-authored-by: devin-ai-integration[bot] <158243242+devin-ai-integration[bot]@users.noreply.github.com>
|
@BugBot run |
- filter PTY progress noise out of add-dependency error details - fall back to the error message when only noisy PTY frames remain - keep response-processor coverage aligned with filtered error cards Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
@BugBot run |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2bacec8274
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| await runner("npx", [...SOCKET_FIREWALL_NPX_ARGS, "--help"], { | ||
| timeoutMs: SOCKET_FIREWALL_PROBE_TIMEOUT_MS, | ||
| }); | ||
| return { available: true }; | ||
| } catch { |
There was a problem hiding this comment.
The new 30s probe timeout is handled by the same catch path as a true "sfw not installed" failure, so a slow first-time npx fetch now marks Socket Firewall unavailable and allows dependency installs to continue without blocking unsafe packages. This creates a fail-open path for users with slower/unstable networks even when unsafe-package blocking is enabled; treat timeout/transient probe errors differently (retry or abort install) instead of silently downgrading protection.
Useful? React with 👍 / 👎.
🔍 Dyadbot Code Review SummaryVerdict: ✅ YES - Ready to merge Reviewed by 3 independent agents: Correctness Expert, Code Health Expert, UX Wizard. This PR has been through multiple prior review rounds, and the current HEAD addresses nearly all raised concerns. No new HIGH or MEDIUM blockers found. Issues SummaryNo new HIGH or MEDIUM issues. A handful of LOW polish items below — none blocking. 🟢 Low Priority Notes (12 items)Code clarity
Duplication / abstraction
Edge cases
Test gap
Microcopy / UX polish (mostly pre-existing, out of scope)
🚫 Dropped False Positives / Duplicates (8 items)
Generated by Dyadbot multi-agent code review |
1 participant
Summary
Test plan
🤖 Generated with Claude Code