fix: resolve #802 — broker: add VT grid via alacritty_terminal (steal from ht, don't use libghostty)#836
Conversation
…erminal (steal from ht, don't use libghostty) Fixes AgentWorkforce#802 Signed-off-by: Dinh Truong (SlncTrZ) <46520299+SlncTrZ@users.noreply.github.com>
…erminal (steal from ht, don't use libghostty) Fixes AgentWorkforce#802 Signed-off-by: Dinh Truong (SlncTrZ) <46520299+SlncTrZ@users.noreply.github.com>
📝 WalkthroughWalkthroughThis PR integrates alacritty_terminal's VT parser into PtySession, adding shared terminal state tracking (grid and cursor) to parse ANSI sequences from PTY output in real time. Each session now maintains Term and Parser instances that are advanced with incoming byte chunks. ChangesTerminal State Tracking Integration
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
src/pty.rs (1)
30-31: 💤 Low valueConsider whether
parserfield is needed after fixing the integration.If switching to
alacritty_terminal::ansi::Processor, it can be instantiated locally in the reader thread since it doesn't need to be shared. Onlytermneeds to be stored for grid/cursor queries (per PR objectives).The
parserfield would become unused unless there's a specific need to access parser state externally.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pty.rs` around lines 30 - 31, The parser field (Arc<Mutex<Parser>>) appears unnecessary after switching to alacritty_terminal::ansi::Processor; remove the parser field from the struct and any related uses, keep only term: Arc<Mutex<Term>> for shared grid/cursor access, and instantiate a local Processor inside the reader thread (e.g., where the reader loop or spawn_reader is created) instead of sharing parser state; update any constructors, impls, and tests that referenced parser to use a locally created Processor or term access as appropriate.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Cargo.toml`:
- Line 41: The crate directly imports vte::Parser (see src/pty.rs import) but
Cargo.toml only relies on alacritty_terminal's transitive dependency; add an
explicit dependency entry vte = "0.15" to the [dependencies] section of
Cargo.toml so vte is guaranteed for direct use.
In `@src/pty.rs`:
- Around line 17-18: The code currently uses vte::Parser directly with
alacritty_terminal::term::Term, but Parser::advance requires a vte::Perform impl
and Term implements alacritty_terminal::ansi::Handler; replace the Parser usage
with alacritty_terminal::ansi::Processor which adapts a vte::Parser to work with
Term: change the parser field type to Processor (instead of vte::Parser), update
imports (remove vte::Parser import and add alacritty_terminal::ansi::Processor),
initialize the Processor at spawn (wrapping a new vte::Parser inside Processor),
and in the parsing loop replace the call parser_guard.advance(&mut *term_guard,
*byte) with processor.advance(&mut *term_guard, *byte) so the types align.
- Around line 125-127: Term::new is called with the wrong signature and a
by-value SizeInfo; change the call to pass a reference and a third event proxy
implementing EventListener (e.g. create or reuse an EventListener/EventProxy
type), i.e. construct SizeInfo as before, then call
Term::new(Default::default(), &size_info, event_proxy) where event_proxy
implements the alacritty_terminal::event::EventListener trait (or use an
existing adapter in your crate), and keep the Arc::new(Mutex::new(...)) wrapping
the resulting Term.
---
Nitpick comments:
In `@src/pty.rs`:
- Around line 30-31: The parser field (Arc<Mutex<Parser>>) appears unnecessary
after switching to alacritty_terminal::ansi::Processor; remove the parser field
from the struct and any related uses, keep only term: Arc<Mutex<Term>> for
shared grid/cursor access, and instantiate a local Processor inside the reader
thread (e.g., where the reader loop or spawn_reader is created) instead of
sharing parser state; update any constructors, impls, and tests that referenced
parser to use a locally created Processor or term access as appropriate.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 38117230-7360-42de-93df-213e2b92ffc0
📒 Files selected for processing (2)
Cargo.tomlsrc/pty.rs
| futures-lite = "2.6" | ||
| uuid = { version = "1.15", features = ["v4", "serde"] } | ||
| urlencoding = "2.1" | ||
| alacritty_terminal = "0.26" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if alacritty_terminal 0.26 exists and inspect its dependencies
curl -s "https://crates.io/api/v1/crates/alacritty_terminal/0.26.0" | jq '.version.num // "not found"'
# Check if vte is a dependency of alacritty_terminal
curl -s "https://crates.io/api/v1/crates/alacritty_terminal/0.26.0/dependencies" | jq '.dependencies[] | select(.crate_id == "vte") | {crate_id, req}'Repository: AgentWorkforce/relay
Length of output: 117
🏁 Script executed:
# Find and examine the src/pty.rs file to see vte imports and usage
find . -name "pty.rs" -type f | head -5Repository: AgentWorkforce/relay
Length of output: 77
🏁 Script executed:
# Check Cargo.toml to see current dependencies
cat Cargo.toml | grep -A 50 "^\[dependencies\]"Repository: AgentWorkforce/relay
Length of output: 1152
🏁 Script executed:
# Read src/pty.rs to see vte and alacritty_terminal imports and usage
cat -n src/pty.rsRepository: AgentWorkforce/relay
Length of output: 19847
🏁 Script executed:
# Check if there are any other direct vte imports in the codebase
rg "use vte::" --type rustRepository: AgentWorkforce/relay
Length of output: 92
🌐 Web query:
vte crate Parser API version 0.15.0 advance method
💡 Result:
In vte (crate) v0.15.0, the Parser advance method is: pub fn advance (&mut self, performer: &mut P, bytes: &[u8]) It “advanc[es] the parser state” and “requires a Perform implementation to handle the triggered actions.” [1] Related: Parser::advance_until_terminated is the same idea but stops once Perform::terminated becomes true after reading a byte, and returns how many bytes were read. [1][2] How to use it (typical shape): create a vte::Parser, implement vte::Perform for your terminal state, then call parser.advance(&mut performer, input_bytes). [3][1]
Citations:
- 1: https://docs.rs/vte/latest/vte/struct.Parser.html
- 2: https://docs.rs/vte/latest/vte/trait.Perform.html
- 3: https://docs.rs/vte/
Add vte as an explicit dependency to Cargo.toml.
The src/pty.rs file directly imports vte::Parser (line 18), but vte is not listed as an explicit dependency in Cargo.toml. While vte is currently available transitively through alacritty_terminal 0.26, relying on transitive dependencies for direct imports is fragile—if alacritty_terminal changes its internal dependency strategy, this will break.
Add vte = "0.15" to the [dependencies] section.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Cargo.toml` at line 41, The crate directly imports vte::Parser (see
src/pty.rs import) but Cargo.toml only relies on alacritty_terminal's transitive
dependency; add an explicit dependency entry vte = "0.15" to the [dependencies]
section of Cargo.toml so vte is guaranteed for direct use.
| use alacritty_terminal::term::{SizeInfo, Term}; | ||
| use vte::Parser; |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
alacritty_terminal ansi Processor advance Term usage
💡 Result:
In Alacritty’s codebase, ANSI escape parsing is handled by the vte::Parser wrapped by an alacritty_terminal::ansi::Processor. 1) What “Processor.advance” does - alacritty_terminal::ansi::Processor::advance(handler, byte, writer) constructs a small “Performer” and calls self.parser.advance(&mut performer, byte), i.e. it advances the underlying ANSI state machine one byte at a time and dispatches parsed actions to the provided handler/performer. [1] 2) How Alacritty uses “advance” for stream processing - The Processor itself is described as wrapping a crate::Parser “to ultimately call methods on a Handler”. [1] - In the vte crate used by Alacritty, the Processor struct also wraps a Parser and provides an advance method that feeds bytes into the parser; this is consistent with how Alacritty’s Processor.advance delegates to vte’s parser.advance. [2] 3) Term usage / “Term usage” in the Handler - Alacritty wires the ANSI “Handler” trait to its terminal state object Term: src/term/mod.rs implements ansi::Handler for Term. That is where parsed ANSI actions (e.g., cursor movement, attributes, etc.) are applied to the terminal grid/state. [3] 4) Example: Processor.advance being called byte-by-byte - Alacritty’s ansi.rs tests show a minimal pattern: create Processor::new, create a handler, then for each byte in an escape-sequence byte array call parser.advance(&mut handler, *byte, &mut Void). [1] 5) Notes on “advance_until_terminated” style APIs - In the vte parser implementation, sequence handling (e.g., for specific BSU/ESU CSI private modes) is done by scanning for termination/extension escapes in newly added bytes, rather than using an “advance_until_terminated” approach for those sequences; this indicates “advance” remains the fundamental byte-driven entry point, while some sequence types may have additional buffering/synchronization logic. [2] Answer to your phrase “ansi Processor advance Term usage” (interpreting it as: “what is Processor::advance and how does it interact with Term”): - Processor::advance consumes a single byte of the terminal stream, advances the ANSI parser state machine, and calls handler methods via a Performer wrapper; in Alacritty, the handler is implemented by Term, so parsed ANSI actions mutate Term’s terminal state (grid/cursor/modes/etc.). [1][3]
Citations:
- 1: https://github.com/alacritty/alacritty/blob/8d3f5f4e179b6fe55617dc72fe60fa1bdfd9b014/src/ansi.rs
- 2: https://github.com/alacritty/vte/blob/master/src/ansi.rs
- 3: https://github.com/alacritty/alacritty/wiki/ANSI-References
🏁 Script executed:
wc -l src/pty.rsRepository: AgentWorkforce/relay
Length of output: 79
🏁 Script executed:
cat -n src/pty.rs | head -200Repository: AgentWorkforce/relay
Length of output: 8053
🏁 Script executed:
cd src && grep -n "use.*Processor" *.rs || echo "No Processor import found"Repository: AgentWorkforce/relay
Length of output: 90
Critical compilation error: vte::Parser::advance cannot accept Term as argument.
At line 142, the code calls parser_guard.advance(&mut *term_guard, *byte), but vte::Parser::advance expects a type implementing the vte::Perform trait. alacritty_terminal::Term implements alacritty_terminal::ansi::Handler instead—these traits are incompatible, and this code will not compile.
The correct approach is to use alacritty_terminal::ansi::Processor, which wraps vte::Parser and adapts it to work with Term:
Suggested fix
-use alacritty_terminal::term::{SizeInfo, Term};
-use vte::Parser;
+use alacritty_terminal::ansi::Processor;
+use alacritty_terminal::term::{SizeInfo, Term};Then replace the parsing loop (lines 139–143):
- {
- let mut parser_guard = parser_clone.lock();
- let mut term_guard = term_clone.lock();
- for byte in &buf[..n] {
- parser_guard.advance(&mut *term_guard, *byte);
- }
- }
+ {
+ let mut processor_guard = processor_clone.lock();
+ let mut term_guard = term_clone.lock();
+ for byte in &buf[..n] {
+ processor_guard.advance(&mut *term_guard, *byte);
+ }
+ }And update the field definition (line 31):
- parser: Arc<Mutex<Parser>>,
+ processor: Arc<Mutex<Processor>>,Initialize at spawn (line 127):
- let parser = Arc::new(Mutex::new(Parser::new()));
+ let processor = Arc::new(Mutex::new(Processor::new()));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| use alacritty_terminal::term::{SizeInfo, Term}; | |
| use vte::Parser; | |
| use alacritty_terminal::ansi::Processor; | |
| use alacritty_terminal::term::{SizeInfo, Term}; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/pty.rs` around lines 17 - 18, The code currently uses vte::Parser
directly with alacritty_terminal::term::Term, but Parser::advance requires a
vte::Perform impl and Term implements alacritty_terminal::ansi::Handler; replace
the Parser usage with alacritty_terminal::ansi::Processor which adapts a
vte::Parser to work with Term: change the parser field type to Processor
(instead of vte::Parser), update imports (remove vte::Parser import and add
alacritty_terminal::ansi::Processor), initialize the Processor at spawn
(wrapping a new vte::Parser inside Processor), and in the parsing loop replace
the call parser_guard.advance(&mut *term_guard, *byte) with
processor.advance(&mut *term_guard, *byte) so the types align.
| let size_info = SizeInfo::new(cols.into(), rows.into(), 0.0, 0.0, 0.0, 0.0, false); | ||
| let term = Arc::new(Mutex::new(Term::new(Default::default(), size_info))); | ||
| let parser = Arc::new(Mutex::new(Parser::new())); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for SizeInfo::new signature in alacritty_terminal docs or source
curl -s "https://docs.rs/alacritty_terminal/0.26.0/alacritty_terminal/term/struct.SizeInfo.html" | grep -oP 'fn new\([^)]+\)' | head -5
# Check Term::new signature
curl -s "https://docs.rs/alacritty_terminal/0.26.0/alacritty_terminal/term/struct.Term.html" | grep -oP 'fn new\([^)]+\)' | head -5Repository: AgentWorkforce/relay
Length of output: 46
🏁 Script executed:
cat Cargo.toml | grep -A 5 alacritty_terminalRepository: AgentWorkforce/relay
Length of output: 216
🏁 Script executed:
head -150 src/pty.rs | tail -30Repository: AgentWorkforce/relay
Length of output: 1291
🏁 Script executed:
rg "SizeInfo::new\(" --context 2Repository: AgentWorkforce/relay
Length of output: 676
🏁 Script executed:
rg "Term::new\(" --context 2Repository: AgentWorkforce/relay
Length of output: 354
🌐 Web query:
alacritty_terminal 0.26 SizeInfo new constructor signature
💡 Result:
In alacritty_terminal (used by Alacritty), the SizeInfo constructor signature is: pub fn new( width: f32, height: f32, cell_width: f32, cell_height: f32, mut padding_x: f32, mut padding_y: f32, dynamic_padding: bool,) -> SizeInfo This is shown in the code added when SizeInfo was extracted into alacritty_terminal. [1] If you’re hitting a “0.26” mismatch: crates.io lists alacritty_terminal version 0.26.0 as the crate version (updated 2026-04-06), but the SizeInfo::new signature itself is defined in the Rust source (and is the same constructor shown above in the extracted implementation). [2]
Citations:
🌐 Web query:
alacritty_terminal 0.26 Term new constructor parameters
💡 Result:
For alacritty_terminal v0.26.0, the relevant “constructor parameters” are the fields of the Term configuration struct (Term::Config). In the Alacritty codebase, TermConfig is constructed with these parameters: semantic_escape_chars, scrolling_history, vi_mode_cursor_style, default_cursor_style, osc52, and kitty_keyboard. [1] So, if you’re updating code for 0.26-era Term construction, check that you now provide (or can derive) these TermConfig fields when calling into Term::new / building a Term from TermConfig. [2][1] Evidence: - TermConfig is used in alacritty’s UI config derivation code to build a TermConfig literal with the fields listed above. [1] - The docs.rs module for term describes that term::Config is “Configuration options for the Term.” [2] Note: the exact Term::new function signature (parameter order/types) wasn’t directly retrievable from the sources captured in this run; the authoritative, source-backed part we can confirm is the set of Term configuration fields used by Alacritty when constructing a Term for v0.26. [2][1]
Citations:
- 1: https://github.com/alacritty/alacritty/blob/master/alacritty/src/config/ui_config.rs
- 2: https://docs.rs/alacritty_terminal/latest/alacritty_terminal/term/index.html
🌐 Web query:
alacritty_terminal 0.26.0 Term::new signature exact parameters
💡 Result:
The exact signature of Term::new in alacritty_terminal 0.26.0 is pub fn new<T>(config: Config, dimensions: &D, event_proxy: T) -> Term<T> where D: Dimensions, T: EventListener.[1] This is shown in the source code snippet from the alacritty_terminal/src/term/mod.rs file on GitHub, which matches the structure for version 0.26.0 as the crate was updated on 2026-04-06.[2][1] The Term struct is documented in the term module on docs.rs.[3]
Citations:
- 1: https://github.com/alacritty/alacritty/blob/f99dc717/alacritty_terminal/src/term/mod.rs
- 2: https://crates.io/crates/alacritty_terminal/0.26.0
- 3: https://docs.rs/alacritty_terminal/latest/alacritty_terminal/term/index.html
🏁 Script executed:
rg "impl.*Default.*for.*Config\|impl.*Default.*for.*Term::Config" --context 3Repository: AgentWorkforce/relay
Length of output: 46
🏁 Script executed:
grep -r "Term::new" --include="*.rs" | head -20Repository: AgentWorkforce/relay
Length of output: 158
🏁 Script executed:
head -30 src/pty.rs | grep -E "^use |^mod "Repository: AgentWorkforce/relay
Length of output: 282
🏁 Script executed:
grep -A 2 -B 2 "Term::new" src/pty.rsRepository: AgentWorkforce/relay
Length of output: 299
Fix Term::new call—missing required event_proxy parameter and incorrect dimensions reference.
The Term::new signature in alacritty_terminal 0.26.0 requires three parameters:
pub fn new<T>(config: Config, dimensions: &D, event_proxy: T) -> Term<T>
where D: Dimensions, T: EventListenerThe current call at line 126 provides only two arguments and passes size_info by value instead of by reference:
Term::new(Default::default(), size_info)This will not compile. You must:
- Pass
size_infoby reference:&size_info - Provide an
event_proxyimplementation (third parameter) that implements theEventListenertrait
(The SizeInfo::new call is correct—the 7-argument signature with false as the final dynamic_padding parameter matches version 0.26.0.)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/pty.rs` around lines 125 - 127, Term::new is called with the wrong
signature and a by-value SizeInfo; change the call to pass a reference and a
third event proxy implementing EventListener (e.g. create or reuse an
EventListener/EventProxy type), i.e. construct SizeInfo as before, then call
Term::new(Default::default(), &size_info, event_proxy) where event_proxy
implements the alacritty_terminal::event::EventListener trait (or use an
existing adapter in your crate), and keep the Arc::new(Mutex::new(...)) wrapping
the resulting Term.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| let size_info = SizeInfo::new(cols.into(), rows.into(), 0.0, 0.0, 0.0, 0.0, false); | ||
| let term = Arc::new(Mutex::new(Term::new(Default::default(), size_info))); |
There was a problem hiding this comment.
🔴 SizeInfo constructed with zero cell_width and cell_height causes malformed terminal grid
At src/pty.rs:125 and src/pty.rs:185, SizeInfo::new is called with 0.0 for both cell_width and cell_height (the 3rd and 4th arguments). In alacritty_terminal, SizeInfo uses cell dimensions to compute the number of grid columns and rows (e.g., columns = (width - 2*padding) / cell_width). With cell_width = 0.0, this results in a division by zero producing f32::INFINITY, which when cast to usize saturates to usize::MAX. This would cause the Term to attempt allocating a grid with an astronomically large number of columns/rows, leading to either an OOM panic or a completely non-functional virtual terminal. The cell dimensions should be set to sensible non-zero values (e.g., 1.0 for both if you only care about character-grid dimensions and not pixel accuracy).
Prompt for agents
In src/pty.rs, SizeInfo::new is called at two locations (line 125 in spawn() and line 185 in resize()) with 0.0 for both cell_width and cell_height (the 3rd and 4th parameters). This causes division-by-zero in alacritty_terminal's grid dimension calculation, producing infinity which overflows to usize::MAX when computing columns/rows.
The fix: pass non-zero cell dimensions. If you only need character-grid tracking (not pixel-accurate rendering), use 1.0 for both cell_width and cell_height. Then the pixel width/height can match cols/rows directly:
SizeInfo::new(cols as f32, rows as f32, 1.0, 1.0, 0.0, 0.0, false)
Apply this fix at both callsites:
- src/pty.rs line 125 (in spawn())
- src/pty.rs line 185 (in resize())
Alternatively, if alacritty_terminal 0.26 has a column/row-based constructor, use that instead.
Was this helpful? React with 👍 or 👎 to provide feedback.
1 participant
Summary
fix: resolve #802 — broker: add VT grid via alacritty_terminal (steal from ht, don't use libghostty)
Problem
Severity:
Medium| File:Cargo.tomlAdd the
alacritty_terminalcrate to enable a real VT100‑compatible parser and grid. This dependency is pure Rust, cross‑platform, and production‑grade (used by Alacritty itself). No additional features are required beyond the default set, as we only need the terminal emulation state, not rendering or windowing.Solution
Under
[dependencies], add:Changes
Cargo.toml(modified)src/pty.rs(modified)Testing