broker(pty): replace TerminalQueryParser with alacritty EventListener#879
Conversation
…#867) PR #836 wired the alacritty VT grid into every PtySession but passed VoidListener to Term::new, so alacritty parsed query sequences (DSR/DA1/DA2/CPR) and discarded the responses. A parallel hand-rolled TerminalQueryParser in helpers.rs answered those queries with a hardcoded 1;1 CPR reply that ignored the real cursor position. Replace VoidListener with RelayEventListener, which owns a std::sync::mpsc::Sender<Vec<u8>>. alacritty's send_event(PtyWrite) hands query response bytes to the channel; a dedicated drainer thread takes the writer lock and pushes them down the PTY. The listener is non-blocking so it is safe to call while processor+term locks are held, and the drainer exits when the listener is dropped at PtySession teardown. CPR responses now reflect the live cursor position (verified by the new unit test: ESC[3;5H ESC[6n yields ESC[3;5R, not ESC[1;1R). Delete the hand-rolled parser and its tests in helpers.rs, plus its three call sites in pty_worker.rs, wrap.rs, and the test module in main.rs. Make Snapshot::from_term generic over EventListener so it accepts both Term<RelayEventListener> from a live PtySession and the Term<VoidListener> used in offline tests. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughReplaces a hand-rolled terminal query parser with Alacritty's RelayEventListener. Adds a bounded mpsc writeback channel drained by a dedicated thread, generalizes Snapshot::from_term to any EventListener, removes parser code and tests, and adds wiring tests verifying DSR/DA1/CPR behavior. ChangesRelayEventListener Integration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
![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: c95a6095d0
ℹ️ 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".
| // (DSR/DA1/DA2/CPR). The listener is sync and non-blocking; the | ||
| // drainer thread below takes the writer lock and pushes the | ||
| // bytes down the PTY. | ||
| let (writeback_tx, writeback_rx) = std_mpsc::channel::<Vec<u8>>(); |
There was a problem hiding this comment.
Using std::sync::mpsc::channel here creates an unbounded buffer between parser events and the drainer thread. If the child emits many query sequences (for example repeated ESC[6n) while not draining stdin fast enough, the drainer can block on write_all and the reader thread will continue enqueueing Event::PtyWrite responses, causing memory usage to grow without limit and potentially OOM the broker.
Useful? React with 👍 / 👎.
| "status": "completed", | ||
| "startedAt": "2026-05-18T01:56:18.236Z", | ||
| "completedAt": "2026-05-18T02:01:49.991Z", | ||
| "path": "/Users/will/Projects/AgentWorkforce/relay/.claude/worktrees/agent-a496ab143c411ff7c/.trajectories/completed/2026-05/traj_piik8r6zu3i7.json" |
There was a problem hiding this comment.
This new index entry records a machine-specific absolute path (/Users/will/...) instead of the repo-relative .trajectories/... format used by neighboring entries. On other environments where that absolute path does not exist, consumers that read index paths directly cannot open this trajectory record even though the file is present in the repository.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Addresses PR #879 review: - Bound writeback channel via sync_channel(128) + try_send. Prevents unbounded memory growth if the child floods query sequences while the drainer is stuck on write_all. Overflow logs a warn and drops the response. - Add listener_answers_da1_with_vt102_ident regression test so DA1 (startup-critical for many CLIs) is exercised before the old TerminalQueryParser path is removed. - Strip absolute /Users/... paths from .trajectories metadata (index.json, traj_piik8r6zu3i7.json projectId) — repo-relative now. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Thanks for the reviews — pushed Fixed:
Declined (with reason):
CI green again (cubic + CodeRabbit), 616 tests pass, clippy clean. |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
1 participant
Closes #867.
Summary
VoidListenerwith a newRelayEventListenerthat captures alacrittyEvent::PtyWrite(DSR / DA1 / DA2 / CPR responses) and writes them back to the PTY's stdin via anstd::sync::mpscchannel drained by a dedicated thread.1;1the hand-rolled parser always returned.TerminalQueryParser,TerminalQueryState, andterminal_query_responsesfromhelpers.rs, plus all call sites inpty_worker.rs,wrap.rs, and themain.rstest module.Snapshot::from_termgeneric overEventListenerso offline snapshot tests still useVoidListenerwhile live capture usesRelayEventListener.Why this shape
send_eventruns insideProcessor::advance(sync), so the channel must be non-blocking.std::sync::mpsckeeps the listenerSync, never blocks, and avoids requiring a tokio runtime inPtySession::spawn. The drainer is astd::threadmatching the existing reader-thread shape; it exits whenterm(and its listener / sender) is dropped at session teardown.writerlock, but the reader thread does not holdwriterwhile inprocessor.advance, andresizedoesn't touchwritereither — so no deadlock with the existing reader.Test plan
cargo buildclean.cargo test— 615 pass / 2 ignored / 0 fail.cargo clippy --all-targets -- -D warningsclean.listener_answers_dsr_with_terminal_ok—ESC[5n→ESC[0nlands at the writer.listener_answers_cpr_with_real_cursor_position—ESC[3;5H ESC[6n→ESC[3;5R(proves CPR is grid-accurate, not hardcoded).🤖 Generated with Claude Code