fix(execd): preserve blank lines in command stdout SSE stream#902
Merged
hittyt merged 5 commits intoMay 19, 2026
Merged
Conversation
readFromPos previously skipped consecutive line terminators when the buffer was empty, dropping standalone newline lines from the SSE output. Emit "\n" for blank lines while keeping the existing line-content emit behavior (terminator is still stripped from non-empty lines). \r\n pairs are coalesced via lastWasCR to avoid duplicate blank emits. Adds a unit test covering blank lines, leading blank, and CRLF, and an end-to-end smoke test that runs `printf 'a\n\nb\n\n\nc\n'` and asserts the stdout event sequence preserves the blanks. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
![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: 0168239327
ℹ️ 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".
…indows Windows runCommand fired OnExecuteComplete immediately after closing the done channel, racing the tail goroutines that emit pending stdout/stderr SSE events. Clients that break on execution_complete then missed final output, e.g. the blank-line smoke assertion saw an empty sequence while the server log showed the events were emitted. Mirror the Linux path: track the tail goroutines with a sync.WaitGroup and wg.Wait() after close(done) so all buffered output drains before the completion event is sent. Also covers the cmd.Start failure path. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
readFromPos previously declared lastWasCR as a per-call local, so the CRLF coalescing only worked when \r and \n were read in the same invocation. When tailStdPipe polls a writer that flushes \r before \n (common on Windows/cmd), the \r and \n can land in separate polls; the second call starts with lastWasCR=false and emits a spurious "\n" blank line for the trailing \n. A bare blank \r\n line split across polls would surface as two blanks. Hoist the state into tailStdPipe and thread it through readFromPos so the CR detection survives between polls. Add regression tests covering split CRLF after content and split blank CRLF. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 37eda911c3
ℹ️ 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".
Replace POSIX printf with single-quoted format string with a python -c one-liner. cmd /C does not strip single quotes, so the previous command only worked on Windows runners that happened to have Git for Windows in PATH (MSYS2 argv pre-processing strips the quotes); on a bare Windows sandbox the smoke would fail before reaching the filesystem checks. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
The previous python -c attempt produced a SyntaxError on Windows because Go's syscall.EscapeArg wraps the cmd /C argument in quotes, escaping the inner quotes as \". cmd /C strips the outer quotes (rule 2 of its parser) but leaves the literal \" inside, and MSVCRT's argv parser then treats \" as a literal double-quote character without toggling quote state, so the first embedded space terminates argv[2] and python sees an unterminated string literal. Use a cmd-native echo chain on Windows (no inner quotes, & is sequential) and keep POSIX printf on Linux/macOS. The execd reader collapses CRLF to LF, so both platforms yield the same event sequence. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
readFromPos previously skipped consecutive line terminators when the buffer was empty, dropping standalone newline lines from the SSE output. Emit "\n" for blank lines while keeping the existing line-content emit behavior (terminator is still stripped from non-empty lines). \r\n pairs are coalesced via lastWasCR to avoid duplicate blank emits.
Adds a unit test covering blank lines, leading blank, and CRLF, and an end-to-end smoke test that runs
printf 'a\n\nb\n\n\nc\n'and asserts the stdout event sequence preserves the blanks.Testing
Breaking Changes
Checklist