Fix Fish terminal capability query before xterm attaches - #1597
Conversation
Handle DA1 queries at the host PTY boundary so shells do not time out while the browser terminal is detached. Remove handled queries from replay output to prevent duplicate responses after reconnects. Amp-Thread-ID: https://ampcode.com/threads/T-019ffee9-be44-75c0-bd8a-1c653b61f62a Co-authored-by: Amp <amp@ampcode.com>
|
🚨 SLOP COP 🚨 · I am SlopCop. I am reviewing this pull request now. I will check security, code quality, architecture, performance, and test coverage. |
![bb-slop-cop[bot]](https://avatars.githubusercontent.com/in/4577281?s=60&v=4){kind=small}
| ); | ||
| session.pendingPrimaryDeviceAttributesQuery = result.pendingQuery; | ||
| for (let index = 0; index < result.queryCount; index += 1) { | ||
| session.pty.write(PRIMARY_DEVICE_ATTRIBUTES_RESPONSE); |
There was a problem hiding this comment.
🚨 slopcop/review — High: Batch and bound the automatic DA1 replies.
This loop calls pty.write once for each query. A 65,535-byte event can contain 21,845 ESC[c queries. Node-pty copies each reply into its unbounded Unix write queue. A terminal process can therefore create many queue items and exhaust memory in the shared host daemon.
Use one bounded write, or a small fixed number of bounded writes. Add a flood test that verifies the write count and response bytes.
There was a problem hiding this comment.
🚨 SLOP COP 🚨 · review
Plain-English summary
This change lets the host daemon answer Fish's early terminal identity query before the browser terminal starts. It removes handled queries from replay. This prevents Fish's startup timeout and duplicate browser answers. It also changes protocol version 119 to 120.
Finding
High — Batch and bound automatic DA1 replies.
At terminal-manager.ts:865-866, the code calls pty.write once for each matched query. A 65,535-byte event can hold 21,845 ESC[c queries. Node-pty copies every reply into an unbounded write queue. A terminal process can create many queue items and exhaust memory in the shared host daemon. This can affect other terminals.
Use one bounded write, or a small fixed number of bounded writes. Add a flood test that verifies bounded writes and correct response bytes.
Other review results
- The security review found no other issue.
- The code review found no correctness issue in the parser, replay logic, exit flush, or protocol change.
- The architecture scan found no duplicate host-side DA1 handler. The small local handler is suitable for one query. Use one headless emulator if detached protocol support expands.
- Exact-head Turbo tests passed: 557 host-daemon tests and 49 contract tests.
- Exact-head Turbo type checks passed for both changed packages.
- All current GitHub checks passed.
- A live Fish test was unavailable because Fish is not installed. The fake-PTY tests cover the pre-attach boundary behavior.
I used GitHub's comment-only review state. I did not approve or request changes.
1 participant
Summary
Handle Primary Device Attributes (DA1) queries at the host PTY boundary.
This prevents Fish from waiting 10 seconds and showing:
Problem
bb starts the host PTY before the browser terminal attaches. This lets terminal processes start immediately, retain their session, and buffer output while no terminal view is open.
During startup, Fish sends a DA1 query (
ESC[corESC[0c) and waits for a terminal emulator to answer. xterm.js supports this query, but it cannot answer before it attaches to the PTY.Forwarding the query through replay does not provide a safe fix. Replayed output can be processed again after reconnects, which could produce duplicate terminal responses.
Solution
The host daemon now:
ESC[?1;2cresponse directly to the PTY;This is intentionally a small protocol-specific handler. Adding a headless terminal emulator package would introduce parser state and a larger dependency for one startup query. If detached terminal emulation needs to support more queries later, the code comments identify
@xterm/headlessas the correct replacement.The host daemon protocol version increases from 119 to 120 so enrolled machines cannot use old terminal semantics with the updated server.
Verification
git diff --checkpassed