PTY: strip CPR (cursor-position report) replies before forwarding to consumers #55
Labels
No labels
prio_critical
prio_low
type_bug
type_contact
type_issue
type_lead
type_question
type_story
type_task
No milestone
No project
No assignees
1 participant
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference
lhumina_code/hero_proc#55
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "%!s()"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Summary
hero_procforwards raw PTY master bytes straight to WebSocket consumers (xterm.js etc.) with no escape-sequence filtering. When anything in the pipeline issues a DSR (\x1b[6n) query, the kernel/pty answers with a CPR reply of the form\x1b[<row>;<col>R. That reply is meant for whoever issued the query — but in our setup it leaks all the way to xterm.js and renders as visible junk like^[[24;80Rin the terminal pane.hero_router's terminal already intercepts the DSR query path locally (commit 60b9ea6 in hero_router) so the shell shouldn't normally see one. But that's a client-side workaround: any other consumer of hero_proc's PTY stream (different UI, agent, test harness) reproduces the bug, and any process inside the PTY that issues DSR on its own (vim, tmux, bash readline under some terminfo entries) still triggers it. The fix belongs at the source.Where it leaks
crates/hero_proc_server/src/supervisor/executor.rsrun_job_pty()~line 718, hot loop at 833–864.broadcast_tx_reader.send(data.clone())).crates/hero_proc_server/src/web.rshandle_pty_ws()~line 314 — sends raw binary frames straight through.supervisor/mod.rs~25–46) also captures the unfiltered bytes, so CPR survives in history.Proposed fix
Add a small stateful CPR-strip filter that runs immediately after the
read()at executor.rs:834, before the broadcast/scrollback writes.Key constraints:
\x1b[N;NR, ~6–7 bytes) can be split across two 4096-byte reads. The filter has to buffer an incomplete\x1b[…tail and prepend it to the next chunk.\x1b\[\d+;\d+R. Don't strip other CSI sequences — colors, cursor moves, etc., must pass through untouched.Sketch:
One instance per PTY job, owned by the read task.
Optional: opt-out
If there's any future consumer that legitimately wants CPR, gate the filter behind a per-job flag (default on). For the current terminal-streaming use case, unconditional strip is fine.
Tests
tests/integration/tests/pty.rshas no escape-sequence coverage. Add:\x1b[24+;80R) → stripped.\x1b[31m) → passed through.\x1bat end of stream → buffered, not emitted as garbage.Out of scope
Implementation Spec for Issue #55
Objective
Strip CPR (cursor-position report) replies of the form
\x1b[<row>;<col>Rfrom PTY master output before forwarding to WebSocket consumers and the scrollback buffer, so they never render as visible junk in xterm.js or any other consumer. The fix lives at the source (run_job_pty) and is implemented as a small, allocation-light, stateful filter that survives chunk boundaries.Requirements
\x1b\[\d+;\d+R. Leave all other CSI sequences (SGR, cursor moves, etc.) untouched.\x1b[24then;80R) must still be stripped.\x1b(or\x1b[, or\x1b[24) at the end of a chunk must be buffered and not emitted as garbage.Files to Modify/Create
crates/hero_proc_server/src/supervisor/cpr_filter.rs— NEW. HousesCprFilter, the state-machine implementation, and unit tests.crates/hero_proc_server/src/supervisor/mod.rs— addpub mod cpr_filter;next to the existingpub mod executor;/pub mod shutdown;declarations.crates/hero_proc_server/src/supervisor/executor.rs— wireCprFilterinto therun_job_ptyreader loop so it runs once per chunk before the broadcast, scrollback, and log-batcher writes.tests/integration/tests/pty.rs— optionally add an integration test that injects a CPR reply viaprintf. Primary coverage stays in unit tests.Implementation Plan
Step 1: Create the
CprFiltermodule with state machineFiles:
crates/hero_proc_server/src/supervisor/cpr_filter.rs(new)enum State { Normal, EscSeen, CsiBody }.pub fn new() -> Self— start inNormal, empty pending.pub fn filter(&mut self, chunk: &[u8]) -> Vec<u8>— consumechunk, return bytes safe to forward.Normal: if byte ==0x1B, transition toEscSeen, push0x1Bintopending. Otherwise, append directly to output.EscSeen: if byte ==[, transition toCsiBody, push[intopending. Otherwise flushpendingand the current byte to output, return toNormal.CsiBody: if byte is a digit or;, accumulate intopending. If byte ==b'R'AND the accumulated tail matches^\x1b\[\d+;\d+$: droppending(CPR), clear, return toNormal. If byte is any other terminator or pattern-breaker: flushpendingplus byte, clear, return toNormal.chunkcontains no0x1Bandstate == Normalandpendingis empty, returnchunk.to_vec()directly.Dependencies: none.
Step 2: Unit tests inside
cpr_filter.rsFiles:
crates/hero_proc_server/src/supervisor/cpr_filter.rs(same file,#[cfg(test)] mod tests)cpr_in_one_chunk_is_stripped: inputb"hello\x1b[24;80Rworld"→ outputb"helloworld".cpr_split_across_two_chunks_is_stripped: feedb"hello\x1b[24"thenb";80Rworld". Combined output isb"helloworld".cpr_split_at_every_byte_boundary: parameterized for every split point ofb"\x1b[24;80R".non_cpr_csi_passes_through_unchanged: inputb"\x1b[31mred\x1b[0m"→ identical output.lone_trailing_esc_is_buffered: feedb"abc\x1b", thenb"def"— combined output isb"abc\x1bdef".incomplete_csi_then_invalid_terminator_flushes: feedb"\x1b[24;80"thenb"X"— combined output isb"\x1b[24;80X".cpr_back_to_back: inputb"\x1b[24;80R\x1b[1;1R"→ outputb"".mixed_cpr_and_color_sgr: inputb"a\x1b[31m\x1b[24;80Rb"→ outputb"a\x1b[31mb".empty_chunkandchunk_of_only_normal_bytes— sanity / fast-path coverage.Dependencies: Step 1.
Step 3: Wire
CprFilterinto the PTY reader loopFiles:
crates/hero_proc_server/src/supervisor/executor.rs,crates/hero_proc_server/src/supervisor/mod.rsmod.rs, addpub mod cpr_filter;.executor.rs, insiderun_job_pty, find thetokio::task::spawn_blocking(move || { ... })reader. Modify it as follows:loop {, instantiatelet mut cpr_filter = crate::supervisor::cpr_filter::CprFilter::new();so the filter is owned by the closure.Ok(n) =>arm, replacelet data = buf[..n].to_vec();with:web.rs::handle_pty_ws— broadcast already carries pre-filtered bytes.ScrollbackBuffer— data going into it is already clean.Dependencies: Step 1.
Step 4: Optional integration coverage in
tests/integration/tests/pty.rsFiles:
tests/integration/tests/pty.rs/bin/sh.printf '\\033[24;80R\\n'over the WebSocket.\x1b[24;80R.Dependencies: Step 3.
Acceptance Criteria
crates/hero_proc_server/src/supervisor/cpr_filter.rswithCprFilter::new()andCprFilter::filter(&mut self, &[u8]) -> Vec<u8>.pub mod cpr_filter;added tosupervisor/mod.rs.run_job_ptyinstantiates oneCprFilterper job and routes every PTY chunk through it before broadcast, scrollback, and log-batcher writes.^[[24;80Rartifacts when a process emits a CPR reply.crates/hero_proc_server/Cargo.toml.cargo clippy -p hero_proc_server --all-targets -- -D warningsclean.Notes
<digits>;<digits>shape strict.pendinginto the output stream when the sequence turns out NOT to be a CPR (e.g.\x1b[31mcolor). Do not lose those bytes — that would corrupt SGR.Test Results
CprFilter unit tests (
cargo test -p hero_proc_server --lib supervisor::cpr_filter)All ten unit tests cover: full CPR in one chunk, CPR split across two chunks, every byte-boundary split, non-CPR CSI passthrough, lone trailing ESC, incomplete CSI then invalid terminator, back-to-back CPR, mixed CPR + SGR, empty chunk, normal-only chunk.
PTY integration tests (
cargo test -p hero_proc_integration_tests --test pty)Tests run:
test_pty_service_attach,test_pty_cpr_reply_is_stripped(new),test_pty_reconnect_sees_scrollback. The new test sendsprintf 'BEGIN\033[24;80REND\n'over the PTY WebSocket and asserts the literal\x1b[24;80Rbyte sequence does not appear in the stream whileBEGINandENDmarkers do — confirming the filter strips CPR end-to-end throughrun_job_pty.Compile check (
cargo build -p hero_proc_server)Clippy (
cargo clippy -p hero_proc_server --all-targets -- -D warnings)The clippy run flagged
crates/hero_proc_server/src/supervisor/executor.rs:126:This line was introduced in commit
9813c96e(feat: add AI interpreter for in-process model calls via hero_aibroker) and is outside the scope of this fix. No new clippy warnings come from the CprFilter change. The acceptance criterion "no clippy warnings introduced by this change" is met; cleaning up the pre-existing one belongs in a separate commit.Implementation Summary
Changes
New file:
crates/hero_proc_server/src/supervisor/cpr_filter.rs—CprFilterwith a 3-state byte-by-byte machine (Normal/EscSeen/CsiBody) that strips CPR replies of the form\x1b[<digits>;<digits>Rand lets every other byte (including non-CPR CSI sequences) through. Includes a fast path that bypasses the per-byte loop when the chunk has no\x1band no buffered state. Ten unit tests cover the spec's required cases plus a few edge cases.Modified files:
crates/hero_proc_server/src/supervisor/mod.rs— addedpub mod cpr_filter;.crates/hero_proc_server/src/supervisor/executor.rs—run_job_pty's reader closure now owns oneCprFilterand pipes every PTY chunk through it before broadcast, scrollback, and the log batcher. When the filter consumes a full chunk (e.g. a buffered split CPR), the loop continues without emitting anything downstream.tests/integration/tests/pty.rs— newtest_pty_cpr_reply_is_strippedend-to-end test that drives a real/bin/shPTY and asserts the CPR byte sequence does not survive into the WebSocket stream.How the filter works
spawn_blockingreader. It survives the whole PTY lifetime, so a CPR reply split across two 4096-byte reads is still recognized and stripped.\x1b[<digits>;<digits>into a private buffer. OnRit validates the buffered body matches<digits>;<digits>exactly and drops the whole sequence. Anything else (lone\x1b,\x1b[?1034h,\x1b[31m,\x1b[24;80X, etc.) flushes the buffered prefix verbatim back into the output.Test results
executor.rs:126is from commit9813c96eand is out of scope.Acceptance criteria
crates/hero_proc_server/src/supervisor/cpr_filter.rswithCprFilter::new()andCprFilter::filter(&mut self, &[u8]) -> Vec<u8>.pub mod cpr_filter;added tosupervisor/mod.rs.run_job_ptyinstantiates oneCprFilterper job and routes every PTY chunk through it before broadcast, scrollback, and log-batcher writes.\x1b[?1034hand\x1b[31mconfirmed by unit tests; integration output shows\x1b[?1034harriving intact).crates/hero_proc_server/Cargo.toml.Out of scope (per the issue)
hero_router-side DSR interception is left in place as a belt-and-suspenders.