fix: drain PTY output after process exit - #2807
Conversation
|
🦞👀 Pull request received. I will update this pull request when review starts. |
136dbe8 to
780367b
Compare
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 136dbe83ff
ℹ️ 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".
| guard try drainNonCodexOutput(for: Self.postExitDrainTimeout) else { | ||
| Self.log.warning("PTY did not close after process exit", metadata: ["binary": binaryName]) | ||
| throw Error.timedOut |
There was a problem hiding this comment.
When a CLI exits successfully after printing output but leaves a background helper/daemon inheriting the PTY fds, this new guard waits for EOF/EIO and then throws timedOut even though buffer already contains the command's result. The generic non-Codex path is used for provider probes such as Kiro and Claude/version checks, so wrappers that briefly leave a child holding stdout/stderr open now report timeouts and discard usable output; the closure requirement should only block empty-result classification or otherwise allow non-empty process-exit output after the drain timeout.
Useful? React with 👍 / 👎.
clawsweeper
Bot
added
rating: 🦐 gold shrimp
|
Codex review: needs maintainer review before merge. Reviewed August 9, 2026, 5:34 AM ET / 09:34 UTC. ClawSweeper reviewWhat this changesThe PR terminates and reaps residual PTY helper processes after a command exits, then drains remaining output through terminal closure before deciding whether the result is empty. Regression provenancePossible regression — probable (reviewed change; failure trace). No predecessor PR is attributed. Merge readinessKeep this owner-authored PR open for normal merge review. The latest branch addresses the prior concern by cleaning residual PTY helpers before terminal draining, while current main still has the post-exit drain path without that cleanup. Priority: P2 Review scores
Verification
How this fits togetherCodexBar runs provider and login commands inside a pseudo-terminal and feeds their captured output into status and version probes. The shared runner sits between process lifecycle events and Kiro, Claude, and Codex provider outcomes. flowchart LR
A[Provider probe] --> B[PTY command runner]
B --> C[Direct command exits]
C --> D[Residual helper cleanup]
D --> E[Terminal output drain]
E --> F[Captured command result]
F --> G[Provider status or login flow]
Before merge
Agent review detailsSecurityNone. Review metrics
Merge-risk optionsMaintainer options:
Technical reviewBest possible solution: Land the narrow residual-helper cleanup with the existing fast-exit and same-group-helper regressions, preserving successful buffered output for provider probes. Do we have a high-confidence way to reproduce the issue? Yes, source-reproducible: the supplied branch adds a 50-iteration fast-exit PTY regression and a same-group-helper cleanup case, although this read-only review did not execute the unavailable PR head. Is this the best way to solve the issue? Yes. Reaping only residual helpers that retain the owned PTY before draining is a narrower solution than discarding buffered output or weakening empty-result handling. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against f5a1f799186e. LabelsLabel justifications:
EvidenceWhat I checked:
Likely related people:
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
HistoryReview history (3 earlier review cycles)
|
clawsweeper
Bot
added
rating: 🐚 platinum hermit
1 participant
Summary
Root exit and reaping can precede PTY delivery. The shared runner now terminates and reaps residual same-group or output-holder helpers before draining, so they cannot keep the PTY slave open. The PTY master then drains through EOF/EIO before output is classified as empty, while genuinely empty exits remain supported.
The shared Kiro and Claude login/provider version paths are covered. Regressions include a 50-iteration fast-process-exit test and a same-group-helper cleanup test.
Testing
DEVELOPER_DIR=/Applications/Xcode-beta.app/Contents/Developer swift build --build-tests(passed)make check(passed with zero violations)tty runner cleans a same group helper after normal exit(passed)fast process exit drains buffered PTY output(passed for 50 iterations)post-exit drain processes trailing chunk through callback path(passed)