Conversation
|
@codex review |
There was a problem hiding this comment.
Pull request overview
Adds a controlled-session PTY output pump that drains workload PTY output in bounded chunks and produces a stable drained/failed finalization result suitable for the lifecycle output-finalization barrier.
Changes:
- Introduce
PTYOutputPumpV1to read PTY output in fixed-size chunks, emit via a flow-controlled callback, and latch an immutable finalization status. - Add unit tests covering ordering, bounded chunking, timeout cancellation, error handling, and lifecycle barrier integration.
- Update the controlled-session design doc to reflect the new bounded draining/finalization behavior.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| internal/controlledsession/output.go | Implements bounded PTY output pumping with deadline-based finalization and immutable results. |
| internal/controlledsession/output_test.go | Adds coverage for delivery ordering, backpressure timeout behavior, and error/EOF handling. |
| docs/CONTROLLED_SESSION_DESIGN.md | Documents the new PTY output pump behavior and its interaction with the lifecycle barrier. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| }() | ||
| <-emitterEntered | ||
|
|
||
| result, err := pump.Finalize(time.Now().Add(20 * time.Millisecond)) |
| barrier and timeout outcome, are implemented; bounded output draining, | ||
| controlled-session networking, and Docker orchestration remain later slices | ||
| barrier and timeout outcome, are implemented. The synthetic PTY output pump | ||
| now preserves byte order with one bounded flow-control chunk, charges time |
|
Codex Review: Didn't find any major issues. Chef's kiss. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
|
@codex review |
![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: 9a23958e1c
ℹ️ 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 closed, closeErr := pump.closeSource(); closed && closeErr != nil { | ||
| pump.appendResultError(fmt.Errorf("close timed-out controlled-session PTY output: %w", closeErr)) |
There was a problem hiding this comment.
When the EOF path is already executing Close as the deadline expires and that close fails, this call returns closed == false with the shared non-nil closeErr, so the condition drops the error. The EOF path also cannot record it because the timeout result has already won the result latch, leaving PTYOutputFinalizationV1.Err with only context.DeadlineExceeded and hiding the underlying PTY adapter failure; append any non-nil close error even when another goroutine performed the close.
Useful? React with 👍 / 👎.
|
@codex review |
|
Codex Review: Didn't find any major issues. Nice work! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
Stack created with Sapling. Best reviewed with ReviewStack.