Fix PTY deadline output drain#2139
Conversation
![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: 0810bbe31d
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } | ||
|
|
||
| if case .closed = readResult, !process.isRunning { break } | ||
| if ptyClosed { break } |
There was a problem hiding this comment.
When a non-Codex child closes or redirects all PTY fds but keeps running, this new break exits the poll loop immediately even though the configured timeout has not elapsed. The later finishSynchronously() only waits its 1s default, so a command like exec </dev/null >/dev/null 2>/dev/null; sleep 5; exit 0 with returnOnEmptyProcessExit now gets killed/reported as timed out instead of waiting for the process to exit and returning its status as before.
Useful? React with 👍 / 👎.
clawsweeper
Bot
added
rating: 🧂 unranked krab
|
Codex review: needs real behavior proof before merge. Reviewed July 13, 2026, 9:26 PM ET / July 14, 2026, 01:26 UTC. Summary Reproducibility: yes. The linked issue supplies a current-main zero-retry grouped-suite command and observed failures for both lost OAuth output and an incorrect immediate-exit result; the review comment also gives a deterministic compatibility case for the proposed patch. Review metrics: 2 noteworthy metrics.
Root-cause cluster Members:
Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything. Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Rank-up moves:
Proof guidance:
Risk before merge
Maintainer options:
Next step before merge
Security Review findings
Review detailsBest possible solution: Drain late readable output without equating PTY closure with child termination: continue waiting for a live child up to the configured deadline, add a regression for closed PTY descriptors followed by a delayed successful exit, and demonstrate both cases with a redacted live terminal transcript. Do we have a high-confidence way to reproduce the issue? Yes. The linked issue supplies a current-main zero-retry grouped-suite command and observed failures for both lost OAuth output and an incorrect immediate-exit result; the review comment also gives a deterministic compatibility case for the proposed patch. Is this the best way to solve the issue? No, not in its current form. A bounded output drain is appropriate, but PTY closure must not be treated as child exit while the process is still running within its configured deadline. Full review comments:
Overall correctness: patch is incorrect AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against b41715f3e3fb. Label changesLabel changes:
Label justifications:
Evidence reviewedWhat I checked:
Likely related people:
What the crustacean ranks mean
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics. How this review workflow works
Review history (1 earlier review cycle)
|
clawsweeper
Bot
added
the
merge-risk: 🚨 compatibility
kiranmagic7
force-pushed
the
kiran/tty-deadline-drain-20260714
branch
from
0810bbe to
911375c
Compare
|
Updated in
Tight local proof on the repaired head: Broader local proof on the repaired head: Hosted CI has restarted on @clawsweeper re-review |
|
🦞👀 Command router queued. I will update this comment with the next step. |
2 participants
Summary
Fixes #2134
Tests
swift test --filter TTYCommandRunnerEnvTests- passed, 28 tests in 1 suitedeadline drain preserves prompt output when first poll misses itPTY closure keeps waiting for child exit before deadlineswift test --skip-build --no-parallel --filter '(^CodexBarTests\.ClaudeEducationAvailabilityTests/|^CodexBarTests\.ClaudeExtraWindowQuotaWarningTests/|^CodexBarTests\.ClaudeLoginFlowTests/|^CodexBarTests\.ClaudeLoginRunnerTests/)'- passed, 12 tests in 4 suitesmake check- passed; existing app-locale missing-translation warnings onlygit diff --check- cleanswift test --filter AdaptiveRefreshTimerTestsandmake testfailed inAdaptiveRefreshTimerTests.swiftonmenu open advances a long idle timer during refresh without postponing an earlier tickwithCancellationError, outside the touched PTY files.Notes
This only changes the non-Codex PTY completion path. The extra drain window is bounded to 0.2-0.5s and runs without sending input, preserving late-readable output while still respecting a live child process through the configured deadline.